Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2754543004:
Make Selection#modify() with word granularity not to skip punctuation (Closed)
|
Created:
3 years, 9 months ago by yosin_UTC9 Modified:
3 years, 9 months ago CC:
blink-reviews, chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake Selection#modify() with word granularity not to skip punctuation
This patch makes |Selection#modify()| with word granularity to work as same
as mouse double-clicking to avoid skipping punctuation characters.
Example: Ctrl+ArrowRight or selection.modify('move', 'forward', 'word')
Before this patch: |ab := bc -> ab := |bc
After this patch: |ab := bc -> ab |:= bc
Where "|" represent caret.
Note: this is Windows only behavior.
BUG=699903
TEST=attached test especially for "first-letter-word-boundary.html"
Review-Url: https://codereview.chromium.org/2754543004
Cr-Commit-Position: refs/heads/master@{#459700}
Committed: https://chromium.googlesource.com/chromium/src/+/117a5ba5073a1c78d08d3be3210afc09af96158c
Patch Set 1 : 2017-03-15T17:17:51 #Patch Set 2 : 2017-03-16T18:32:12 #Patch Set 3 : 2017-03-17T19:52:56 #
Total comments: 4
Patch Set 4 : 2017-03-21T18:18:41 #Patch Set 5 : 2017-03-26T01:40:34 rebase #
Messages
Total messages: 29 (21 generated)
The CQ bit was checked by yosin@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: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Description was changed from ========== 2017-03-15T17:17:51 2017-03-15T17:17:11 BUG= ========== to ========== editing/pasteboard/4242293-1.html Linux Mac editing/text-iterator/first-letter-word-boundary.html Linux ==========
Description was changed from ========== editing/pasteboard/4242293-1.html Linux Mac editing/text-iterator/first-letter-word-boundary.html Linux ========== to ========== editing/pasteboard/4242293-1.html Linux Mac Win editing/text-iterator/first-letter-word-boundary.html Linux Win ==========
Description was changed from ========== editing/pasteboard/4242293-1.html Linux Mac Win editing/text-iterator/first-letter-word-boundary.html Linux Win ========== to ========== editing/pasteboard/4242293-1.html Linux Mac Win editing/text-iterator/first-letter-word-boundary.html Linux Win BUG=699903 ==========
The CQ bit was checked by yosin@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.
The CQ bit was checked by yosin@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...
Description was changed from
==========
editing/pasteboard/4242293-1.html Linux Mac Win
editing/text-iterator/first-letter-word-boundary.html Linux Win
BUG=699903
==========
to
==========
Make Selection#modify() with word granularity not to skip punctuation
This patch makes |Selection#modify()| with word granularity to work as same
as mouse double-clicking to avoid skipping punctuation characters.
Example: Ctrl+ArrowLeft or selection.modify('move', 'forward', 'word')
Before this patch: |ab := bc -> ab := |bc
After this patch: |ab := bc -> ab |:= bc
Where "|" represent caret.
Note: this is Windows only behavior.
BUG=699903
TEST=attached test especially for "first-letter-word-boundary.html"
==========
yosin@chromium.org changed reviewers: + tkent@chromium.org, xiaochengh@chromium.org, yoichio@chromium.org
PTAL https://codereview.chromium.org/2754543004/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/editing/selection/extend-selection-enclosing-block-win-expected.txt (right): https://codereview.chromium.org/2754543004/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/selection/extend-selection-enclosing-block-win-expected.txt:16: Extending left: "Lorem "[(0,5), (0,0), (0,0), (0,5)], "ipsum dolor sit"[(0,6)] Due to a bug[1], I could not rewrite this test to us |assert_selection()|. [1] crbug.com/702049 Ctrl+Right from LTR to RTL makes infinite loop
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I'm a little skeptical about what happened in extend-selection-enclosing-block-win.html... Besides, the code change in VisibleUnits and VisibleSelection looks good. Could you separate them into another patch? https://codereview.chromium.org/2754543004/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/editing/selection/extend-selection-enclosing-block-win-expected.txt (right): https://codereview.chromium.org/2754543004/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/selection/extend-selection-enclosing-block-win-expected.txt:16: Extending left: "Lorem "[(0,5), (0,0), (0,0), (0,5)], "ipsum dolor sit"[(0,6)] On 2017/03/17 at 11:00:12, yosin_UTC9 wrote: > Due to a bug[1], I could not rewrite this test to us |assert_selection()|. > > [1] crbug.com/702049 Ctrl+Right from LTR to RTL makes infinite loop Could you explain why the test cannot be rewritten? Even for just transcribing the current behavior? https://codereview.chromium.org/2754543004/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/editing/selection/modify_extend/extend_by_word_002.html (right): https://codereview.chromium.org/2754543004/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/selection/modify_extend/extend_by_word_002.html:33: // should be at 'Steak & |Ribs'. A deeper reason is that Blink Why did I say the correct focus is 'Steak & |Ribs'... 'Steak |& Ribs' seems pretty reasonable. Could you remove my comments about non-Mac behavior?
> Example: Ctrl+ArrowLeft or selection.modify('move', 'forward', 'word')
This description should be Ctrl+ArrowRight?
Description was changed from
==========
Make Selection#modify() with word granularity not to skip punctuation
This patch makes |Selection#modify()| with word granularity to work as same
as mouse double-clicking to avoid skipping punctuation characters.
Example: Ctrl+ArrowLeft or selection.modify('move', 'forward', 'word')
Before this patch: |ab := bc -> ab := |bc
After this patch: |ab := bc -> ab |:= bc
Where "|" represent caret.
Note: this is Windows only behavior.
BUG=699903
TEST=attached test especially for "first-letter-word-boundary.html"
==========
to
==========
Make Selection#modify() with word granularity not to skip punctuation
This patch makes |Selection#modify()| with word granularity to work as same
as mouse double-clicking to avoid skipping punctuation characters.
Example: Ctrl+ArrowRight or selection.modify('move', 'forward', 'word')
Before this patch: |ab := bc -> ab := |bc
After this patch: |ab := bc -> ab |:= bc
Where "|" represent caret.
Note: this is Windows only behavior.
BUG=699903
TEST=attached test especially for "first-letter-word-boundary.html"
==========
On 2017/03/21 at 05:52:38, yoichio wrote:
> > Example: Ctrl+ArrowLeft or selection.modify('move', 'forward', 'word')
> This description should be Ctrl+ArrowRight?
PTAL
Yes. Updated.
PTAL https://codereview.chromium.org/2754543004/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/editing/selection/modify_extend/extend_by_word_002.html (right): https://codereview.chromium.org/2754543004/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/editing/selection/modify_extend/extend_by_word_002.html:33: // should be at 'Steak & |Ribs'. A deeper reason is that Blink On 2017/03/17 at 22:49:02, Xiaocheng wrote: > Why did I say the correct focus is 'Steak & |Ribs'... > > 'Steak |& Ribs' seems pretty reasonable. > > Could you remove my comments about non-Mac behavior? Done.
lgtm
The CQ bit was checked by yosin@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": 80001, "attempt_start_ts": 1490580192433290,
"parent_rev": "fe22ae6b4a2c4b0989efb3c8ea8035b62431c9e7", "commit_rev":
"117a5ba5073a1c78d08d3be3210afc09af96158c"}
Message was sent while issue was closed.
Description was changed from
==========
Make Selection#modify() with word granularity not to skip punctuation
This patch makes |Selection#modify()| with word granularity to work as same
as mouse double-clicking to avoid skipping punctuation characters.
Example: Ctrl+ArrowRight or selection.modify('move', 'forward', 'word')
Before this patch: |ab := bc -> ab := |bc
After this patch: |ab := bc -> ab |:= bc
Where "|" represent caret.
Note: this is Windows only behavior.
BUG=699903
TEST=attached test especially for "first-letter-word-boundary.html"
==========
to
==========
Make Selection#modify() with word granularity not to skip punctuation
This patch makes |Selection#modify()| with word granularity to work as same
as mouse double-clicking to avoid skipping punctuation characters.
Example: Ctrl+ArrowRight or selection.modify('move', 'forward', 'word')
Before this patch: |ab := bc -> ab := |bc
After this patch: |ab := bc -> ab |:= bc
Where "|" represent caret.
Note: this is Windows only behavior.
BUG=699903
TEST=attached test especially for "first-letter-word-boundary.html"
Review-Url: https://codereview.chromium.org/2754543004
Cr-Commit-Position: refs/heads/master@{#459700}
Committed:
https://chromium.googlesource.com/chromium/src/+/117a5ba5073a1c78d08d3be3210a...
==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/117a5ba5073a1c78d08d3be3210a... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
