Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2925363002:
Add SpellChecker::GetSpellCheckMarkerTouchingSelection() (Closed)
DescriptionAdd SpellChecker::GetSpellCheckMarkerTouchingSelection()
This CL makes some changes to SpellChecker that I need to implement the Android
spellcheck menu in Chrome. On most platforms, users can apply spellcheck
suggestions by right-clicking in the interior of a misspelled word (nothing
happens if you right-click at an endpoint) and selecting a replacement. On
Android, the spellcheck menu is triggered by tapping on a word (either in the
interior or at an endpoint). This means we need to do two things (both done in
this CL):
- Expose a way for a caller to determine if the selection (which may be an
empty caret) is currently touching a spellcheck marker. If some text is
actually selected, we can just see if any spellcheck markers on the text
nodes making up the selected text intersect the selection. If we just have a
caret selection, we need to create a range containing one character on either
side and do the same operation. This method is being added as
SpellChecker::GetSpellCheckMarkerTouchingSelection(). (Note: this will
eventually be useful for suggestion markers as well once those are added, so
we might want to consider putting this method somewhere else).
- Make SpellChecker::ReplaceMisspelledRange() (which looks for a spellcheck
marker intersecting the selection) work in the case where we have a caret
selection touching the spellcheck marker. This is done by using the new
GetSpellCheckMarkerTouchingSelection() method to find the marker.
BUG=715365
Review-Url: https://codereview.chromium.org/2925363002
Cr-Commit-Position: refs/heads/master@{#478380}
Committed: https://chromium.googlesource.com/chromium/src/+/3a98eaedc0233af64b6e647e3dd377321ee2861c
Patch Set 1 #
Total comments: 7
Patch Set 2 : Respond to comments #
Total comments: 3
Patch Set 3 : Respond to commments #
Messages
Total messages: 27 (16 generated)
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...
rlanday@chromium.org changed reviewers: + yosin@chromium.org
yosin@, I've split this off from https://codereview.chromium.org/2931443003 (Add support for Android spellcheck menu in Chrome/WebViews) as you requested. Note: the CL listed as a dependency for this CL isn't really a dependency, but I need to stack the main Android spellcheck menu CL on top of both this CL and the one listed as a dependency.
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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
yosin@chromium.org changed reviewers: + xiaochengh@chromium.org
+xiaochengh@ for second look https://codereview.chromium.org/2925363002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp (right): https://codereview.chromium.org/2925363002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:836: const VisiblePosition& caret_pos = selection.VisibleStart(); nit: s/caret_pos/caret_position/ https://codereview.chromium.org/2925363002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:838: Position range_to_check_start = Could you utilize const variable? const Position& previous_position = PreviousPositionOf(caret_pos).DeepEquivalent(); const Position& range_to_check_start = previous_position.isNull() ? caret_pos : previous_position; ... We might not need to use |range_to_check_start|, e.g. return EphemeralRange( previous_position.IsNull() ? caret_pos : previous_position, next_position.IsNull() ? caret_pos : next_position); https://codereview.chromium.org/2925363002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:847: return EphemeralRange(range_to_check_start, range_to_check_end); Not sure why we expand range both side in one character. Could you explain the reason? I imagine: "ab|cd" => "a^bc|d" where "^" is anchor and "|" is focus. https://codereview.chromium.org/2925363002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:862: Node* start_container = range_to_check.StartPosition().ComputeContainerNode(); nit: s/Node*/Node* const/ https://codereview.chromium.org/2925363002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:865: Node* end_container = range_to_check.EndPosition().ComputeContainerNode(); nit: s/Node*/Node* const/ https://codereview.chromium.org/2925363002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:896: const EphemeralRange misspelled_range( It seems using Collapse() and Extend() is simpler. GetFrame().Selection().SetSelection( SelectionInDOMTree::Builder() .Collapse(Position(container_node, marker->StartOffset()) .Extend(Position(contaner_node, marker->EndOffset()) .Build());
https://codereview.chromium.org/2925363002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp (right): https://codereview.chromium.org/2925363002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:847: return EphemeralRange(range_to_check_start, range_to_check_end); On 2017/06/09 at 01:27:09, yosin_UTC9 wrote: > Not sure why we expand range both side in one character. > Could you explain the reason? > > I imagine: > "ab|cd" => "a^bc|d" where "^" is anchor and "|" is focus. This is so the method works properly if we have a caret selection immediately before or after a marker. We need to expand the selection so that we're actually inside a text node. For example, say we have something like: <span>word1 <b>word2</b></span> If we have a caret selection immediately before "word2", it's not necessarily positioned relative to a text node (it could be positioned relative to the <b> element, or the <span>). I don't think there's any operation we can apply to the caret position to necessarily convert it to be relative to the correct text node since there's one text node immediately before the <b> element starts ("word1 ") and one immediately after ("word2"). But if we expand one character before and one character after, then we get a range that necessarily intersects any spellcheck markers the caret is touching. Are there any cases where this doesn't work properly (e.g. we return a marker we shouldn't)?
The CQ bit was checked by rlanday@chromium.org to run a CQ dry run
Updated
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/06/09 at 01:49:12, rlanday wrote: > https://codereview.chromium.org/2925363002/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp (right): > > https://codereview.chromium.org/2925363002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:847: return EphemeralRange(range_to_check_start, range_to_check_end); > On 2017/06/09 at 01:27:09, yosin_UTC9 wrote: > > Not sure why we expand range both side in one character. > > Could you explain the reason? > > > > I imagine: > > "ab|cd" => "a^bc|d" where "^" is anchor and "|" is focus. > > This is so the method works properly if we have a caret selection immediately before or after a marker. We need to expand the selection so that we're actually inside a text node. For example, say we have something like: > > <span>word1 <b>word2</b></span> > > If we have a caret selection immediately before "word2", it's not necessarily positioned relative to a text node (it could be positioned relative to the <b> element, or the <span>). I don't think there's any operation we can apply to the caret position to necessarily convert it to be relative to the correct text node since there's one text node immediately before the <b> element starts ("word1 ") and one immediately after ("word2"). But if we expand one character before and one character after, then we get a range that necessarily intersects any spellcheck markers the caret is touching. > > Are there any cases where this doesn't work properly (e.g. we return a marker we shouldn't)? No, I don't have. Someone will find such case if it exists.
lgtm w/ nit https://codereview.chromium.org/2925363002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp (right): https://codereview.chromium.org/2925363002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:836: const VisiblePosition& caret_position = selection.VisibleStart(); Could you add an example, your provided in reply?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Does this patch change the behavior on Win/Linux when right clicking at the edge of a misspelling? If so, is the behavior change desired? We have some layout tests in editing/spelling for the interaction between context menu and misspelling markers, but they don't cover the above case. Should we add some tests there? https://codereview.chromium.org/2925363002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp (right): https://codereview.chromium.org/2925363002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:617: const Position& caret_positionition = Irrelevant change. https://codereview.chromium.org/2925363002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:815: namespace { We already have an anonymous namespace at the beginning of this file. No need to create a new one.
On 2017/06/09 at 04:17:27, xiaochengh wrote: > Does this patch change the behavior on Win/Linux when right clicking at the edge of a misspelling? If so, is the behavior change desired? No, it does not. This CL makes the replace operation when the selection is at the edge of a misspelling, but I'm pretty sure the code for whether or not to highlight the misspelled word and show the suggestions in the context menu uses separate logic. I will double-check since I haven't looked at it in a while. > We have some layout tests in editing/spelling for the interaction between context menu and misspelling markers, but they don't cover the above case. > > Should we add some tests there? I will look into it (might put these up in a second CL so they can be reviewed separately).
The CQ bit was checked by rlanday@chromium.org
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/2925363002/#ps40001 (title: "Respond to commments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1497030136739030,
"parent_rev": "cd4a0934f93239d0791e80d7a5da5b81eea368d9", "commit_rev":
"3a98eaedc0233af64b6e647e3dd377321ee2861c"}
Message was sent while issue was closed.
Description was changed from ========== Add SpellChecker::GetSpellCheckMarkerTouchingSelection() This CL makes some changes to SpellChecker that I need to implement the Android spellcheck menu in Chrome. On most platforms, users can apply spellcheck suggestions by right-clicking in the interior of a misspelled word (nothing happens if you right-click at an endpoint) and selecting a replacement. On Android, the spellcheck menu is triggered by tapping on a word (either in the interior or at an endpoint). This means we need to do two things (both done in this CL): - Expose a way for a caller to determine if the selection (which may be an empty caret) is currently touching a spellcheck marker. If some text is actually selected, we can just see if any spellcheck markers on the text nodes making up the selected text intersect the selection. If we just have a caret selection, we need to create a range containing one character on either side and do the same operation. This method is being added as SpellChecker::GetSpellCheckMarkerTouchingSelection(). (Note: this will eventually be useful for suggestion markers as well once those are added, so we might want to consider putting this method somewhere else). - Make SpellChecker::ReplaceMisspelledRange() (which looks for a spellcheck marker intersecting the selection) work in the case where we have a caret selection touching the spellcheck marker. This is done by using the new GetSpellCheckMarkerTouchingSelection() method to find the marker. BUG=715365 ========== to ========== Add SpellChecker::GetSpellCheckMarkerTouchingSelection() This CL makes some changes to SpellChecker that I need to implement the Android spellcheck menu in Chrome. On most platforms, users can apply spellcheck suggestions by right-clicking in the interior of a misspelled word (nothing happens if you right-click at an endpoint) and selecting a replacement. On Android, the spellcheck menu is triggered by tapping on a word (either in the interior or at an endpoint). This means we need to do two things (both done in this CL): - Expose a way for a caller to determine if the selection (which may be an empty caret) is currently touching a spellcheck marker. If some text is actually selected, we can just see if any spellcheck markers on the text nodes making up the selected text intersect the selection. If we just have a caret selection, we need to create a range containing one character on either side and do the same operation. This method is being added as SpellChecker::GetSpellCheckMarkerTouchingSelection(). (Note: this will eventually be useful for suggestion markers as well once those are added, so we might want to consider putting this method somewhere else). - Make SpellChecker::ReplaceMisspelledRange() (which looks for a spellcheck marker intersecting the selection) work in the case where we have a caret selection touching the spellcheck marker. This is done by using the new GetSpellCheckMarkerTouchingSelection() method to find the marker. BUG=715365 Review-Url: https://codereview.chromium.org/2925363002 Cr-Commit-Position: refs/heads/master@{#478380} Committed: https://chromium.googlesource.com/chromium/src/+/3a98eaedc0233af64b6e647e3dd3... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/3a98eaedc0233af64b6e647e3dd3... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
