Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2650183002:
Remove spelling markers when element is not editable (Closed)
|
Created:
3 years, 11 months ago by Manuel Rego Modified:
3 years, 10 months ago CC:
blink-reviews, blink-reviews-html_chromium.org, chromium-reviews, dglazkov+blink, florian_rivoal.net, groby+blinkspell_chromium.org, jfernandez, timvolodine Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove spelling markers when element is not editable
If you mark a contenteditable element as not editable,
any spelling marker that appears there should disappear.
This behavior was discussed and agreed with the CSS WG at:
https://github.com/w3c/csswg-drafts/issues/623
BUG=683896
TEST=editing/spelling/spellcheck-remove-markers.html
Review-Url: https://codereview.chromium.org/2650183002
Cr-Commit-Position: refs/heads/master@{#447727}
Committed: https://chromium.googlesource.com/chromium/src/+/03bd67fe0ded2c7031eb1c3efb39150056737492
Patch Set 1 #Patch Set 2 : Add Mac and Windows baselines #
Total comments: 10
Patch Set 3 : Fix issue and add new spellcheck_test #
Total comments: 5
Patch Set 4 : New version #Patch Set 5 : Add missing comment #
Total comments: 6
Patch Set 6 : Apply suggested changes #
Messages
Total messages: 28 (8 generated)
Description was changed from ========== Remove spelling markers when element is not editable If you mark a contenteditable element as not editable, any spelling marker that appears there should disappear. This behavior was discussed and agreed with the CSS WG at: https://github.com/w3c/csswg-drafts/issues/623 BUG=683896 TEST=editing/spelling/remove_spelling_markers.html ========== to ========== Remove spelling markers when element is not editable If you mark a contenteditable element as not editable, any spelling marker that appears there should disappear. This behavior was discussed and agreed with the CSS WG at: https://github.com/w3c/csswg-drafts/issues/623 BUG=683896 TEST=editing/spelling/remove_spelling_markers.html ==========
rego@igalia.com changed reviewers: + yosin@chromium.org
rego@igalia.com changed reviewers: + tkent@chromium.org, xiaochengh@chromium.org
https://codereview.chromium.org/2650183002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/editing/spelling/remove_spelling_markers.html (right): https://codereview.chromium.org/2650183002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/spelling/remove_spelling_markers.html:1: <!DOCTYPE html> Could you use spellcheckTest[1] to avoid pixel ref test for faster execution and ease of maintain? https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/editing/s... https://codereview.chromium.org/2650183002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLElement.cpp (right): https://codereview.chromium.org/2650183002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLElement.cpp:441: document().frame()->spellChecker().removeSpellingAndGrammarMarkers(*this); Do we really need to remove markers synchronously? I think we can do it in idle time since spelling markers are set in asynchronously. More specifically, we can remove markers in idle time like idle time spell checker[1], e.g. IdleTimeSpellingMarkerCallback to handle both add and removal of markers. WDYT? [1] http://crrev.com/2578493002 Add a skeleton of idle time spell checker https://codereview.chromium.org/2650183002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLElement.cpp:466: // document().frame()->spellChecker().removeSpellingAndGrammarMarkers(*this); Could you remove this comment?
https://codereview.chromium.org/2650183002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/editing/spelling/remove_spelling_markers.html (right): https://codereview.chromium.org/2650183002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/spelling/remove_spelling_markers.html:1: <!DOCTYPE html> On 2017/01/25 at 02:31:41, Yosi_UTC9 wrote: > Could you use spellcheckTest[1] to avoid pixel ref test for faster execution and ease of maintain? > > > https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/editing/s... Yeah, please use spellcheck_test. You may follow (most of) the other tests in editing/spelling/ Please also note that Blink's test runner uses a mock spell checker with a limited dictionary of misspellings. In one word, just use "zz" for misspelling and "I have a issue" for bad grammar. https://codereview.chromium.org/2650183002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLElement.cpp (right): https://codereview.chromium.org/2650183002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLElement.cpp:441: document().frame()->spellChecker().removeSpellingAndGrammarMarkers(*this); On 2017/01/25 at 02:31:41, Yosi_UTC9 wrote: > Do we really need to remove markers synchronously? > I think we can do it in idle time since spelling markers are set in asynchronously. > More specifically, we can remove markers in idle time like idle time spell checker[1], > e.g. IdleTimeSpellingMarkerCallback to handle both add and removal of markers. > > WDYT? > > [1] http://crrev.com/2578493002 Add a skeleton of idle time spell checker I think synchronous marker removal is fine, as we are already doing that for <input> on blur. However, there are some tricky cases, for example: <div contenteditable="false"> <span contenteditable>misspelllling.</span> </div> When removing contenteditable from <div>, this patch incorrectly removes the marker in <span>.
Thanks for the reviews! https://codereview.chromium.org/2650183002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/editing/spelling/remove_spelling_markers.html (right): https://codereview.chromium.org/2650183002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/spelling/remove_spelling_markers.html:1: <!DOCTYPE html> I already tried to use spellcheck_test without success, that's why I created the reftest. I was doing: spellcheck_test( '<div contenteditable>|zz.</div>', document => { document.querySelector('div').focus(); document.querySelector('div').blur(); document.querySelector('div').removeAttribute('contenteditable'); }, '<div>zz.</div>', 'Remove markers.'); But I was always getting an error: FAIL Remove markers. assert_equals: expected "<div>zz.</div>" but got "<div>#zz#.</div>" However in the reftest, I could verify that the markers are removed. Am I missing something? https://codereview.chromium.org/2650183002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLElement.cpp (right): https://codereview.chromium.org/2650183002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLElement.cpp:441: document().frame()->spellChecker().removeSpellingAndGrammarMarkers(*this); On 2017/01/25 02:56:57, Xiaocheng wrote: > On 2017/01/25 at 02:31:41, Yosi_UTC9 wrote: > > Do we really need to remove markers synchronously? > > I think we can do it in idle time since spelling markers are set in > asynchronously. > > More specifically, we can remove markers in idle time like idle time spell > checker[1], > > e.g. IdleTimeSpellingMarkerCallback to handle both add and removal of markers. > > > > WDYT? > > > > [1] http://crrev.com/2578493002 Add a skeleton of idle time spell checker > > I think synchronous marker removal is fine, as we are already doing that for > <input> on blur. However, there are some tricky cases, for example: > > <div contenteditable="false"> > <span contenteditable>misspelllling.</span> > </div> > > When removing contenteditable from <div>, this patch incorrectly removes the > marker in <span>. Good catch! Probably I should use isEditable() to remove or not the markers. https://codereview.chromium.org/2650183002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLElement.cpp:466: // document().frame()->spellChecker().removeSpellingAndGrammarMarkers(*this); On 2017/01/25 02:31:41, Yosi_UTC9 wrote: > Could you remove this comment? Sure this is a leftover from a previous version of the patch, this method parseAttribute() shouldn't be modified at all. Sorry about that.
https://codereview.chromium.org/2650183002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/editing/spelling/remove_spelling_markers.html (right): https://codereview.chromium.org/2650183002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/spelling/remove_spelling_markers.html:1: <!DOCTYPE html> Blink's spell checker works asynchronously, so spell check test cases can sometimes be tricky to write... On 2017/01/25 at 08:03:10, Manuel Rego wrote: > I already tried to use spellcheck_test without success, > that's why I created the reftest. > > I was doing: > spellcheck_test( > '<div contenteditable>|zz.</div>', > document => { > document.querySelector('div').focus(); > document.querySelector('div').blur(); At here, spell checking is requested but nothing is really checked yet. > document.querySelector('div').removeAttribute('contenteditable'); At here, the attribute is removed before any marker appears. After the test body finishes, the real spell checking is done, and then a markers is added to "zz", causing the failure. > > }, > '<div>zz.</div>', > 'Remove markers.'); > > But I was always getting an error: > FAIL Remove markers. assert_equals: expected "<div>zz.</div>" but got "<div>#zz#.</div>" > > However in the reftest, I could verify that the markers are removed. > > Am I missing something? The correct test case should have two parts: 1. A spellcheck_test that ensures the appearance of the marker 2. A callback spellcheck_test to verify marker removal So it should be something like: spellcheck_test( '<div contenteditable>zz.|</div>', '', // nothing is needed here because '|' already sets focus to the <div> '<div contenteditable>#zz#.|</div>', { title: 'Setup for the initial marker.', callback: sample => spellcheck_test( sample, document => document.querySelector('div').removeAttribute('contenteditable'), '<div contenteditable>zz.|</div>', 'Removing editability removes marker.') }); Btw. It's a known bug that markers can be added to non-editable nodes when spell check finishes. Anyway, a correct test should be something like above.
https://codereview.chromium.org/2650183002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/editing/spelling/remove_spelling_markers.html (right): https://codereview.chromium.org/2650183002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/spelling/remove_spelling_markers.html:1: <!DOCTYPE html> On 2017/01/25 08:28:36, Xiaocheng wrote: > spellcheck_test( > '<div contenteditable>zz.|</div>', > '', // nothing is needed here because '|' already sets focus to the <div> > '<div contenteditable>#zz#.|</div>', > { > title: 'Setup for the initial marker.', > callback: sample => spellcheck_test( > sample, > document => > document.querySelector('div').removeAttribute('contenteditable'), > '<div contenteditable>zz.|</div>', > 'Removing editability removes marker.') > }); @Xiaocheng: Thank you very much, this indeed fixed the issue. I'll add tests like that, and also checking the special case you mentioned earlier and upload a new version of the patch.
Description was changed from ========== Remove spelling markers when element is not editable If you mark a contenteditable element as not editable, any spelling marker that appears there should disappear. This behavior was discussed and agreed with the CSS WG at: https://github.com/w3c/csswg-drafts/issues/623 BUG=683896 TEST=editing/spelling/remove_spelling_markers.html ========== to ========== Remove spelling markers when element is not editable If you mark a contenteditable element as not editable, any spelling marker that appears there should disappear. This behavior was discussed and agreed with the CSS WG at: https://github.com/w3c/csswg-drafts/issues/623 BUG=683896 TEST=editing/spelling/spellcheck-remove-markers.html ==========
Uploaded new version, PTAL. Thanks!
lgtm https://codereview.chromium.org/2650183002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp (right): https://codereview.chromium.org/2650183002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:808: void SpellChecker::removeSpellingAndGrammarMarkers(HTMLElement& element) { Note: We want to make this function to have |const HTMLElement&|, but |DocumentMarkerController::removeMarkers()| takes |Node*| incorrectly.
https://codereview.chromium.org/2650183002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp (right): https://codereview.chromium.org/2650183002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:811: for (Node& node : NodeTraversal::inclusiveDescendantsOf(element)) This function still removes all markers from the entire subtree, which means it still doesn't pass the test case I previously gave. We should remove markers only from non-editable elements. https://codereview.chromium.org/2650183002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLElement.cpp (right): https://codereview.chromium.org/2650183002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLElement.cpp:440: if (!isEditable(*this)) Please do not use |isEditable| inside Blink. Instead, this part should be // TODO(editing-dev): The use of updateStyleAndLayoutIgnorePendingStylesheets // needs to be audited. See http://crbug.com/590369 for more details. document().updateStyleAndLayoutTreeForNode(this); if (!hasEditableStyle(*this)) document().frame()->spellChecker().removeSpellingAndGrammarMarkers(*this); For some historical reasons, Blink is using a style-based editability system, which is not quite spec-compatible. We are working on making it compatible, though... Blink also implements a spec-compatible |Element::isContentEditableForBinding()|, which is, however, a fake function only for Web API, and is not supposed to be used inside Blink. |isEditable(Node&)| is a helper function for |isContentEditableForBinding|, and should not be used internally, either.
Does this CL work well in a case where an element becomes non-editable by -webkit-user-modify CSS property change?
https://codereview.chromium.org/2650183002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp (right): https://codereview.chromium.org/2650183002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:811: for (Node& node : NodeTraversal::inclusiveDescendantsOf(element)) On 2017/01/26 at 05:12:33, Xiaocheng wrote: > This function still removes all markers from the entire subtree, which means it still doesn't pass the test case I previously gave. > > We should remove markers only from non-editable elements. Let me cancel my "lgtm". We need to support following scenario: <div id="outer" contenteditable>foo<div id="innner" contenteditable>bar</div>baz</div> When outer.setAttribute('contenteditable', 'false'), we should keep document marker for "inner".
I've uploaded a new version with the missing test case. Regarding "-webkit-user-modify" this patch doesn't detect the change, so it doesn't remove the markers. Should that be part of the same patch or a different bug? https://codereview.chromium.org/2650183002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp (right): https://codereview.chromium.org/2650183002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:811: for (Node& node : NodeTraversal::inclusiveDescendantsOf(element)) On 2017/01/26 05:12:33, Xiaocheng wrote: > This function still removes all markers from the entire subtree, which means it > still doesn't pass the test case I previously gave. True, sorry about that. I was checking only one thing, removing "contenteditable" from "child", but not the other option, removing it from "parent". E.g. <div contenteditable id="parent"> zz. <div contenteditable id="child">zz.</div> zz. </div>
On 2017/01/27 at 12:56:20, rego wrote: > Regarding "-webkit-user-modify" this patch doesn't detect > the change, so it doesn't remove the markers. > Should that be part of the same patch or a different bug? It's ok not to handle -webkit-user-modify in this CL. We're planning deprecation of -webkit-user-modify now.
On 2017/01/30 01:06:38, tkent wrote: > On 2017/01/27 at 12:56:20, rego wrote: > > Regarding "-webkit-user-modify" this patch doesn't detect > > the change, so it doesn't remove the markers. > > Should that be part of the same patch or a different bug? > > It's ok not to handle -webkit-user-modify in this CL. > We're planning deprecation of -webkit-user-modify now. Ok. @Yosi & @Xiaocheng what do you think about the last version of the patch?
Sorry for the long delay. Just back from a sick leave... I'm basically OK with Patch 5, except for a few minor issues. https://codereview.chromium.org/2650183002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/editing/spelling/spellcheck-remove-markers.html (right): https://codereview.chromium.org/2650183002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/spelling/spellcheck-remove-markers.html:13: title: 'Setup for the initial markers.', nit: Could you number the setup test cases? https://codereview.chromium.org/2650183002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp (right): https://codereview.chromium.org/2650183002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:816: frame().document()->updateStyleAndLayoutTreeForNode(&node); This style and layout update should be placed out of the for loop. https://codereview.chromium.org/2650183002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.h (right): https://codereview.chromium.org/2650183002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.h:76: enum ElementsType { All, OnlyNonEditable }; Please use |enum class|, and name the members in kCamelCase style. Forgot the link to the coding style reference, which yosin@ sent when reviewing my patches several times...
Uploaded new version applying suggested changes. On 2017/02/01 05:20:28, Xiaocheng wrote: > Sorry for the long delay. Just back from a sick leave... No worries about that and take care. Also it's BlinkOn week so it's expected reviews are slower than usual. https://codereview.chromium.org/2650183002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/editing/spelling/spellcheck-remove-markers.html (right): https://codereview.chromium.org/2650183002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/spelling/spellcheck-remove-markers.html:13: title: 'Setup for the initial markers.', On 2017/02/01 05:20:28, Xiaocheng wrote: > nit: Could you number the setup test cases? Sure. https://codereview.chromium.org/2650183002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp (right): https://codereview.chromium.org/2650183002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:816: frame().document()->updateStyleAndLayoutTreeForNode(&node); On 2017/02/01 05:20:28, Xiaocheng wrote: > This style and layout update should be placed out of the for loop. Done. https://codereview.chromium.org/2650183002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.h (right): https://codereview.chromium.org/2650183002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.h:76: enum ElementsType { All, OnlyNonEditable }; On 2017/02/01 05:20:28, Xiaocheng wrote: > Please use |enum class|, and name the members in kCamelCase style. > > Forgot the link to the coding style reference, which yosin@ sent when reviewing > my patches several times... Acknowledged.
lgtm
The CQ bit was checked by rego@igalia.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/2650183002/#ps100001 (title: "Apply suggested changes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for the thoroughly review. :-)
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1486021778154550,
"parent_rev": "7d05879bfe53b74ead2428406224903b398e261c", "commit_rev":
"03bd67fe0ded2c7031eb1c3efb39150056737492"}
Message was sent while issue was closed.
Description was changed from ========== Remove spelling markers when element is not editable If you mark a contenteditable element as not editable, any spelling marker that appears there should disappear. This behavior was discussed and agreed with the CSS WG at: https://github.com/w3c/csswg-drafts/issues/623 BUG=683896 TEST=editing/spelling/spellcheck-remove-markers.html ========== to ========== Remove spelling markers when element is not editable If you mark a contenteditable element as not editable, any spelling marker that appears there should disappear. This behavior was discussed and agreed with the CSS WG at: https://github.com/w3c/csswg-drafts/issues/623 BUG=683896 TEST=editing/spelling/spellcheck-remove-markers.html Review-Url: https://codereview.chromium.org/2650183002 Cr-Commit-Position: refs/heads/master@{#447727} Committed: https://chromium.googlesource.com/chromium/src/+/03bd67fe0ded2c7031eb1c3efb39... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/03bd67fe0ded2c7031eb1c3efb39... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
