third_party/WebKit/LayoutTests/editing/spelling/remove_spelling_markers.html - Issue 2650183002: Remove spelling markers when element is not editable

Side by Side Diff: third_party/WebKit/LayoutTests/editing/spelling/remove_spelling_markers.html

Issue 2650183002: Remove spelling markers when element is not editable (Closed)

Patch Set: Add Mac and Windows baselines Created 3 years, 10 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

« no previous file with comments | « no previous file | third_party/WebKit/LayoutTests/platform/linux/editing/spelling/remove_spelling_markers-expected.png » ('j') | third_party/WebKit/Source/core/html/HTMLElement.cpp » ('J')
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

OLD	NEW
(Empty)
	1 <!DOCTYPE html>
	yosin_UTC9 2017/01/25 02:31:41 Could you use spellcheckTest[1] to avoid pixel ref 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... Xiaocheng 2017/01/25 02:56:57 Yeah, please use spellcheck_test. You may follow ( Show quoted text 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. Manuel Rego 2017/01/25 08:03:10 I already tried to use spellcheck_test without suc 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? Xiaocheng 2017/01/25 08:28:36 Blink's spell checker works asynchronously, so spe 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: Show quoted text > 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. Show quoted text > 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. Show quoted text > > }, > '<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. Manuel Rego 2017/01/25 09:10:21 @Xiaocheng: Thank you very much, this indeed fixed Show quoted text 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.
	2 <div id="warning" style="color: red;">

	3 This test requires window.internals. If you use content_shell,

	4 "--expose-internals-for-testing" command flag enables it.

	5 </div>

	6 <script>

	7 if (window.internals)

	8 document.getElementById("warning").style.display = "none";

	9 </script>

	10

	11 <p>

	12 The test checks that removing "contenteditable" from an element,

	13 also removes the spelling markers.

	14 It passes if you don't see any spelling marker in the following text.

	15 </p>

	16 <div id="wrapper" contenteditable=true>

	17 <span id="grammar">some</span> <span id="spell">mispelllled</span> text.

	18 </div>

	19 <script>

	20 var rangeGrammar = document.createRange();

	21 var grammar = document.getElementById("grammar");

	22 rangeGrammar.setStart(grammar, 0);

	23 rangeGrammar.setEnd(grammar, 1);

	24 var rangeSpell = document.createRange();

	25 var spell = document.getElementById("spell");

	26 rangeSpell.setStart(spell, 0);

	27 rangeSpell.setEnd(spell, 1);

	28 if (window.internals) {

	29 internals.setMarker(document, rangeGrammar, 'grammar');

	30 internals.setMarker(document, rangeSpell, 'spelling');

	31 }

	32 var wrapper = document.getElementById("wrapper");

	33 wrapper.removeAttribute("contenteditable");

	34 </script>

OLD	NEW