Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1636883003:
Perform Spellcheck Requesting before Dispatching Events (Closed)
|
Created:
4 years, 11 months ago by Xiaocheng Modified:
4 years, 10 months ago Reviewers:
yosin_UTC9 CC:
blink-reviews, chromium-reviews, kochi Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPerform Spellcheck Requesting before Dispatching Events
|ReplaceSelectionCommand| stores the inserted range after |doApply()|.
This range may be invalidated if |Editor::appliedEditing()| modifies the
DOM, causing spellcheck run on an invalid range.
This CL moves the spellcheck request into |Editor::appliedEditing()|
before dispatching any event, ensuring spellcheck run on a valid range.
BUG=580950
TEST=LayoutTests/editing/pasteboard/paste-webkit-editable-content-changed-crash.html
Committed: https://crrev.com/275214958096dafab0cd78968f66d5108d83ab1c
Cr-Commit-Position: refs/heads/master@{#371776}
Patch Set 1 #Patch Set 2 : Move spell check requesting to |Editor::appliedEditing()| #Patch Set 3 : Must dispatch non-scoped events #
Total comments: 2
Patch Set 4 : #Patch Set 5 : Add test case #
Total comments: 14
Patch Set 6 : #
Total comments: 2
Patch Set 7 : #
Messages
Total messages: 26 (10 generated)
xiaochengh@chromium.org changed reviewers: + yosin@chromium.org
PTAL. I'm still figuring out how to minimize the repro case. A test case will be added later.
Rather than nullify inserted range, we would like to fix root cause, e.g. postpone |webkitEditableContentChanged| event dispatch.
On 2016/01/26 at 03:46:24, yosin wrote: > Rather than nullify inserted range, we would like to fix root cause, e.g. postpone |webkitEditableContentChanged| event dispatch. Just did some experiments. Regardless of using |dispatchEvent()| or |dispatchScopedEvent()|, the DOM tree doesn't change within |Editor::appliedEditing()|, but got changed just after |Editor::appliedEditing()| returned. So I think the nullification in |ReplaceSelectedCommand::insertedRange| is still necessary. Whether to dispatch event or scoped event seems to be another topic.
On 2016/01/26 at 05:27:24, xiaochengh wrote: > On 2016/01/26 at 03:46:24, yosin wrote: > > Rather than nullify inserted range, we would like to fix root cause, e.g. postpone |webkitEditableContentChanged| event dispatch. > > Just did some experiments. Regardless of using |dispatchEvent()| or |dispatchScopedEvent()|, the DOM tree doesn't change within |Editor::appliedEditing()|, but got changed just after |Editor::appliedEditing()| returned. > > So I think the nullification in |ReplaceSelectedCommand::insertedRange| is still necessary. Whether to dispatch event or scoped event seems to be another topic. |changeSelectionAfterCommand()| dispatches "selectionchange" event, so DOM tree can be mutated. That said, we should request spell checking before. Another option is placed before dispatchEditableContentChangedEvents(), but once we postpone "editableContentChange" event, we don't need to count it as unsafe function.
PTAL. I also tried changing |dispatchEvent| to |dispatchScopedEvent|, which caused two layout tests to fail (see patch 2 for details.) So finally I just moved the spell checking code to before dispatching those content changed events.
Description was changed from ========== Add Validity Check in ReplaceSelectionCommand::insertedRange() |ReplaceSelectionCommand| stores the inserted range after |doApply()|. This range may be invalidated if |Editor::appliedEditing()| modifies the DOM. This CL checks if the stored range is still valid when the command finishes. BUG=580950 ========== to ========== Perform Spellcheck Requesting before Dispatching Events |ReplaceSelectionCommand| stores the inserted range after |doApply()|. This range may be invalidated if |Editor::appliedEditing()| modifies the DOM, causing spellcheck run on an invalid range. This CL moves the spellcheck request into |Editor::appliedEditing()| before dispatching any event, ensuring spellcheck run on a valid range. BUG=580950 ==========
lgtm w/ style suggestion. https://codereview.chromium.org/1636883003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/Editor.cpp (right): https://codereview.chromium.org/1636883003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/Editor.cpp:657: if (cmd->editingAction() == EditActionPaste) { Can we have a function |Editor::requestSpellCheckingBasedOnCommand()| or another name? It seems this conditions are complicated, I would like to have early return style to make a condition clear and hide details in |appliedEdiging()|. BTW, I'm wonder why we don't request spell checking in |unappliedEditigin()| and |reapplyiedEditing()|. For consistency, I think we should request spellcing in them. Anyway, once we have aync-spellcheck-request, we don't need to worry about this. (^_^)
The CQ bit was checked by xiaochengh@chromium.org
Revised accordingly. https://codereview.chromium.org/1636883003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/Editor.cpp (right): https://codereview.chromium.org/1636883003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/Editor.cpp:657: if (cmd->editingAction() == EditActionPaste) { On 2016/01/27 at 02:06:43, Yosi_UTC9 wrote: > Can we have a function |Editor::requestSpellCheckingBasedOnCommand()| or another name? > It seems this conditions are complicated, I would like to have early return style to make a condition clear and hide details in |appliedEdiging()|. Good point, I'll move them to a separate function. > > BTW, I'm wonder why we don't request spell checking in |unappliedEditigin()| and |reapplyiedEditing()|. For consistency, I think we should request spellcing in them. > > Anyway, once we have aync-spellcheck-request, we don't need to worry about this. (^_^) Yeah.
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/1636883003/#ps60001 (title: " ")
The CQ bit was unchecked by xiaochengh@chromium.org
Description was changed from ========== Perform Spellcheck Requesting before Dispatching Events |ReplaceSelectionCommand| stores the inserted range after |doApply()|. This range may be invalidated if |Editor::appliedEditing()| modifies the DOM, causing spellcheck run on an invalid range. This CL moves the spellcheck request into |Editor::appliedEditing()| before dispatching any event, ensuring spellcheck run on a valid range. BUG=580950 ========== to ========== Perform Spellcheck Requesting before Dispatching Events |ReplaceSelectionCommand| stores the inserted range after |doApply()|. This range may be invalidated if |Editor::appliedEditing()| modifies the DOM, causing spellcheck run on an invalid range. This CL moves the spellcheck request into |Editor::appliedEditing()| before dispatching any event, ensuring spellcheck run on a valid range. BUG=580950 TEST=LayoutTests/editing/pasteboard/paste-webkit-content-editable-changed-crash.html ==========
Description was changed from ========== Perform Spellcheck Requesting before Dispatching Events |ReplaceSelectionCommand| stores the inserted range after |doApply()|. This range may be invalidated if |Editor::appliedEditing()| modifies the DOM, causing spellcheck run on an invalid range. This CL moves the spellcheck request into |Editor::appliedEditing()| before dispatching any event, ensuring spellcheck run on a valid range. BUG=580950 TEST=LayoutTests/editing/pasteboard/paste-webkit-content-editable-changed-crash.html ========== to ========== Perform Spellcheck Requesting before Dispatching Events |ReplaceSelectionCommand| stores the inserted range after |doApply()|. This range may be invalidated if |Editor::appliedEditing()| modifies the DOM, causing spellcheck run on an invalid range. This CL moves the spellcheck request into |Editor::appliedEditing()| before dispatching any event, ensuring spellcheck run on a valid range. BUG=580950 TEST=LayoutTests/editing/pasteboard/paste-webkit-editable-content-changed-crash.html ==========
Added a layout test. PTAL.
https://codereview.chromium.org/1636883003/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/editing/pasteboard/paste-webkit-editable-content-changed-crash.html (right): https://codereview.chromium.org/1636883003/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/pasteboard/paste-webkit-editable-content-changed-crash.html:2: nit: We don't need to have an extra blank line. https://codereview.chromium.org/1636883003/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/pasteboard/paste-webkit-editable-content-changed-crash.html:5: nit: We don't need to have an extra blank line. https://codereview.chromium.org/1636883003/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/pasteboard/paste-webkit-editable-content-changed-crash.html:10: nit: We don't need to have an extra blank line. https://codereview.chromium.org/1636883003/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/pasteboard/paste-webkit-editable-content-changed-crash.html:13: nit: We don't need to have an extra blank line. https://codereview.chromium.org/1636883003/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/pasteboard/paste-webkit-editable-content-changed-crash.html:20: dest.addEventListener('webkitEditableContentChanged', function() { dest.innerHTML='' }); nit: Should be multiple lines. nit: Need spaces around |=| How about below? ES6-ish dest.addEventListener('webkitEditableContentChanged', () => dest.innerHTML = ''); https://codereview.chromium.org/1636883003/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/pasteboard/paste-webkit-editable-content-changed-crash.html:31: src.style.display = "none"; Add <div id="log"></div> instead of hide output. https://codereview.chromium.org/1636883003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/Editor.h (right): https://codereview.chromium.org/1636883003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/Editor.h:285: nit: we don't need to have an extra blank line
Revised style according to the comments. Also fixed a bug newly introduced in Patch 5 due to my carelessness... PTAL. https://codereview.chromium.org/1636883003/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/editing/pasteboard/paste-webkit-editable-content-changed-crash.html (right): https://codereview.chromium.org/1636883003/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/pasteboard/paste-webkit-editable-content-changed-crash.html:2: On 2016/01/27 at 06:02:38, Yosi_UTC9 wrote: > nit: We don't need to have an extra blank line. Done. https://codereview.chromium.org/1636883003/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/pasteboard/paste-webkit-editable-content-changed-crash.html:5: On 2016/01/27 at 06:02:38, Yosi_UTC9 wrote: > nit: We don't need to have an extra blank line. Done. https://codereview.chromium.org/1636883003/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/pasteboard/paste-webkit-editable-content-changed-crash.html:10: On 2016/01/27 at 06:02:38, Yosi_UTC9 wrote: > nit: We don't need to have an extra blank line. Done. https://codereview.chromium.org/1636883003/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/pasteboard/paste-webkit-editable-content-changed-crash.html:13: On 2016/01/27 at 06:02:38, Yosi_UTC9 wrote: > nit: We don't need to have an extra blank line. Done. https://codereview.chromium.org/1636883003/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/pasteboard/paste-webkit-editable-content-changed-crash.html:20: dest.addEventListener('webkitEditableContentChanged', function() { dest.innerHTML='' }); On 2016/01/27 at 06:02:38, Yosi_UTC9 wrote: > nit: Should be multiple lines. > nit: Need spaces around |=| > > How about below? ES6-ish > dest.addEventListener('webkitEditableContentChanged', () => dest.innerHTML = ''); Done. https://codereview.chromium.org/1636883003/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/pasteboard/paste-webkit-editable-content-changed-crash.html:31: src.style.display = "none"; On 2016/01/27 at 06:02:38, Yosi_UTC9 wrote: > Add <div id="log"></div> instead of hide output. Ah I didn't know this. Done. https://codereview.chromium.org/1636883003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/Editor.h (right): https://codereview.chromium.org/1636883003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/Editor.h:285: On 2016/01/27 at 06:02:38, Yosi_UTC9 wrote: > nit: we don't need to have an extra blank line Done.
still lgtm w/ nit https://codereview.chromium.org/1636883003/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/editing/pasteboard/paste-webkit-editable-content-changed-crash.html (right): https://codereview.chromium.org/1636883003/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/editing/pasteboard/paste-webkit-editable-content-changed-crash.html:16: dest.addEventListener('webkitEditableContentChanged', () => dest.innerHTML = ''); nit: It is better to insert non-empty string and not in HTML, e.g. "foo".
The CQ bit was checked by xiaochengh@chromium.org
Revised accordingly. https://codereview.chromium.org/1636883003/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/editing/pasteboard/paste-webkit-editable-content-changed-crash.html (right): https://codereview.chromium.org/1636883003/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/editing/pasteboard/paste-webkit-editable-content-changed-crash.html:16: dest.addEventListener('webkitEditableContentChanged', () => dest.innerHTML = ''); On 2016/01/27 at 08:40:29, Yosi_UTC9 wrote: > nit: It is better to insert non-empty string and not in HTML, e.g. "foo". Done.
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/1636883003/#ps120001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1636883003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1636883003/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Perform Spellcheck Requesting before Dispatching Events |ReplaceSelectionCommand| stores the inserted range after |doApply()|. This range may be invalidated if |Editor::appliedEditing()| modifies the DOM, causing spellcheck run on an invalid range. This CL moves the spellcheck request into |Editor::appliedEditing()| before dispatching any event, ensuring spellcheck run on a valid range. BUG=580950 TEST=LayoutTests/editing/pasteboard/paste-webkit-editable-content-changed-crash.html ========== to ========== Perform Spellcheck Requesting before Dispatching Events |ReplaceSelectionCommand| stores the inserted range after |doApply()|. This range may be invalidated if |Editor::appliedEditing()| modifies the DOM, causing spellcheck run on an invalid range. This CL moves the spellcheck request into |Editor::appliedEditing()| before dispatching any event, ensuring spellcheck run on a valid range. BUG=580950 TEST=LayoutTests/editing/pasteboard/paste-webkit-editable-content-changed-crash.html Committed: https://crrev.com/275214958096dafab0cd78968f66d5108d83ab1c Cr-Commit-Position: refs/heads/master@{#371776} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/275214958096dafab0cd78968f66d5108d83ab1c Cr-Commit-Position: refs/heads/master@{#371776} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
