Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1654123002:
Invalidate the previous caret even if the parent text node is removed. (Closed)
DescriptionInvalidate the previous caret even if the parent text node is removed.
When the last character is removed from an input, the caret node is detached,
which is the anonymous #text node. However, FrameSelection still refer to
the previous caret node to invalidate the previous caret.
Before deleting the last character "a" >
INPUT 0x3be145763a50 (focused)
#shadow-root 0x3be145763b50
DIV 0x3be145763c20 ID="inner-editor" (editable)
* #text 0x3be145763f58 "a"
After deleting the last character "a" >
INPUT 0x3be145763a50 (focused)
#shadow-root 0x3be145763b50
* DIV 0x3be145763c20 ID="inner-editor" (editable)
Currently, Blink doesn't invalidate the previous caret if the caret is detached
from DOM. This CL invalidates the caret region when the caret is detached.
TEST=webkit_unit_tests FrameSelectionTest.InvalidatePreviousCaretAfterRemovingLastCharacter
ManualTests/caret-paint-after-last-text-is-removed.html with --enable-partial-raster
BUG=528922
Committed: https://crrev.com/26006147cabc6a22d94afc81bdd657328cdc7cb7
Cr-Commit-Position: refs/heads/master@{#377696}
Patch Set 1 #
Total comments: 4
Patch Set 2 : add manual tests #Patch Set 3 : more clear code #
Total comments: 1
Patch Set 4 : more polishing if statement #
Total comments: 2
Patch Set 5 : add only test #Patch Set 6 : handle it in FrameSelection::nodeWillBeRemoved #Patch Set 7 : handle layouttests failure #
Total comments: 2
Patch Set 8 : add FrameSelectionTest.InvalidatePreviousCaretAfterRemovingLastCharacter #
Total comments: 2
Patch Set 9 : patch for land #
Total comments: 3
Patch Set 10 : patch for land v2 #
Messages
Total messages: 53 (23 generated)
Description was changed from ========== Invalidate the previous caret even if the parent text node is removed. When the last character is removed from a text box, the caret loses the parent node, which is the Blink-generated inline text node. However, FrameSelection still refer to the previous caret node to invalidate the previous caret. Currently, Blink doesn't invalidate the previous caret if the caret is detached from DOM. This CL fixs the bug. TEST=./out/Release/chrome --enable-partial-raster ./input_partial_raster_bug.html BUG=528922 ========== to ========== Invalidate the previous caret even if the parent text node is removed. When the last character is removed from a text box, the caret loses the parent node, which is the Blink-generated inline text node. However, FrameSelection still refer to the previous caret node to invalidate the previous caret. Currently, Blink doesn't invalidate the previous caret if the caret is detached from DOM. This CL fixs the bug. TEST=./out/Release/chrome --enable-partial-raster ./input_partial_raster_bug.html BUG=528922 ==========
dongseong.hwang@intel.com changed reviewers: + jbroman@chromium.org, yosin@chromium.org
yosin, could you review? I test it using input_partial_raster_bug.html attached in https://code.google.com/p/chromium/issues/detail?id=528922#c18 jbroman's analysis in https://code.google.com/p/chromium/issues/detail?id=528922#c4 is very helpful to make this CL.
It seems we should hold global(?) caret rect rather than local caret rect with node which is origin of local caret rect. Could you add add your test in this CL? https://codereview.chromium.org/1654123002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/FrameSelection.cpp (right): https://codereview.chromium.org/1654123002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/FrameSelection.cpp:709: if (m_previousCaretNode && newNode && !m_previousCaretNode->parentNode()) nit: s/!m_previousCaretNode->parentNode()/!m_previousCaretNode->inDocument()/ to handle detached tree which contains |m_previousCaretNode|. https://codereview.chromium.org/1654123002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/FrameSelection.cpp:710: invalidateLocalCaretRect(newNode, m_previousCaretRect); This call seems to be wrong, since |m_previousCaretRect| is calculated from |m_previousCaretNode|. |newNode| and |m_previousCaretNode| aren't related. In the bug case, it is tightly related, |newNode| is formatted at |m_previousNode|.
Description was changed from ========== Invalidate the previous caret even if the parent text node is removed. When the last character is removed from a text box, the caret loses the parent node, which is the Blink-generated inline text node. However, FrameSelection still refer to the previous caret node to invalidate the previous caret. Currently, Blink doesn't invalidate the previous caret if the caret is detached from DOM. This CL fixs the bug. TEST=./out/Release/chrome --enable-partial-raster ./input_partial_raster_bug.html BUG=528922 ========== to ========== Invalidate the previous caret even if the parent text node is removed. When the last character is removed from a text box, the caret loses the parent node, which is the Blink-generated inline text node. However, FrameSelection still refer to the previous caret node to invalidate the previous caret. Currently, Blink doesn't invalidate the previous caret if the caret is detached from DOM. This CL fixs the bug. TEST=ManualTests/caret-paint-after-last-text-is-removed.html with --enable-partial-raster BUG=528922 ==========
On 2016/02/02 01:56:57, Yosi_UTC9 wrote: > It seems we should hold global(?) caret rect rather than local caret rect with > node which is origin of local caret rect. global caret rect is not enough. you concern about general case, and we don't know which layer should be invalidated. but we don't need to concern the general case. I explain below. > Could you add add your test in this CL? I add the test to Manual test like other similar caret tests, because the test requires user interaction. https://codereview.chromium.org/1654123002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/FrameSelection.cpp (right): https://codereview.chromium.org/1654123002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/FrameSelection.cpp:709: if (m_previousCaretNode && newNode && !m_previousCaretNode->parentNode()) On 2016/02/02 01:56:56, Yosi_UTC9 wrote: > nit: s/!m_previousCaretNode->parentNode()/!m_previousCaretNode->inDocument()/ > to handle detached tree which contains |m_previousCaretNode|. Done. https://codereview.chromium.org/1654123002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/FrameSelection.cpp:710: invalidateLocalCaretRect(newNode, m_previousCaretRect); On 2016/02/02 01:56:56, Yosi_UTC9 wrote: > This call seems to be wrong, since |m_previousCaretRect| is calculated from > |m_previousCaretNode|. |newNode| and |m_previousCaretNode| aren't related. In > the bug case, it is tightly related, |newNode| is formatted at |m_previousNode|. True. this code expects |newNode| is formatted at |m_previousNode|. However, this bug is only case that |m_previousNode| is detached. Let me explain why this case happens 1. when a user type characters, Blink creates a generated #text node belonging to the input node. the caret node belongs to the generated #text node. 2. when a user delete all characters, the generated #text node is deleted and Blink creates new caret node belonging to the input node. You concern about user moving selection to other node. However, in the case, |m_previousCaretNode| never be detached.
On 2016/02/02 at 14:58:46, dongseong.hwang wrote: > On 2016/02/02 01:56:57, Yosi_UTC9 wrote: > > It seems we should hold global(?) caret rect rather than local caret rect with > > node which is origin of local caret rect. > > global caret rect is not enough. you concern about general case, and we don't know which layer should be invalidated. > but we don't need to concern the general case. I explain below. > > > Could you add add your test in this CL? > > I add the test to Manual test like other similar caret tests, because the test requires user interaction. > > https://codereview.chromium.org/1654123002/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/editing/FrameSelection.cpp (right): > > https://codereview.chromium.org/1654123002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/editing/FrameSelection.cpp:709: if (m_previousCaretNode && newNode && !m_previousCaretNode->parentNode()) > On 2016/02/02 01:56:56, Yosi_UTC9 wrote: > > nit: s/!m_previousCaretNode->parentNode()/!m_previousCaretNode->inDocument()/ > > to handle detached tree which contains |m_previousCaretNode|. > > Done. > > https://codereview.chromium.org/1654123002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/editing/FrameSelection.cpp:710: invalidateLocalCaretRect(newNode, m_previousCaretRect); > On 2016/02/02 01:56:56, Yosi_UTC9 wrote: > > This call seems to be wrong, since |m_previousCaretRect| is calculated from > > |m_previousCaretNode|. |newNode| and |m_previousCaretNode| aren't related. In > > the bug case, it is tightly related, |newNode| is formatted at |m_previousNode|. > > True. this code expects |newNode| is formatted at |m_previousNode|. If so, we need to have if-statement to check this. I'm not sure how to do that and it is still incomplete. I think using |m_previousNode| and |m_previousCaretRect| are wrong. We can detach |m_previousNode| then attach |m_previousNode| somewhere other than original DOM position. In this case, invalidate rect from |m_previousNode|+|m_previousRect| isn't what we desire.
On 2016/02/03 04:49:15, Yosi_UTC9 wrote: > https://codereview.chromium.org/1654123002/diff/1/third_party/WebKit/Source/c... > > third_party/WebKit/Source/core/editing/FrameSelection.cpp:710: > invalidateLocalCaretRect(newNode, m_previousCaretRect); > > On 2016/02/02 01:56:56, Yosi_UTC9 wrote: > > > This call seems to be wrong, since |m_previousCaretRect| is calculated from > > > |m_previousCaretNode|. |newNode| and |m_previousCaretNode| aren't related. > In > > > the bug case, it is tightly related, |newNode| is formatted at > |m_previousNode|. > > > > True. this code expects |newNode| is formatted at |m_previousNode|. > If so, we need to have if-statement to check this. I'm not sure how to do that > and it is still incomplete. the if-statement has the intention "if (m_previousCaretNode && newNode && !m_previousCaretNode->inDocument())" > I think using |m_previousNode| and |m_previousCaretRect| are wrong. > We can detach |m_previousNode| then |m_previousNode| is already detached. > attach |m_previousNode| somewhere other than > original DOM position. We cannot know where |m_previousNode| originally belong to. inside iframe or inside some shadow dom? In general, it's nearly impossible unless keeping very big copy of dom tree. > In this case, invalidate rect from |m_previousNode|+|m_previousRect| isn't what > we desire. General solution is too complicated compared to getting benefit. Fortunately, "backspacing the last letter" is the only CASE in which |m_previousNode| is detached, as I explained in #6 So we can invalidate |m_previousRect| on the |newNode| only "if (m_previousCaretNode && newNode && !m_previousCaretNode->inDocument())".
On 2016/02/03 at 11:27:39, dongseong.hwang wrote: > On 2016/02/03 04:49:15, Yosi_UTC9 wrote: > > https://codereview.chromium.org/1654123002/diff/1/third_party/WebKit/Source/c... > > > third_party/WebKit/Source/core/editing/FrameSelection.cpp:710: > > invalidateLocalCaretRect(newNode, m_previousCaretRect); > > > On 2016/02/02 01:56:56, Yosi_UTC9 wrote: > > > > This call seems to be wrong, since |m_previousCaretRect| is calculated from > > > > |m_previousCaretNode|. |newNode| and |m_previousCaretNode| aren't related. > > In > > > > the bug case, it is tightly related, |newNode| is formatted at > > |m_previousNode|. > > > > > > True. this code expects |newNode| is formatted at |m_previousNode|. > > If so, we need to have if-statement to check this. I'm not sure how to do that > > and it is still incomplete. > > the if-statement has the intention > "if (m_previousCaretNode && newNode && !m_previousCaretNode->inDocument())" > > > I think using |m_previousNode| and |m_previousCaretRect| are wrong. > > We can detach |m_previousNode| then > > |m_previousNode| is already detached. > > > attach |m_previousNode| somewhere other than > > original DOM position. > > We cannot know where |m_previousNode| originally belong to. inside iframe or inside some shadow dom? > In general, it's nearly impossible unless keeping very big copy of dom tree. > > > In this case, invalidate rect from |m_previousNode|+|m_previousRect| isn't what > > we desire. > > General solution is too complicated compared to getting benefit. > Fortunately, "backspacing the last letter" is the only CASE in which |m_previousNode| is detached, as I explained in #6 > So we can invalidate |m_previousRect| on the |newNode| only "if (m_previousCaretNode && newNode && !m_previousCaretNode->inDocument())". This is very hacky and hard to understand code. This kind of heck will be source of confusion and attack. We're thinking re-architect caret implementation, e.g. using layer to avoid invalidation, but not conclude yet. Please try to seek clean and intuitive way to implement caret handling rather than sticking current implementation.
Description was changed from ========== Invalidate the previous caret even if the parent text node is removed. When the last character is removed from a text box, the caret loses the parent node, which is the Blink-generated inline text node. However, FrameSelection still refer to the previous caret node to invalidate the previous caret. Currently, Blink doesn't invalidate the previous caret if the caret is detached from DOM. This CL fixs the bug. TEST=ManualTests/caret-paint-after-last-text-is-removed.html with --enable-partial-raster BUG=528922 ========== to ========== Invalidate the previous caret even if the parent text node is removed. When the last character is removed from an input, the caret node is detached, which is the anonymous #text node. However, FrameSelection still refer to the previous caret node to invalidate the previous caret. Before deleting the last character "a" > INPUT 0x3be145763a50 (focused) #shadow-root 0x3be145763b50 DIV 0x3be145763c20 ID="inner-editor" (editable) * #text 0x3be145763f58 "a" After deleting the last character "a" > INPUT 0x3be145763a50 (focused) #shadow-root 0x3be145763b50 * DIV 0x3be145763c20 ID="inner-editor" (editable) In this case, Blink invalidates the caret of #text using parent "inner-editor" node. TEST=ManualTests/caret-paint-after-last-text-is-removed.html with --enable-partial-raster BUG=528922 ==========
Description was changed from ========== Invalidate the previous caret even if the parent text node is removed. When the last character is removed from an input, the caret node is detached, which is the anonymous #text node. However, FrameSelection still refer to the previous caret node to invalidate the previous caret. Before deleting the last character "a" > INPUT 0x3be145763a50 (focused) #shadow-root 0x3be145763b50 DIV 0x3be145763c20 ID="inner-editor" (editable) * #text 0x3be145763f58 "a" After deleting the last character "a" > INPUT 0x3be145763a50 (focused) #shadow-root 0x3be145763b50 * DIV 0x3be145763c20 ID="inner-editor" (editable) In this case, Blink invalidates the caret of #text using parent "inner-editor" node. TEST=ManualTests/caret-paint-after-last-text-is-removed.html with --enable-partial-raster BUG=528922 ========== to ========== Invalidate the previous caret even if the #text node is detached. When the last character is removed from an input, the caret node is detached, which is the anonymous #text node. However, FrameSelection still refer to the previous caret node to invalidate the previous caret. Before deleting the last character "a" > INPUT 0x3be145763a50 (focused) #shadow-root 0x3be145763b50 DIV 0x3be145763c20 ID="inner-editor" (editable) * #text 0x3be145763f58 "a" After deleting the last character "a" > INPUT 0x3be145763a50 (focused) #shadow-root 0x3be145763b50 * DIV 0x3be145763c20 ID="inner-editor" (editable) In this case, Blink invalidates the caret of #text using parent "inner-editor" node. TEST=ManualTests/caret-paint-after-last-text-is-removed.html with --enable-partial-raster BUG=528922 ==========
On 2016/02/04 01:29:44, Yosi_UTC9 wrote: > This is very hacky and hard to understand code. This kind of heck will be source > of confusion and attack. > We're thinking re-architect caret implementation, e.g. using layer to avoid > invalidation, but not conclude yet. > Please try to seek clean and intuitive way to implement caret handling rather > than sticking current implementation. Agree. The new patch set clearly says this case by code. I change the description to explain it better. Could you review again?
https://codereview.chromium.org/1654123002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/FrameSelection.cpp (right): https://codereview.chromium.org/1654123002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/FrameSelection.cpp:723: if (m_previousCaretNode && newNode == m_parentOfPreviousCaretNode && !m_previousCaretNode->inDocument()) This condition isn't enough, e.g. - ancestor of |m_previousCaretNode| can be detached from document. - |m_previousCaretNode| can be move into another tree location. I don't think this approach is good.
Patchset #4 (id:60001) has been deleted
https://codereview.chromium.org/1654123002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/FrameSelection.cpp (right): https://codereview.chromium.org/1654123002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/FrameSelection.cpp:724: && newNode == m_parentOfPreviousCaretNode.get()) { > This condition isn't enough, e.g. > - ancestor of |m_previousCaretNode| can be detached from document. It means |newNode| is also detached from document. If so, invalidateLocalCaretRect() do nothing. > - |m_previousCaretNode| can be move into another tree location. the condition checks m_previousCaretNode->parentNode() > I don't think this approach is good. Could you review again? I improve little more. If we decide refactoring wide range of code, I have a idea. All editable element must has anonymous #text node from creation time. However, there might be unexpected side effect. e.g. when javascript sets 'editable' to any element, Blink has to create and add additional #text node for box element, while inline element become editable by itself. caretBrowsingEnabled is also big obstacle. To be honest, I'm a bit afraid to touch well-working legacy editor code without concrete knowledge.
On 2016/02/05 10:25:47, dshwang wrote: > https://codereview.chromium.org/1654123002/diff/80001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/editing/FrameSelection.cpp (right): > > https://codereview.chromium.org/1654123002/diff/80001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/editing/FrameSelection.cpp:724: && newNode == > m_parentOfPreviousCaretNode.get()) { > > This condition isn't enough, e.g. > > - ancestor of |m_previousCaretNode| can be detached from document. > > It means |newNode| is also detached from document. If so, > invalidateLocalCaretRect() do nothing. > > > - |m_previousCaretNode| can be move into another tree location. > > the condition checks m_previousCaretNode->parentNode() > > > I don't think this approach is good. > > Could you review again? I improve little more. > > If we decide refactoring wide range of code, I have a idea. > All editable element must has anonymous #text node from creation time. However, > there might be unexpected side effect. e.g. when javascript sets 'editable' to > any element, Blink has to create and add additional #text node for box element, > while inline element become editable by itself. caretBrowsingEnabled is also big > obstacle. To be honest, I'm a bit afraid to touch well-working legacy editor > code without concrete knowledge. PTAL
https://codereview.chromium.org/1654123002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/FrameSelection.cpp (right): https://codereview.chromium.org/1654123002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/FrameSelection.cpp:724: && newNode == m_parentOfPreviousCaretNode.get()) { On 2016/02/05 at 10:25:47, dshwang wrote: > > This condition isn't enough, e.g. > > - ancestor of |m_previousCaretNode| can be detached from document. > > It means |newNode| is also detached from document. If so, invalidateLocalCaretRect() do nothing. > > > - |m_previousCaretNode| can be move into another tree location. > > the condition checks m_previousCaretNode->parentNode() > > > I don't think this approach is good. > > Could you review again? I improve little more. > > If we decide refactoring wide range of code, I have a idea. > All editable element must has anonymous #text node from creation time. However, there might be unexpected side effect. e.g. when javascript sets 'editable' to any element, Blink has to create and add additional #text node for box element, while inline element become editable by itself. caretBrowsingEnabled is also big obstacle. To be honest, I'm a bit afraid to touch well-working legacy editor code without concrete knowledge. I would like to have radical change. Current implementation of Caret handling doesn't match rendering pipeline concept. We don't have clear idea how caret to be implemented, but we have some idea: - paint caret in its own layer; we don't need to remember previous rectangle(?) - calculate caret height based on a character will be inserted - integrated into rendering pipeline and live with lifecycle == calculation of caret rectangle just before LayoutClean - ... and more ... It is a time to think about caret re-implementation. I would like to leave current code as is until we have concrete plan.
On 2016/02/12 at 03:53:13, Yosi_UTC9 wrote: > https://codereview.chromium.org/1654123002/diff/80001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/editing/FrameSelection.cpp (right): > > https://codereview.chromium.org/1654123002/diff/80001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/editing/FrameSelection.cpp:724: && newNode == m_parentOfPreviousCaretNode.get()) { > On 2016/02/05 at 10:25:47, dshwang wrote: > > > This condition isn't enough, e.g. > > > - ancestor of |m_previousCaretNode| can be detached from document. > > > > It means |newNode| is also detached from document. If so, invalidateLocalCaretRect() do nothing. > > > > > - |m_previousCaretNode| can be move into another tree location. > > > > the condition checks m_previousCaretNode->parentNode() > > > > > I don't think this approach is good. > > > > Could you review again? I improve little more. > > > > If we decide refactoring wide range of code, I have a idea. > > All editable element must has anonymous #text node from creation time. However, there might be unexpected side effect. e.g. when javascript sets 'editable' to any element, Blink has to create and add additional #text node for box element, while inline element become editable by itself. caretBrowsingEnabled is also big obstacle. To be honest, I'm a bit afraid to touch well-working legacy editor code without concrete knowledge. > > I would like to have radical change. Current implementation of Caret handling doesn't match rendering pipeline concept. > We don't have clear idea how caret to be implemented, but we have some idea: > - paint caret in its own layer; we don't need to remember previous rectangle(?) > - calculate caret height based on a character will be inserted > - integrated into rendering pipeline and live with lifecycle == calculation of caret rectangle just before LayoutClean > - ... and more ... > > It is a time to think about caret re-implementation. I would like to leave current code as is until we have concrete plan. Before radical change, it is worth to change caret painting to use Content coordinate instead of Node + Node local coordinate. http://dev.chromium.org/developers/design-documents/blink-coordinate-spaces This issue and other life time issue of Node/LayoutObject come from holding Node in caret. Regardless how we'll paint caret or calculating caret height, using Content coordinate is required thing.
On 2016/02/12 04:11:51, Yosi_UTC9 wrote: > On 2016/02/12 at 03:53:13, Yosi_UTC9 wrote: > > > https://codereview.chromium.org/1654123002/diff/80001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/editing/FrameSelection.cpp (right): > > > > > https://codereview.chromium.org/1654123002/diff/80001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/editing/FrameSelection.cpp:724: && newNode == > m_parentOfPreviousCaretNode.get()) { > > On 2016/02/05 at 10:25:47, dshwang wrote: > > > > This condition isn't enough, e.g. > > > > - ancestor of |m_previousCaretNode| can be detached from document. > > > > > > It means |newNode| is also detached from document. If so, > invalidateLocalCaretRect() do nothing. > > > > > > > - |m_previousCaretNode| can be move into another tree location. > > > > > > the condition checks m_previousCaretNode->parentNode() > > > > > > > I don't think this approach is good. > > > > > > Could you review again? I improve little more. > > > > > > If we decide refactoring wide range of code, I have a idea. > > > All editable element must has anonymous #text node from creation time. > However, there might be unexpected side effect. e.g. when javascript sets > 'editable' to any element, Blink has to create and add additional #text node for > box element, while inline element become editable by itself. > caretBrowsingEnabled is also big obstacle. To be honest, I'm a bit afraid to > touch well-working legacy editor code without concrete knowledge. > > > > I would like to have radical change. Current implementation of Caret handling > doesn't match rendering pipeline concept. > > We don't have clear idea how caret to be implemented, but we have some idea: > > - paint caret in its own layer; we don't need to remember previous > rectangle(?) > > - calculate caret height based on a character will be inserted > > - integrated into rendering pipeline and live with lifecycle == calculation > of caret rectangle just before LayoutClean > > - ... and more ... > > > > It is a time to think about caret re-implementation. I would like to leave > current code as is until we have concrete plan. > > Before radical change, it is worth to change caret painting to use Content > coordinate instead of Node + Node local coordinate. > http://dev.chromium.org/developers/design-documents/blink-coordinate-spaces > > This issue and other life time issue of Node/LayoutObject come from holding Node > in caret. Regardless how we'll paint caret > or calculating caret height, using Content coordinate is required thing. I understand your plan. However, this bug is blocker of partial raster, which significantly speeds up rasterization and saves battery. Meanwhile Android and ChromeOS drains battery. I think we need to approach more practically. My suggestion is 1. land this CL 2. and then re-implement caret
On 2016/02/16 at 18:54:15, dongseong.hwang wrote: > On 2016/02/12 04:11:51, Yosi_UTC9 wrote: > > On 2016/02/12 at 03:53:13, Yosi_UTC9 wrote: > > > > > https://codereview.chromium.org/1654123002/diff/80001/third_party/WebKit/Sour... > > > File third_party/WebKit/Source/core/editing/FrameSelection.cpp (right): > > > > > > > > https://codereview.chromium.org/1654123002/diff/80001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/core/editing/FrameSelection.cpp:724: && newNode == > > m_parentOfPreviousCaretNode.get()) { > > > On 2016/02/05 at 10:25:47, dshwang wrote: > > > > > This condition isn't enough, e.g. > > > > > - ancestor of |m_previousCaretNode| can be detached from document. > > > > > > > > It means |newNode| is also detached from document. If so, > > invalidateLocalCaretRect() do nothing. > > > > > > > > > - |m_previousCaretNode| can be move into another tree location. > > > > > > > > the condition checks m_previousCaretNode->parentNode() > > > > > > > > > I don't think this approach is good. > > > > > > > > Could you review again? I improve little more. > > > > > > > > If we decide refactoring wide range of code, I have a idea. > > > > All editable element must has anonymous #text node from creation time. > > However, there might be unexpected side effect. e.g. when javascript sets > > 'editable' to any element, Blink has to create and add additional #text node for > > box element, while inline element become editable by itself. > > caretBrowsingEnabled is also big obstacle. To be honest, I'm a bit afraid to > > touch well-working legacy editor code without concrete knowledge. > > > > > > I would like to have radical change. Current implementation of Caret handling > > doesn't match rendering pipeline concept. > > > We don't have clear idea how caret to be implemented, but we have some idea: > > > - paint caret in its own layer; we don't need to remember previous > > rectangle(?) > > > - calculate caret height based on a character will be inserted > > > - integrated into rendering pipeline and live with lifecycle == calculation > > of caret rectangle just before LayoutClean > > > - ... and more ... > > > > > > It is a time to think about caret re-implementation. I would like to leave > > current code as is until we have concrete plan. > > > > Before radical change, it is worth to change caret painting to use Content > > coordinate instead of Node + Node local coordinate. > > http://dev.chromium.org/developers/design-documents/blink-coordinate-spaces > > > > This issue and other life time issue of Node/LayoutObject come from holding Node > > in caret. Regardless how we'll paint caret > > or calculating caret height, using Content coordinate is required thing. > > I understand your plan. > > However, this bug is blocker of partial raster, which significantly speeds up rasterization and saves battery. > Meanwhile Android and ChromeOS drains battery. > I think we need to approach more practically. > My suggestion is > 1. land this CL > 2. and then re-implement caret I don't agree with this approach. New node and previous caret rectangle are disconnected logically. Cluster Fuzz can find the hole of your check. But, it isn't crash and we just invalidate wrong thing. How about remembering absolute caret bounds instead of caret painter node + local caret rect? It can be bigger than this patch but it is reasonable and align to future direction.
On 2016/02/17 at 04:29:26, Yosi_UTC9 wrote: > On 2016/02/16 at 18:54:15, dongseong.hwang wrote: > > On 2016/02/12 04:11:51, Yosi_UTC9 wrote: > > > On 2016/02/12 at 03:53:13, Yosi_UTC9 wrote: > > > > > > > https://codereview.chromium.org/1654123002/diff/80001/third_party/WebKit/Sour... > > > > File third_party/WebKit/Source/core/editing/FrameSelection.cpp (right): > > > > > > > > > > > https://codereview.chromium.org/1654123002/diff/80001/third_party/WebKit/Sour... > > > > third_party/WebKit/Source/core/editing/FrameSelection.cpp:724: && newNode == > > > m_parentOfPreviousCaretNode.get()) { > > > > On 2016/02/05 at 10:25:47, dshwang wrote: > > > > > > This condition isn't enough, e.g. > > > > > > - ancestor of |m_previousCaretNode| can be detached from document. > > > > > > > > > > It means |newNode| is also detached from document. If so, > > > invalidateLocalCaretRect() do nothing. > > > > > > > > > > > - |m_previousCaretNode| can be move into another tree location. > > > > > > > > > > the condition checks m_previousCaretNode->parentNode() > > > > > > > > > > > I don't think this approach is good. > > > > > > > > > > Could you review again? I improve little more. > > > > > > > > > > If we decide refactoring wide range of code, I have a idea. > > > > > All editable element must has anonymous #text node from creation time. > > > However, there might be unexpected side effect. e.g. when javascript sets > > > 'editable' to any element, Blink has to create and add additional #text node for > > > box element, while inline element become editable by itself. > > > caretBrowsingEnabled is also big obstacle. To be honest, I'm a bit afraid to > > > touch well-working legacy editor code without concrete knowledge. > > > > > > > > I would like to have radical change. Current implementation of Caret handling > > > doesn't match rendering pipeline concept. > > > > We don't have clear idea how caret to be implemented, but we have some idea: > > > > - paint caret in its own layer; we don't need to remember previous > > > rectangle(?) > > > > - calculate caret height based on a character will be inserted > > > > - integrated into rendering pipeline and live with lifecycle == calculation > > > of caret rectangle just before LayoutClean > > > > - ... and more ... > > > > > > > > It is a time to think about caret re-implementation. I would like to leave > > > current code as is until we have concrete plan. > > > > > > Before radical change, it is worth to change caret painting to use Content > > > coordinate instead of Node + Node local coordinate. > > > http://dev.chromium.org/developers/design-documents/blink-coordinate-spaces > > > > > > This issue and other life time issue of Node/LayoutObject come from holding Node > > > in caret. Regardless how we'll paint caret > > > or calculating caret height, using Content coordinate is required thing. > > > > I understand your plan. > > > > However, this bug is blocker of partial raster, which significantly speeds up rasterization and saves battery. > > Meanwhile Android and ChromeOS drains battery. > > I think we need to approach more practically. > > My suggestion is > > 1. land this CL > > 2. and then re-implement caret > > I don't agree with this approach. New node and previous caret rectangle are disconnected logically. Cluster Fuzz > can find the hole of your check. But, it isn't crash and we just invalidate wrong thing. > > How about remembering absolute caret bounds instead of caret painter node + local caret rect? > It can be bigger than this patch but it is reasonable and align to future direction. I found yet another approach, but this is the solution. The root cause of this issue is we do nothing when caret not is removed, see |FrameSelection::respondToNodeModification()|. I think we should invalidate caret rectangle here and reset remembered caret not and local rect in this function. Invalidating here can invalidate more than needed, e.g. repeating remove/set of caret. Remember absolute caret bounds in this function then invalidate at in |FrameSeleciton::invalidateCaret()| is better. This change may be simple and safe, caret node being removed is available in this function.
Description was changed from ========== Invalidate the previous caret even if the #text node is detached. When the last character is removed from an input, the caret node is detached, which is the anonymous #text node. However, FrameSelection still refer to the previous caret node to invalidate the previous caret. Before deleting the last character "a" > INPUT 0x3be145763a50 (focused) #shadow-root 0x3be145763b50 DIV 0x3be145763c20 ID="inner-editor" (editable) * #text 0x3be145763f58 "a" After deleting the last character "a" > INPUT 0x3be145763a50 (focused) #shadow-root 0x3be145763b50 * DIV 0x3be145763c20 ID="inner-editor" (editable) In this case, Blink invalidates the caret of #text using parent "inner-editor" node. TEST=ManualTests/caret-paint-after-last-text-is-removed.html with --enable-partial-raster BUG=528922 ========== to ========== add the test to check the caret paint after removing the last text. When the last character is removed from an input, the caret node is detached, which is the anonymous #text node. However, FrameSelection still refer to the previous caret node to invalidate the previous caret. So the anonymous #text node must be invalidated correctly, before removing the node. Before deleting the last character "a" > INPUT 0x3be145763a50 (focused) #shadow-root 0x3be145763b50 DIV 0x3be145763c20 ID="inner-editor" (editable) * #text 0x3be145763f58 "a" After deleting the last character "a" > INPUT 0x3be145763a50 (focused) #shadow-root 0x3be145763b50 * DIV 0x3be145763c20 ID="inner-editor" (editable) TEST=ManualTests/caret-paint-after-last-text-is-removed.html with --enable-partial-raster BUG=528922 ==========
This bug is fixed recently. I failed to find what commit fix it. Now Blink invalidates whole region of the anonymous #text node, before removing the node. The region includes the previous caret rect. The fix is elegant, though I could not find the patch. This CL is just manual test. Is it good to add the test?
Description was changed from ========== add the test to check the caret paint after removing the last text. When the last character is removed from an input, the caret node is detached, which is the anonymous #text node. However, FrameSelection still refer to the previous caret node to invalidate the previous caret. So the anonymous #text node must be invalidated correctly, before removing the node. Before deleting the last character "a" > INPUT 0x3be145763a50 (focused) #shadow-root 0x3be145763b50 DIV 0x3be145763c20 ID="inner-editor" (editable) * #text 0x3be145763f58 "a" After deleting the last character "a" > INPUT 0x3be145763a50 (focused) #shadow-root 0x3be145763b50 * DIV 0x3be145763c20 ID="inner-editor" (editable) TEST=ManualTests/caret-paint-after-last-text-is-removed.html with --enable-partial-raster BUG=528922 ========== to ========== Invalidate the previous caret even if the parent text node is removed. When the last character is removed from an input, the caret node is detached, which is the anonymous #text node. However, FrameSelection still refer to the previous caret node to invalidate the previous caret. Before deleting the last character "a" > INPUT 0x3be145763a50 (focused) #shadow-root 0x3be145763b50 DIV 0x3be145763c20 ID="inner-editor" (editable) * #text 0x3be145763f58 "a" After deleting the last character "a" > INPUT 0x3be145763a50 (focused) #shadow-root 0x3be145763b50 * DIV 0x3be145763c20 ID="inner-editor" (editable) Currently, Blink doesn't invalidate the previous caret if the caret is detached from DOM. This CL invalidates the caret region when the caret is detached. TEST=ManualTests/caret-paint-after-last-text-is-removed.html with --enable-partial-raster BUG=528922 ==========
If I change the caret style to bold, it's still reproduced. New CL applies your suggestion, which invalidates the caret rect in FrameSelection::nodeWillBeRemoved() Yosi, could you review again?
Patchset #6 (id:120001) has been deleted
Patchset #6 (id:140001) has been deleted
Sorry for late response, I was OOO Monday and Tuesday. C++ changes good. Can you add layout-test or unit test(preferred) for your change? https://codereview.chromium.org/1654123002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/FrameSelection.cpp (right): https://codereview.chromium.org/1654123002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/FrameSelection.cpp:409: if (m_previousCaretNode && node.isSameNode(m_previousCaretNode.get())) { |node == m_previousCaretNode| Node::isSmaeNode() exists for backward compatibility. https://developer.mozilla.org/en/docs/Web/API/Node/isSameNode
Description was changed from ========== Invalidate the previous caret even if the parent text node is removed. When the last character is removed from an input, the caret node is detached, which is the anonymous #text node. However, FrameSelection still refer to the previous caret node to invalidate the previous caret. Before deleting the last character "a" > INPUT 0x3be145763a50 (focused) #shadow-root 0x3be145763b50 DIV 0x3be145763c20 ID="inner-editor" (editable) * #text 0x3be145763f58 "a" After deleting the last character "a" > INPUT 0x3be145763a50 (focused) #shadow-root 0x3be145763b50 * DIV 0x3be145763c20 ID="inner-editor" (editable) Currently, Blink doesn't invalidate the previous caret if the caret is detached from DOM. This CL invalidates the caret region when the caret is detached. TEST=ManualTests/caret-paint-after-last-text-is-removed.html with --enable-partial-raster BUG=528922 ========== to ========== Invalidate the previous caret even if the parent text node is removed. When the last character is removed from an input, the caret node is detached, which is the anonymous #text node. However, FrameSelection still refer to the previous caret node to invalidate the previous caret. Before deleting the last character "a" > INPUT 0x3be145763a50 (focused) #shadow-root 0x3be145763b50 DIV 0x3be145763c20 ID="inner-editor" (editable) * #text 0x3be145763f58 "a" After deleting the last character "a" > INPUT 0x3be145763a50 (focused) #shadow-root 0x3be145763b50 * DIV 0x3be145763c20 ID="inner-editor" (editable) Currently, Blink doesn't invalidate the previous caret if the caret is detached from DOM. This CL invalidates the caret region when the caret is detached. TEST=webkit_unit_tests FrameSelectionTest.InvalidatePreviousCaretAfterRemovingLastCharacter ManualTests/caret-paint-after-last-text-is-removed.html with --enable-partial-raster BUG=528922 ==========
On 2016/02/24 03:55:25, Yosi_UTC9 wrote: > C++ changes good. > Can you add layout-test or unit test(preferred) for your change? I added FrameSelectionTest.InvalidatePreviousCaretAfterRemovingLastCharacter in webkit_unit_tests Could you review again? https://codereview.chromium.org/1654123002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/FrameSelection.cpp (right): https://codereview.chromium.org/1654123002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/FrameSelection.cpp:409: if (m_previousCaretNode && node.isSameNode(m_previousCaretNode.get())) { On 2016/02/24 03:55:25, Yosi_UTC9 wrote: > |node == m_previousCaretNode| > > Node::isSmaeNode() exists for backward compatibility. > https://developer.mozilla.org/en/docs/Web/API/Node/isSameNode Done.
Patchset #8 (id:200001) has been deleted
lgtm https://codereview.chromium.org/1654123002/diff/220001/third_party/WebKit/Man... File third_party/WebKit/ManualTests/caret-paint-after-last-text-is-removed.html (right): https://codereview.chromium.org/1654123002/diff/220001/third_party/WebKit/Man... third_party/WebKit/ManualTests/caret-paint-after-last-text-is-removed.html:1: <!DOCTYPE html> Since we have an unit test, we don't need to have a manual test.
Thank you for patient review! https://codereview.chromium.org/1654123002/diff/220001/third_party/WebKit/Man... File third_party/WebKit/ManualTests/caret-paint-after-last-text-is-removed.html (right): https://codereview.chromium.org/1654123002/diff/220001/third_party/WebKit/Man... third_party/WebKit/ManualTests/caret-paint-after-last-text-is-removed.html:1: <!DOCTYPE html> On 2016/02/25 02:03:55, Yosi_UTC9 wrote: > Since we have an unit test, we don't need to have a manual test. Ok, removed
The CQ bit was checked by dongseong.hwang@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from yosin@chromium.org Link to the patchset: https://codereview.chromium.org/1654123002/#ps240001 (title: "patch for land")
I trust yosin's review (and this seems reasonable enough), but I have one nit, sorry. https://codereview.chromium.org/1654123002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/FrameSelection.h (right): https://codereview.chromium.org/1654123002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/FrameSelection.h:290: bool ShouldPaintCaretForTesting() const { return m_shouldPaintCaret; } nit: method names should begin with a lowercase letter in Blink. https://codereview.chromium.org/1654123002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/FrameSelectionTest.cpp (right): https://codereview.chromium.org/1654123002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/FrameSelectionTest.cpp:39: bool ShouldPaintCaretForTesting() const { return selection().ShouldPaintCaretForTesting(); } same here
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1654123002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1654123002/240001
The CQ bit was unchecked by dongseong.hwang@intel.com
https://codereview.chromium.org/1654123002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/FrameSelection.h (right): https://codereview.chromium.org/1654123002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/FrameSelection.h:290: bool ShouldPaintCaretForTesting() const { return m_shouldPaintCaret; } On 2016/02/25 18:29:22, jbroman wrote: > nit: method names should begin with a lowercase letter in Blink. Done. I didn't touch existing method, but good idea to fix here.
The CQ bit was checked by dongseong.hwang@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from yosin@chromium.org Link to the patchset: https://codereview.chromium.org/1654123002/#ps260001 (title: "patch for land v2")
The CQ bit was unchecked by dongseong.hwang@intel.com
The CQ bit was checked by dongseong.hwang@intel.com
The CQ bit was unchecked by dongseong.hwang@intel.com
The CQ bit was checked by dongseong.hwang@intel.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1654123002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1654123002/260001
Message was sent while issue was closed.
Description was changed from ========== Invalidate the previous caret even if the parent text node is removed. When the last character is removed from an input, the caret node is detached, which is the anonymous #text node. However, FrameSelection still refer to the previous caret node to invalidate the previous caret. Before deleting the last character "a" > INPUT 0x3be145763a50 (focused) #shadow-root 0x3be145763b50 DIV 0x3be145763c20 ID="inner-editor" (editable) * #text 0x3be145763f58 "a" After deleting the last character "a" > INPUT 0x3be145763a50 (focused) #shadow-root 0x3be145763b50 * DIV 0x3be145763c20 ID="inner-editor" (editable) Currently, Blink doesn't invalidate the previous caret if the caret is detached from DOM. This CL invalidates the caret region when the caret is detached. TEST=webkit_unit_tests FrameSelectionTest.InvalidatePreviousCaretAfterRemovingLastCharacter ManualTests/caret-paint-after-last-text-is-removed.html with --enable-partial-raster BUG=528922 ========== to ========== Invalidate the previous caret even if the parent text node is removed. When the last character is removed from an input, the caret node is detached, which is the anonymous #text node. However, FrameSelection still refer to the previous caret node to invalidate the previous caret. Before deleting the last character "a" > INPUT 0x3be145763a50 (focused) #shadow-root 0x3be145763b50 DIV 0x3be145763c20 ID="inner-editor" (editable) * #text 0x3be145763f58 "a" After deleting the last character "a" > INPUT 0x3be145763a50 (focused) #shadow-root 0x3be145763b50 * DIV 0x3be145763c20 ID="inner-editor" (editable) Currently, Blink doesn't invalidate the previous caret if the caret is detached from DOM. This CL invalidates the caret region when the caret is detached. TEST=webkit_unit_tests FrameSelectionTest.InvalidatePreviousCaretAfterRemovingLastCharacter ManualTests/caret-paint-after-last-text-is-removed.html with --enable-partial-raster BUG=528922 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== Invalidate the previous caret even if the parent text node is removed. When the last character is removed from an input, the caret node is detached, which is the anonymous #text node. However, FrameSelection still refer to the previous caret node to invalidate the previous caret. Before deleting the last character "a" > INPUT 0x3be145763a50 (focused) #shadow-root 0x3be145763b50 DIV 0x3be145763c20 ID="inner-editor" (editable) * #text 0x3be145763f58 "a" After deleting the last character "a" > INPUT 0x3be145763a50 (focused) #shadow-root 0x3be145763b50 * DIV 0x3be145763c20 ID="inner-editor" (editable) Currently, Blink doesn't invalidate the previous caret if the caret is detached from DOM. This CL invalidates the caret region when the caret is detached. TEST=webkit_unit_tests FrameSelectionTest.InvalidatePreviousCaretAfterRemovingLastCharacter ManualTests/caret-paint-after-last-text-is-removed.html with --enable-partial-raster BUG=528922 ========== to ========== Invalidate the previous caret even if the parent text node is removed. When the last character is removed from an input, the caret node is detached, which is the anonymous #text node. However, FrameSelection still refer to the previous caret node to invalidate the previous caret. Before deleting the last character "a" > INPUT 0x3be145763a50 (focused) #shadow-root 0x3be145763b50 DIV 0x3be145763c20 ID="inner-editor" (editable) * #text 0x3be145763f58 "a" After deleting the last character "a" > INPUT 0x3be145763a50 (focused) #shadow-root 0x3be145763b50 * DIV 0x3be145763c20 ID="inner-editor" (editable) Currently, Blink doesn't invalidate the previous caret if the caret is detached from DOM. This CL invalidates the caret region when the caret is detached. TEST=webkit_unit_tests FrameSelectionTest.InvalidatePreviousCaretAfterRemovingLastCharacter ManualTests/caret-paint-after-last-text-is-removed.html with --enable-partial-raster BUG=528922 Committed: https://crrev.com/26006147cabc6a22d94afc81bdd657328cdc7cb7 Cr-Commit-Position: refs/heads/master@{#377696} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/26006147cabc6a22d94afc81bdd657328cdc7cb7 Cr-Commit-Position: refs/heads/master@{#377696}
Message was sent while issue was closed.
I found it! Sorry, my memory is too small...
Message was sent while issue was closed.
haha, no problem. Good to see you in person :) - Dongseong On Thu, Jun 16, 2016 at 3:14 PM, yosin@chromium.org via codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com> wrote: > I found it! Sorry, my memory is too small... > > https://codereview.chromium.org/1654123002/ > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
haha, no problem. Good to see you in person :) - Dongseong On Thu, Jun 16, 2016 at 3:14 PM, yosin@chromium.org via codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com> wrote: > I found it! Sorry, my memory is too small... > > https://codereview.chromium.org/1654123002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
