Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2932123002:
Add test case for context click at misspelled word boundary (Closed)
DescriptionAdd test case for context click at misspelled word boundary
On most platforms, right-clicking a misspelled word only selects it and shows
suggestions in the context menu if you right-clik in the interior of the word.
In https://codereview.chromium.org/2925363002, I modified some of the spellcheck
code (specifically, the method that replaces the misspelled word) to support the
case where we have a caret selection immediately before or after a word, since
tapping at a word boundary should open the spellcheck menu on Android (this
feature is still in progress, but this is the behavior for a native EditText
widget, so we're trying to match it.)
This CL adds a test case to verify we don't accidentally change the behavior for
other platforms.
BUG=715365
Review-Url: https://codereview.chromium.org/2932123002
Cr-Commit-Position: refs/heads/master@{#478805}
Committed: https://chromium.googlesource.com/chromium/src/+/e7fe96812663e16778c2517bd3587eded541088e
Patch Set 1 #Patch Set 2 : Attempt to fix test on Mac #Patch Set 3 : Actually fix Mac #
Total comments: 6
Patch Set 4 : Fix Mac #Messages
Total messages: 29 (16 generated)
The CQ bit was checked by rlanday@chromium.org to run a CQ dry run
rlanday@chromium.org changed reviewers: + xiaochengh@chromium.org, yosin@chromium.org
Dry run: 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
Dry run: 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 rlanday@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...
The CQ bit was checked by rlanday@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/2932123002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/editing/spelling/context_click_select_misspelling.html (right): https://codereview.chromium.org/2932123002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/spelling/context_click_select_misspelling.html:103: container, 0, '', Note: we might want to modify this test so the misspelled word isn't at the start of the range; right-clicking immediately before a misspelled word selects it on Mac, but when the test runs, nothing is getting selected. I this is because clicking right on the boundary of some editable text is treated differently than clicking in the interior of the text.
https://codereview.chromium.org/2932123002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/editing/spelling/context_click_select_misspelling.html (right): https://codereview.chromium.org/2932123002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/spelling/context_click_select_misspelling.html:97: title: 'Context click at boundary of misspelled word.', nit: The title should be "Setup of context clicking at boundary of a misspelled word". https://codereview.chromium.org/2932123002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/spelling/context_click_select_misspelling.html:103: container, 0, '', On 2017/06/10 at 00:40:05, rlanday wrote: > Note: we might want to modify this test so the misspelled word isn't at the start of the range; right-clicking immediately before a misspelled word selects it on Mac, but when the test runs, nothing is getting selected. I this is because clicking right on the boundary of some editable text is treated differently than clicking in the interior of the text. If the behavior on Mac is different when the misspelled word is not at the beginning, could you add another test case for it, so that the current behavior is covered as much as possible?
https://codereview.chromium.org/2932123002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/editing/spelling/context_click_select_misspelling.html (right): https://codereview.chromium.org/2932123002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/spelling/context_click_select_misspelling.html:103: container, 0, '', On 2017/06/10 at 01:02:51, Xiaocheng wrote: > On 2017/06/10 at 00:40:05, rlanday wrote: > > Note: we might want to modify this test so the misspelled word isn't at the start of the range; right-clicking immediately before a misspelled word selects it on Mac, but when the test runs, nothing is getting selected. I this is because clicking right on the boundary of some editable text is treated differently than clicking in the interior of the text. > > If the behavior on Mac is different when the misspelled word is not at the beginning, could you add another test case for it, so that the current behavior is covered as much as possible? Wait... Let's investigate first why the behavior in layout test is different from real Chrome. We should eliminate such cases.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2932123002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/editing/spelling/context_click_select_misspelling.html (right): https://codereview.chromium.org/2932123002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/spelling/context_click_select_misspelling.html:42: const x = document.offsetLeft + rect.left; If we move the x coord here over not 1 but 2 pixels over to the right, that's far enough into the word to make it be selected on Mac. I think the issue is that the way I currently have the test, it's clicking on the very edge of the <div />, which Chrome doesn't consider to be actually inside the editable text.
https://codereview.chromium.org/2932123002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/editing/spelling/context_click_select_misspelling.html (right): https://codereview.chromium.org/2932123002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/spelling/context_click_select_misspelling.html:42: const x = document.offsetLeft + rect.left; On 2017/06/10 at 06:20:39, rlanday wrote: > If we move the x coord here over not 1 but 2 pixels over to the right, that's far enough into the word to make it be selected on Mac. I think the issue is that the way I currently have the test, it's clicking on the very edge of the <div />, which Chrome doesn't consider to be actually inside the editable text. So the root cause seems to incorrect coordinate calculation... Let's assert that the selection exists after the context click and see what happens. If the click position is inside the editable text, it should first set selection to '|wellcome home.' and then select the word. If the click position is outside the editable text, it should unselect everything instead.
On 2017/06/12 at 19:06:26, xiaochengh wrote: > https://codereview.chromium.org/2932123002/diff/40001/third_party/WebKit/Layo... > File third_party/WebKit/LayoutTests/editing/spelling/context_click_select_misspelling.html (right): > > https://codereview.chromium.org/2932123002/diff/40001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/editing/spelling/context_click_select_misspelling.html:42: const x = document.offsetLeft + rect.left; > On 2017/06/10 at 06:20:39, rlanday wrote: > > If we move the x coord here over not 1 but 2 pixels over to the right, that's far enough into the word to make it be selected on Mac. I think the issue is that the way I currently have the test, it's clicking on the very edge of the <div />, which Chrome doesn't consider to be actually inside the editable text. > > So the root cause seems to incorrect coordinate calculation... Let's assert that the selection exists after the context click and see what happens. > > If the click position is inside the editable text, it should first set selection to '|wellcome home.' and then select the word. > > If the click position is outside the editable text, it should unselect everything instead. The selection does not exist after the context click (Selection.anchorNode and Section.focusNode are null, which means that not only was no text selected, we don't even acknowledge that the click was in the text area, since merely clicking/right-clicking in some text, whether editable or not, seems to be enough to update the selection). So should I modify the coord calculation?
On 2017/06/12 at 20:12:44, rlanday wrote: > On 2017/06/12 at 19:06:26, xiaochengh wrote: > > https://codereview.chromium.org/2932123002/diff/40001/third_party/WebKit/Layo... > > File third_party/WebKit/LayoutTests/editing/spelling/context_click_select_misspelling.html (right): > > > > https://codereview.chromium.org/2932123002/diff/40001/third_party/WebKit/Layo... > > third_party/WebKit/LayoutTests/editing/spelling/context_click_select_misspelling.html:42: const x = document.offsetLeft + rect.left; > > On 2017/06/10 at 06:20:39, rlanday wrote: > > > If we move the x coord here over not 1 but 2 pixels over to the right, that's far enough into the word to make it be selected on Mac. I think the issue is that the way I currently have the test, it's clicking on the very edge of the <div />, which Chrome doesn't consider to be actually inside the editable text. > > > > So the root cause seems to incorrect coordinate calculation... Let's assert that the selection exists after the context click and see what happens. > > > > If the click position is inside the editable text, it should first set selection to '|wellcome home.' and then select the word. > > > > If the click position is outside the editable text, it should unselect everything instead. > > The selection does not exist after the context click (Selection.anchorNode and Section.focusNode are null, which means that not only was no text selected, we don't even acknowledge that the click was in the text area, since merely clicking/right-clicking in some text, whether editable or not, seems to be enough to update the selection). > > So should I modify the coord calculation? Yeah, let's do that! ... And thanks for fixing bugs in the code I wrote!
The CQ bit was checked by rlanday@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...
Ok, I've moved the x coord over two pixels. There's one more thing I had to do, which is to de-select the selection in-between right-clicking the start and end of the word. On macOS, if you right-click immediately after a word with no active selection, it will select the space after the word, but if there *is* a word selected, right-clicking will try to select a word instead (in this case, the same word, so the selection won't change).
lgtm Shifting by 2 px seems pretty hacky but... whatever.
The CQ bit was checked by rlanday@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1497304792223270,
"parent_rev": "f46becbfa68ab91275ca64480fcacbc5b98d771c", "commit_rev":
"e7fe96812663e16778c2517bd3587eded541088e"}
Message was sent while issue was closed.
Description was changed from ========== Add test case for context click at misspelled word boundary On most platforms, right-clicking a misspelled word only selects it and shows suggestions in the context menu if you right-clik in the interior of the word. In https://codereview.chromium.org/2925363002, I modified some of the spellcheck code (specifically, the method that replaces the misspelled word) to support the case where we have a caret selection immediately before or after a word, since tapping at a word boundary should open the spellcheck menu on Android (this feature is still in progress, but this is the behavior for a native EditText widget, so we're trying to match it.) This CL adds a test case to verify we don't accidentally change the behavior for other platforms. BUG=715365 ========== to ========== Add test case for context click at misspelled word boundary On most platforms, right-clicking a misspelled word only selects it and shows suggestions in the context menu if you right-clik in the interior of the word. In https://codereview.chromium.org/2925363002, I modified some of the spellcheck code (specifically, the method that replaces the misspelled word) to support the case where we have a caret selection immediately before or after a word, since tapping at a word boundary should open the spellcheck menu on Android (this feature is still in progress, but this is the behavior for a native EditText widget, so we're trying to match it.) This CL adds a test case to verify we don't accidentally change the behavior for other platforms. BUG=715365 Review-Url: https://codereview.chromium.org/2932123002 Cr-Commit-Position: refs/heads/master@{#478805} Committed: https://chromium.googlesource.com/chromium/src/+/e7fe96812663e16778c2517bd358... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/e7fe96812663e16778c2517bd358... |
