Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1931513003:
Invalidate the previous caret location when editing text nodes. (Closed)
|
Created:
4 years, 7 months ago by chrishtr Modified:
4 years, 7 months ago Reviewers:
yosin_UTC9 CC:
blink-reviews, blink-reviews-dom_chromium.org, chromium-reviews, dglazkov+blink, eae+blinkwatch, rwlbuis, sof Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionInvalidate the previous caret location when editing text nodes.
Previously we would miss this invalidation.
BUG=603389
Committed: https://crrev.com/c8811f690116723cd62bb57ff6e3ff046bc3de42
Cr-Commit-Position: refs/heads/master@{#390982}
Patch Set 1 #Patch Set 2 : #
Total comments: 4
Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #
Total comments: 10
Patch Set 7 : #
Total comments: 14
Patch Set 8 : #Patch Set 9 : #Patch Set 10 : #Patch Set 11 : #
Messages
Total messages: 32 (15 generated)
Description was changed from ========== none none BUG= ========== to ========== Invalidate the previous caret location when editing text nodes. Previously we would miss this invalidation. BUG=603389 ==========
chrishtr@chromium.org changed reviewers: + yosin@chromium.org
Working on a paint invalidation test to go with this now.
Waiting for test script... https://codereview.chromium.org/1931513003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/CharacterData.cpp (right): https://codereview.chromium.org/1931513003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/CharacterData.cpp:45: document().dataWillChange(*this); At glance, calling |dataWillChange()| is enough in |setDataAnUpdate()|. Why do we need to call |dataWillChange()| for |m_data === nonNullData| case? Is this root cause of this bug? https://codereview.chromium.org/1931513003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/CharacterData.cpp:164: document().dataWillChange(*this); Should we call this for |source != UpdateFromParser| for paring with |didUpdateCharacterData()| call?
Added a test. https://codereview.chromium.org/1931513003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/CharacterData.cpp (right): https://codereview.chromium.org/1931513003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/CharacterData.cpp:45: document().dataWillChange(*this); On 2016/04/28 at 01:50:28, Yosi_Back_May_9 wrote: > At glance, calling |dataWillChange()| is enough in |setDataAnUpdate()|. setData() is an indepenent call site, no? > > Why do we need to call |dataWillChange()| for |m_data === nonNullData| case? Done. Also noticed that there is no early-out logic in setDataAndUpdate. I'll try that in a followup patch. > Is this root cause of this bug? The root cause is that if a text node is inserted by script (and not an editing command), and then the last character of the text is deleted, the text node's layout object will be eagerly deleted in updateTextLayoutObject. Thus FrameSelection doesn't have a hook to invalidate the old caret location before its old location becomes unknown. https://codereview.chromium.org/1931513003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/CharacterData.cpp:164: document().dataWillChange(*this); On 2016/04/28 at 01:50:28, Yosi_Back_May_9 wrote: > Should we call this for |source != UpdateFromParser| for paring with |didUpdateCharacterData()| call? Done.
https://codereview.chromium.org/1931513003/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/paint/invalidation/invalidate-caret-before-text-node-update.html (right): https://codereview.chromium.org/1931513003/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/paint/invalidation/invalidate-caret-before-text-node-update.html:1: <script src="../../editing/editing.js"></script> Could you avoid to use "editing/editing.js"? Because of this test doesn't need to check "selectionchange" event and it makes refactoring of FrameSeleciton harder. https://codereview.chromium.org/1931513003/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/paint/invalidation/invalidate-caret-before-text-node-update.html:9: function editingTest() { Let's use w3c test harness directly: test(function() { ... }); https://codereview.chromium.org/1931513003/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/paint/invalidation/invalidate-caret-before-text-node-update.html:12: moveSelectionForwardByCharacterCommand(); window.getSelection().collapse(root.firstChild, 1); https://codereview.chromium.org/1931513003/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/paint/invalidation/invalidate-caret-before-text-node-update.html:17: deleteCommand(); document.execCommand('delete'); https://codereview.chromium.org/1931513003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/FrameSelection.cpp (right): https://codereview.chromium.org/1931513003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/FrameSelection.cpp:739: m_caretBase->invalidateLocalCaretRect(m_previousCaretNode.get(), m_previousCaretRect); Should we reset other states of previous cares like |nodeWillBeRemoved()|? There are: m_caretBase->invalidateLocalCaretRect(m_previousCaretNode.get(), m_previousCaretRect); m_previousCaretNode = nullptr; m_previousCaretRect = LayoutRect(); m_previousCaretVisibility = CaretVisibility::Hidden; If so, it is better to have |resetPreviousCaretStates()| or another good name to share code.
https://codereview.chromium.org/1931513003/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/paint/invalidation/invalidate-caret-before-text-node-update.html (right): https://codereview.chromium.org/1931513003/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/paint/invalidation/invalidate-caret-before-text-node-update.html:1: <script src="../../editing/editing.js"></script> On 2016/04/29 at 15:03:12, Yosi_Back_May_9 wrote: > Could you avoid to use "editing/editing.js"? > Because of this test doesn't need to check "selectionchange" event and it makes refactoring of FrameSeleciton harder. Done. https://codereview.chromium.org/1931513003/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/paint/invalidation/invalidate-caret-before-text-node-update.html:9: function editingTest() { On 2016/04/29 at 15:03:12, Yosi_Back_May_9 wrote: > Let's use w3c test harness directly: > > test(function() { > ... > }); Done. https://codereview.chromium.org/1931513003/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/paint/invalidation/invalidate-caret-before-text-node-update.html:12: moveSelectionForwardByCharacterCommand(); On 2016/04/29 at 15:03:12, Yosi_Back_May_9 wrote: > window.getSelection().collapse(root.firstChild, 1); Done. https://codereview.chromium.org/1931513003/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/paint/invalidation/invalidate-caret-before-text-node-update.html:17: deleteCommand(); On 2016/04/29 at 15:03:12, Yosi_Back_May_9 wrote: > document.execCommand('delete'); Done. https://codereview.chromium.org/1931513003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/FrameSelection.cpp (right): https://codereview.chromium.org/1931513003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/FrameSelection.cpp:739: m_caretBase->invalidateLocalCaretRect(m_previousCaretNode.get(), m_previousCaretRect); On 2016/04/29 at 15:03:12, Yosi_Back_May_9 wrote: > Should we reset other states of previous cares like |nodeWillBeRemoved()|? > There are: > m_caretBase->invalidateLocalCaretRect(m_previousCaretNode.get(), m_previousCaretRect); > m_previousCaretNode = nullptr; > m_previousCaretRect = LayoutRect(); > m_previousCaretVisibility = CaretVisibility::Hidden; > If so, it is better to have |resetPreviousCaretStates()| or another good name to share code. I think those other changes will still be handled correctly by nodeWillBeRemoved. If a text node was mutated but not deleted, there is no need to zero out the caret node as far as I can see. Updating m_previousCaretRect needs to happen, but that can also be done in invalidateCaretRect() later in the lifecycle for the current frame.
The CQ bit was checked by chrishtr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1931513003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1931513003/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
lgtm Please update test file to follow w3c test script style. Here is sample: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... https://codereview.chromium.org/1931513003/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/paint/invalidation/invalidate-caret-before-text-node-update-expected.txt (right): https://codereview.chromium.org/1931513003/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/paint/invalidation/invalidate-caret-before-text-node-update-expected.txt:1: Summary <div id="log"></div> doesn't make output. The benefit of using w3c test harness is we don't need to have expected text. https://codereview.chromium.org/1931513003/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/paint/invalidation/invalidate-caret-before-text-node-update.html (right): https://codereview.chromium.org/1931513003/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/paint/invalidation/invalidate-caret-before-text-node-update.html:5: <pre id="output"> nit: Pleas use <div id="log"></div>, if so you don't need to have *-expected.txt https://codereview.chromium.org/1931513003/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/paint/invalidation/invalidate-caret-before-text-node-update.html:8: if (window.testRunner) You don't need to have this if-statement, w3c test harness automatically does so. https://codereview.chromium.org/1931513003/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/paint/invalidation/invalidate-caret-before-text-node-update.html:11: test(function editingTest() { s/test(function editingTest() {/test(function() {/ https://codereview.chromium.org/1931513003/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/paint/invalidation/invalidate-caret-before-text-node-update.html:23: assert_true(layers.children[0].paintInvalidations[2].reason == "invalidate paint rectangle"); Please use assert_equal(actual, expected, [description]) See https://github.com/w3c/testharness.js/blob/master/docs/api.md https://codereview.chromium.org/1931513003/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/paint/invalidation/invalidate-caret-before-text-node-update.html:25: assert_true(layers.children[0].paintInvalidations[2].rect[3] == 22); Please use assert_equal(actual, expected) https://codereview.chromium.org/1931513003/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/paint/invalidation/invalidate-caret-before-text-node-update.html:26: assert_true(layers.children[0].paintInvalidations[3].reason == "invalidate paint rectangle"); Please use assert_equal(actual, expected) https://codereview.chromium.org/1931513003/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/paint/invalidation/invalidate-caret-before-text-node-update.html:28: assert_true(layers.children[0].paintInvalidations[3].rect[3] == 21); Please use assert_equal(actual, expected) https://codereview.chromium.org/1931513003/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/paint/invalidation/invalidate-caret-before-text-node-update.html:31: nit: No need to have extra blank lines.
The CQ bit was checked by chrishtr@chromium.org
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/1931513003/#ps140001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1931513003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1931513003/140001
https://codereview.chromium.org/1931513003/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/paint/invalidation/invalidate-caret-before-text-node-update.html (right): https://codereview.chromium.org/1931513003/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/paint/invalidation/invalidate-caret-before-text-node-update.html:5: <pre id="output"> On 2016/04/30 at 03:16:17, Yosi_Back_May_9 wrote: > nit: Pleas use <div id="log"></div>, if so you don't need to have *-expected.txt Done. https://codereview.chromium.org/1931513003/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/paint/invalidation/invalidate-caret-before-text-node-update.html:8: if (window.testRunner) On 2016/04/30 at 03:16:16, Yosi_Back_May_9 wrote: > You don't need to have this if-statement, w3c test harness automatically does so. Done. https://codereview.chromium.org/1931513003/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/paint/invalidation/invalidate-caret-before-text-node-update.html:25: assert_true(layers.children[0].paintInvalidations[2].rect[3] == 22); On 2016/04/30 at 03:16:16, Yosi_Back_May_9 wrote: > Please use assert_equal(actual, expected) Done. https://codereview.chromium.org/1931513003/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/paint/invalidation/invalidate-caret-before-text-node-update.html:26: assert_true(layers.children[0].paintInvalidations[3].reason == "invalidate paint rectangle"); On 2016/04/30 at 03:16:17, Yosi_Back_May_9 wrote: > Please use assert_equal(actual, expected) Done. https://codereview.chromium.org/1931513003/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/paint/invalidation/invalidate-caret-before-text-node-update.html:28: assert_true(layers.children[0].paintInvalidations[3].rect[3] == 21); On 2016/04/30 at 03:16:17, Yosi_Back_May_9 wrote: > Please use assert_equal(actual, expected) Done.
The CQ bit was checked by chrishtr@chromium.org
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/1931513003/#ps160001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1931513003/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1931513003/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by chrishtr@chromium.org
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/1931513003/#ps180001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1931513003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1931513003/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by chrishtr@chromium.org
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/1931513003/#ps200001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1931513003/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1931513003/200001
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Invalidate the previous caret location when editing text nodes. Previously we would miss this invalidation. BUG=603389 ========== to ========== Invalidate the previous caret location when editing text nodes. Previously we would miss this invalidation. BUG=603389 Committed: https://crrev.com/c8811f690116723cd62bb57ff6e3ff046bc3de42 Cr-Commit-Position: refs/heads/master@{#390982} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/c8811f690116723cd62bb57ff6e3ff046bc3de42 Cr-Commit-Position: refs/heads/master@{#390982} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
