Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2450733002:
Make spellcheck_test easier to use (Closed)
|
Created:
4 years, 1 month ago by Xiaocheng Modified:
4 years, 1 month ago Reviewers:
yosin_UTC9 CC:
blink-reviews, chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake spellcheck_test easier to use
This patch aims at simplifying the usage of |spellcheck_test| by changing
the way how expectation is passed. With this patch, we no longer count
character offsets as before, but pass an HTML fragment instead, in the same
spirit as |assert_selection|, with the only difference that we do not
indicate the ending selection, but instead, surround marked text with '_'
for spelling marker and '~' for grammar marker, respectively. Example:
spellcheck_test(
'<div contenteditable>|</div>',
'insertText zz.',
'<div contenteditable>_zz_.</div>',
'Mark misspellings after typing.');
This patch temporarily removes the functionality of checking marker
description from |spellcheck_test|, which will be added back in the future.
BUG=517298
Committed: https://crrev.com/1f5ab09f8114481f968bddfe614887b2b00a13c4
Cr-Commit-Position: refs/heads/master@{#427586}
Patch Set 1 #
Total comments: 18
Patch Set 2 : minor revision #
Total comments: 3
Patch Set 3 : Tue Oct 25 21:56:06 JST 2016 #
Total comments: 2
Patch Set 4 : Wed Oct 26 11:19:55 JST 2016 #
Dependent Patchsets: Messages
Total messages: 29 (17 generated)
xiaochengh@chromium.org changed reviewers: + yosin@chromium.org
This is still WIP, but I would like to have some comments first :)
Description was changed from
==========
Make spellcheck_test easier to use
BUG=517298
==========
to
==========
Make spellcheck_test easier to use
This patch aims at simplifying the usage of |spellcheck_test| by changing
the way how expectation is passed. With this patch, we no longer count
character offsets as before, but pass an HTML fragment instead, in the same
spirit as |assert_selection|, with the only difference that we do not
indicate the ending selection, but instead, replace the marked text with
special symbols. Example:
spellcheck_test(
'<div contenteditable>|</div>',
'insertText zz.',
'<div contenteditable>__.</div>',
'Mark misspellings after typing.');
This patch temporarily removes the functionality of checking marker
description from |spellcheck_test|, which will be added back in the future.
BUG=517298
==========
PTAL at the updated patch.
The CQ bit was checked by xiaochengh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2450733002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.html (right): https://codereview.chromium.org/2450733002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.html:13: '<div contenteditable>__.</div>', How about having expected selection? spellcheck_test( '<div contenteditable>|</div>', 'insertText zz.', '<div contenteditable>zz.|</div>', '<div contenteditable>__.|</div>'); It seems extending existing Serializer is considerable option. I propose to introduce callback or extends Serializer class to handle markers. This might help where markers are. It seems excluding tags is too sparse. spellcheck_test( '<div contenteditable>|</div>', 'insertText zz.', '<div contenteditable>zz.</div>', ' __'); https://codereview.chromium.org/2450733002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js (right): https://codereview.chromium.org/2450733002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js:82: if (!isElement(node)) |node.nodeName === 'INPUT'| is enough. https://codereview.chromium.org/2450733002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js:93: if (!isElement(node)) |node.nodeName === 'TEXTAREA'| is enough. https://codereview.chromium.org/2450733002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js:132: get activeMarkerTypes() { This should be a method instead of a getter, since an implementation isn't simple. https://codereview.chromium.org/2450733002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js:159: assert_equals(marker.startContainer, node, We don't need to verity marker. It should work as expected. https://codereview.chromium.org/2450733002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js:163: if (marker.startOffset == offset) { nit: s/==/===/ https://codereview.chromium.org/2450733002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js:280: const childNodes = Array.from(element.childNodes); Since Node#childNodes works with for-of, we don't need to use Array.from() https://codereview.chromium.org/2450733002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js:281: if (childNodes.length === 0) { if (element.childNodes.length === 0) https://codereview.chromium.org/2450733002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js:433: if (window.testRunner) We should abort if window.internals is unavailable by assert_undefined().
https://codereview.chromium.org/2450733002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.html (right): https://codereview.chromium.org/2450733002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.html:26: '<div contenteditable>~~~~~~~##~~~~~~~</div>', Yet another idea: spellcheck_test( '<div contenteditable>|</div>', 'insertText zz.', '<div contenteditable>~zz~.</div>', 'Mark misspellings after typing.'); spellcheck_test( '<div contenteditable>|</div>', 'insertText zz.', '<div contenteditable>_orange,~zz~,apple_</div>') Pair of "~" denotes spelling marker range. Pair of "_" denotes grammar marker range. So no need to have combination marker "#"
Description was changed from
==========
Make spellcheck_test easier to use
This patch aims at simplifying the usage of |spellcheck_test| by changing
the way how expectation is passed. With this patch, we no longer count
character offsets as before, but pass an HTML fragment instead, in the same
spirit as |assert_selection|, with the only difference that we do not
indicate the ending selection, but instead, replace the marked text with
special symbols. Example:
spellcheck_test(
'<div contenteditable>|</div>',
'insertText zz.',
'<div contenteditable>__.</div>',
'Mark misspellings after typing.');
This patch temporarily removes the functionality of checking marker
description from |spellcheck_test|, which will be added back in the future.
BUG=517298
==========
to
==========
Make spellcheck_test easier to use
This patch aims at simplifying the usage of |spellcheck_test| by changing
the way how expectation is passed. With this patch, we no longer count
character offsets as before, but pass an HTML fragment instead, in the same
spirit as |assert_selection|, with the only difference that we do not
indicate the ending selection, but instead, replace the marked text with
special symbols. Example:
spellcheck_test(
'<div contenteditable>|</div>',
'insertText zz.',
'<div contenteditable>~~.</div>',
'Mark misspellings after typing.');
This patch temporarily removes the functionality of checking marker
description from |spellcheck_test|, which will be added back in the future.
BUG=517298
==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by xiaochengh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Updated with: - changed the specification of |expectedText| - fixed a bug with serializing TEXTAREA - removed most call sites of |advancedTo|. Only those in |handleCharacterData| are left since markers are added only to CharacterData nodes. - addressing some other comments https://codereview.chromium.org/2450733002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.html (right): https://codereview.chromium.org/2450733002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.html:13: '<div contenteditable>__.</div>', On 2016/10/25 at 09:41:10, Yosi_UTC9 wrote: > How about having expected selection? > > spellcheck_test( > '<div contenteditable>|</div>', > 'insertText zz.', > '<div contenteditable>zz.|</div>', > '<div contenteditable>__.|</div>'); I don't think it makes too much sense to verify selection in a spellcheck test. In fact, I would like to even omit all tags and extract just the plain text. I don't have a good idea how to do so, though... Another reason not to do so is that we don't have a good way to serialize selection in INPUT or TEXTAREA. > > It seems extending existing Serializer is considerable option. > I propose to introduce callback or extends Serializer class to handle markers. This is worth doing, but maybe not for now as we need to somehow identify the common code paths of the two serializers. I prefer doing it in the future when we have time, as an independent refactoring task. > > > > This might help where markers are. > > It seems excluding tags is too sparse. > spellcheck_test( > '<div contenteditable>|</div>', > 'insertText zz.', > '<div contenteditable>zz.</div>', > ' __'); https://codereview.chromium.org/2450733002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js (right): https://codereview.chromium.org/2450733002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js:82: if (!isElement(node)) On 2016/10/25 at 09:41:11, Yosi_UTC9 wrote: > |node.nodeName === 'INPUT'| is enough. Done. https://codereview.chromium.org/2450733002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js:93: if (!isElement(node)) On 2016/10/25 at 09:41:11, Yosi_UTC9 wrote: > |node.nodeName === 'TEXTAREA'| is enough. Done. https://codereview.chromium.org/2450733002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js:132: get activeMarkerTypes() { On 2016/10/25 at 09:41:11, Yosi_UTC9 wrote: > This should be a method instead of a getter, since an implementation isn't simple. Done. https://codereview.chromium.org/2450733002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js:159: assert_equals(marker.startContainer, node, On 2016/10/25 at 09:41:10, Yosi_UTC9 wrote: > We don't need to verity marker. It should work as expected. I prefer keeping them in case anything goes wrong. I've changed the failure message to emphasize that it's an internal failure instead of due to a wrong test case. https://codereview.chromium.org/2450733002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js:163: if (marker.startOffset == offset) { On 2016/10/25 at 09:41:11, Yosi_UTC9 wrote: > nit: s/==/===/ Done. https://codereview.chromium.org/2450733002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js:280: const childNodes = Array.from(element.childNodes); On 2016/10/25 at 09:41:11, Yosi_UTC9 wrote: > Since Node#childNodes works with for-of, we don't need to use Array.from() Switched to a for loop. https://codereview.chromium.org/2450733002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js:281: if (childNodes.length === 0) { On 2016/10/25 at 09:41:11, Yosi_UTC9 wrote: > if (element.childNodes.length === 0) Switched to a for loop and removed this check. Perhaps you should do the same for assert_selection... https://codereview.chromium.org/2450733002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js:433: if (window.testRunner) On 2016/10/25 at 09:41:11, Yosi_UTC9 wrote: > We should abort if window.internals is unavailable by assert_undefined(). There's a reason not to abort here. |internals| is needed only for inspecting the markers. Unless we need to manipulate clipboard, |tester| can be executed without |internals|. By not aborting here, we are allowed to load the test file in a browser and see the result. https://codereview.chromium.org/2450733002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.html (right): https://codereview.chromium.org/2450733002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.html:26: '<div contenteditable>~~~~~~~##~~~~~~~</div>', On 2016/10/25 at 09:54:00, Yosi_UTC9 wrote: > Yet another idea: > > spellcheck_test( > '<div contenteditable>|</div>', > 'insertText zz.', > '<div contenteditable>~zz~.</div>', > 'Mark misspellings after typing.'); > > > spellcheck_test( > '<div contenteditable>|</div>', > 'insertText zz.', > '<div contenteditable>_orange,~zz~,apple_</div>') > > Pair of "~" denotes spelling marker range. > Pair of "_" denotes grammar marker range. > > So no need to have combination marker "#" Seems good. Done. https://codereview.chromium.org/2450733002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js (right): https://codereview.chromium.org/2450733002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js:256: this.serializeChildren(element); This is a bug that if a TEXTAREA has non-empty children, both its inner editor and children are serialized. This is fixed in the latest patch.
Description was changed from
==========
Make spellcheck_test easier to use
This patch aims at simplifying the usage of |spellcheck_test| by changing
the way how expectation is passed. With this patch, we no longer count
character offsets as before, but pass an HTML fragment instead, in the same
spirit as |assert_selection|, with the only difference that we do not
indicate the ending selection, but instead, replace the marked text with
special symbols. Example:
spellcheck_test(
'<div contenteditable>|</div>',
'insertText zz.',
'<div contenteditable>~~.</div>',
'Mark misspellings after typing.');
This patch temporarily removes the functionality of checking marker
description from |spellcheck_test|, which will be added back in the future.
BUG=517298
==========
to
==========
Make spellcheck_test easier to use
This patch aims at simplifying the usage of |spellcheck_test| by changing
the way how expectation is passed. With this patch, we no longer count
character offsets as before, but pass an HTML fragment instead, in the same
spirit as |assert_selection|, with the only difference that we do not
indicate the ending selection, but instead, surround marked text with '_'
for spelling marker and '~' for grammar marker, respectively. Example:
spellcheck_test(
'<div contenteditable>|</div>',
'insertText zz.',
'<div contenteditable>_zz_.</div>',
'Mark misspellings after typing.');
This patch temporarily removes the functionality of checking marker
description from |spellcheck_test|, which will be added back in the future.
BUG=517298
==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm w/ small nits https://codereview.chromium.org/2450733002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js (right): https://codereview.chromium.org/2450733002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js:66: if (!isElement(node)) nit: We don't need to have |isElement()|. Node#nodeName equals to Element#tagName. For other node, Node#nodeName returns '#text', '#comment' and so on. https://codereview.chromium.org/2450733002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/spelling/spellcheck_test.js:76: if (!isElement(node)) Ditto
The CQ bit was checked by xiaochengh@chromium.org
Committing with nits fixed.
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/2450733002/#ps60001 (title: "Wed Oct 26 11:19:55 JST 2016")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by xiaochengh@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from
==========
Make spellcheck_test easier to use
This patch aims at simplifying the usage of |spellcheck_test| by changing
the way how expectation is passed. With this patch, we no longer count
character offsets as before, but pass an HTML fragment instead, in the same
spirit as |assert_selection|, with the only difference that we do not
indicate the ending selection, but instead, surround marked text with '_'
for spelling marker and '~' for grammar marker, respectively. Example:
spellcheck_test(
'<div contenteditable>|</div>',
'insertText zz.',
'<div contenteditable>_zz_.</div>',
'Mark misspellings after typing.');
This patch temporarily removes the functionality of checking marker
description from |spellcheck_test|, which will be added back in the future.
BUG=517298
==========
to
==========
Make spellcheck_test easier to use
This patch aims at simplifying the usage of |spellcheck_test| by changing
the way how expectation is passed. With this patch, we no longer count
character offsets as before, but pass an HTML fragment instead, in the same
spirit as |assert_selection|, with the only difference that we do not
indicate the ending selection, but instead, surround marked text with '_'
for spelling marker and '~' for grammar marker, respectively. Example:
spellcheck_test(
'<div contenteditable>|</div>',
'insertText zz.',
'<div contenteditable>_zz_.</div>',
'Mark misspellings after typing.');
This patch temporarily removes the functionality of checking marker
description from |spellcheck_test|, which will be added back in the future.
BUG=517298
Committed: https://crrev.com/1f5ab09f8114481f968bddfe614887b2b00a13c4
Cr-Commit-Position: refs/heads/master@{#427586}
==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/1f5ab09f8114481f968bddfe614887b2b00a13c4 Cr-Commit-Position: refs/heads/master@{#427586} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
