Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2903193003:
MacViews: Tweak kDragToEndIfOutsideVerticalBounds for single line fields. (Closed)
|
Created:
3 years, 7 months ago by tapted Modified:
3 years, 6 months ago Reviewers:
msw CC:
chromium-reviews, nona+watch_chromium.org, James Su, shuchen+watch_chromium.org, yusukes+watch_chromium.org, tfarina, chrome-apps-syd-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMacViews: Tweak kDragToEndIfOutsideVerticalBounds for single line fields.
Currently the direction to extend is based off the y-position. Dragging
above extends to the logical start and dragging below extends to the
logical end. This matches <textarea> and single-line <input> fields in
webcontents, but does not match single line Cocoa text fields such as
the current Chrome Mac omnibox.
Instead, compare x positions of the selection start and the mouse
cursor. For single line RenderText, extend to the end that is in the
visual direction towards the mouse cursor.
BUG=600166
Review-Url: https://codereview.chromium.org/2903193003
Cr-Commit-Position: refs/heads/master@{#475260}
Committed: https://chromium.googlesource.com/chromium/src/+/4a1a83be8f109eb727dd9e8724cce49801279faf
Patch Set 1 #Patch Set 2 : Fixed #Patch Set 3 : such testing #
Total comments: 26
Patch Set 4 : respond to comments #
Messages
Total messages: 33 (27 generated)
The CQ bit was checked by tapted@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 ========== MacViews: When dragging selections up or down, extend based on the mouse x-position BUG=600166 ========== to ========== MacViews: When dragging selections up or down, extend based on the mouse x-position. Currently the direction to extend is based off the y-position (above -> left, below -> right). This matches <textarea> and single-line <input> fields in webcontents, but does not match single line Cocoa text fields such as the current Mac omnibox. Instead, compare x positions, and always extend the visual cursor direction accordingly. BUG=600166 ==========
tapted@chromium.org changed reviewers: + msw@chromium.org
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 tapted@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 ========== MacViews: When dragging selections up or down, extend based on the mouse x-position. Currently the direction to extend is based off the y-position (above -> left, below -> right). This matches <textarea> and single-line <input> fields in webcontents, but does not match single line Cocoa text fields such as the current Mac omnibox. Instead, compare x positions, and always extend the visual cursor direction accordingly. BUG=600166 ========== to ========== MacViews: Tweak kDragToEndIfOutsideVerticalBounds for single line fields. Currently the direction to extend is based off the y-position (above -> left, below -> right). This matches <textarea> and single-line <input> fields in webcontents, but does not match single line Cocoa text fields such as the current Mac omnibox. Instead, compare x positions, and always extend the visual cursor direction towards the mouse for single line fields. BUG=600166 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by tapted@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...
Patchset #3 (id:40001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== MacViews: Tweak kDragToEndIfOutsideVerticalBounds for single line fields. Currently the direction to extend is based off the y-position (above -> left, below -> right). This matches <textarea> and single-line <input> fields in webcontents, but does not match single line Cocoa text fields such as the current Mac omnibox. Instead, compare x positions, and always extend the visual cursor direction towards the mouse for single line fields. BUG=600166 ========== to ========== MacViews: Tweak kDragToEndIfOutsideVerticalBounds for single line fields. Currently the direction to extend is based off the y-position. Dragging above extends to the logical start and dragging below extends to the logical end. This matches <textarea> and single-line <input> fields in webcontents, but does not match single line Cocoa text fields such as the current Chrome Mac omnibox. Instead, compare x positions of the selection start at the mouse cursor. For single line RenderText, extend to the end that is in the visual direction towards the mouse cursor. BUG=600166 ==========
Hi Mike please take a look
nice; some minor comments and test refactoring ideas. https://codereview.chromium.org/2903193003/diff/60001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/2903193003/diff/60001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:911: else optional nit: remove else after return https://codereview.chromium.org/2903193003/diff/60001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:917: else optional nit: remove else after return https://codereview.chromium.org/2903193003/diff/60001/ui/views/controls/label... File ui/views/controls/label_unittest.cc (right): https://codereview.chromium.org/2903193003/diff/60001/ui/views/controls/label... ui/views/controls/label_unittest.cc:109: rtl += static_cast<base::string16::value_type>(*c); q: can you really just cast ascii chars to string16 chars? https://codereview.chromium.org/2903193003/diff/60001/ui/views/controls/label... ui/views/controls/label_unittest.cc:168: // Some tests use cardinal directions to index an array of points above and Please inline the array values and comment on each inlined line, eg: PerformMouseDragTo(gfx::Point(GetCursorPoint(1).x(), -5)); // NW Otherwise, it would be nice if the enum/array order matched the execution order. https://codereview.chromium.org/2903193003/diff/60001/ui/views/controls/label... ui/views/controls/label_unittest.cc:1068: // For multiline, N* extends right, S* extends left. I think you have this backwards? https://codereview.chromium.org/2903193003/diff/60001/ui/views/controls/label... ui/views/controls/label_unittest.cc:1076: EXPECT_STR_EQ(kExtends ? kExtendLeft : "", GetSelectedText()); good test case https://codereview.chromium.org/2903193003/diff/60001/ui/views/controls/label... ui/views/controls/label_unittest.cc:1085: label()->SetText(ASCIIToUTF16("abcdnefgh")); nit: shorten to 5 or 6 (like rtl) chars and remove the 'n'? https://codereview.chromium.org/2903193003/diff/60001/ui/views/controls/label... ui/views/controls/label_unittest.cc:1100: // For single line, extends right unless west. nit: // For single line, western directions extend left, all others extend right. https://codereview.chromium.org/2903193003/diff/60001/ui/views/controls/label... ui/views/controls/label_unittest.cc:1185: // For single line, extends right unless west. ditto rephrasing https://codereview.chromium.org/2903193003/diff/60001/ui/views/controls/textf... File ui/views/controls/textfield/textfield_unittest.cc (right): https://codereview.chromium.org/2903193003/diff/60001/ui/views/controls/textf... ui/views/controls/textfield/textfield_unittest.cc:1576: const int kStartX = GetCursorPositionX(5); optional nit: inline this? https://codereview.chromium.org/2903193003/diff/60001/ui/views/controls/textf... ui/views/controls/textfield/textfield_unittest.cc:1577: const int kRightX = GetCursorPositionX(7); optional nit: unix_hacker naming style to match other consts here? https://codereview.chromium.org/2903193003/diff/60001/ui/views/controls/textf... ui/views/controls/textfield/textfield_unittest.cc:1583: auto SelectionAfterDrag = [&, this](const gfx::Point& end) -> base::string16 { nifty! https://codereview.chromium.org/2903193003/diff/60001/ui/views/controls/textf... ui/views/controls/textfield/textfield_unittest.cc:1590: // Click at |end|. This is important because drags do not count as clicks Since these all have the same start point, we should be able to put the MoveMouseTo(start_point); and PressLeftMouseButton(); before the lambda [executions] and then just make the lambda body { DragMouseTo(end); return textfield_->GetSelectedText(); } (without any mouse release, press, long comment, or restart of move and press) We could also just match what the label tests do and repeat the commands inline: DragMouseTo(gfx::Point(kLeftX, -500)); // NW EXPECT_EQ(expected_left, textfield_->GetSelectedText()); DragMouseTo... EXPECT_EQ... https://codereview.chromium.org/2903193003/diff/60001/ui/views/controls/textf... ui/views/controls/textfield/textfield_unittest.cc:1600: EXPECT_EQ(expected_left, SelectionAfterDrag(end_point[NW])); nit: I'd rather have the coordinates inlined here and just comment " // NW" or similar, then get rid of the enum and |end_point| array.
Description was changed from ========== MacViews: Tweak kDragToEndIfOutsideVerticalBounds for single line fields. Currently the direction to extend is based off the y-position. Dragging above extends to the logical start and dragging below extends to the logical end. This matches <textarea> and single-line <input> fields in webcontents, but does not match single line Cocoa text fields such as the current Chrome Mac omnibox. Instead, compare x positions of the selection start at the mouse cursor. For single line RenderText, extend to the end that is in the visual direction towards the mouse cursor. BUG=600166 ========== to ========== MacViews: Tweak kDragToEndIfOutsideVerticalBounds for single line fields. Currently the direction to extend is based off the y-position. Dragging above extends to the logical start and dragging below extends to the logical end. This matches <textarea> and single-line <input> fields in webcontents, but does not match single line Cocoa text fields such as the current Chrome Mac omnibox. Instead, compare x positions of the selection start and the mouse cursor. For single line RenderText, extend to the end that is in the visual direction towards the mouse cursor. BUG=600166 ==========
Patchset #4 (id:80001) has been deleted
The CQ bit was checked by tapted@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...
Patchset #4 (id:70004) has been deleted
Thanks mike! https://codereview.chromium.org/2903193003/diff/60001/ui/gfx/render_text_harf... File ui/gfx/render_text_harfbuzz.cc (right): https://codereview.chromium.org/2903193003/diff/60001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:911: else On 2017/05/25 18:11:55, msw wrote: > optional nit: remove else after return Done. https://codereview.chromium.org/2903193003/diff/60001/ui/gfx/render_text_harf... ui/gfx/render_text_harfbuzz.cc:917: else On 2017/05/25 18:11:55, msw wrote: > optional nit: remove else after return Done. https://codereview.chromium.org/2903193003/diff/60001/ui/views/controls/label... File ui/views/controls/label_unittest.cc (right): https://codereview.chromium.org/2903193003/diff/60001/ui/views/controls/label... ui/views/controls/label_unittest.cc:109: rtl += static_cast<base::string16::value_type>(*c); On 2017/05/25 18:11:55, msw wrote: > q: can you really just cast ascii chars to string16 chars? yup - the first 255 codepoints of utf-16 is just the ASCII table - http://asecuritysite.com/coding/asc2?val=0%2C255 ASCIIToUTF16 just takes begin/end iterators to char* and uses a templatized range assignment constructor https://codereview.chromium.org/2903193003/diff/60001/ui/views/controls/label... ui/views/controls/label_unittest.cc:168: // Some tests use cardinal directions to index an array of points above and On 2017/05/25 18:11:55, msw wrote: > Please inline the array values and comment on each inlined line, eg: > PerformMouseDragTo(gfx::Point(GetCursorPoint(1).x(), -5)); // NW > Otherwise, it would be nice if the enum/array order matched the execution order. I'll reorder the steps. I meant to leave a comment saying the first two EXPECT in each test were chosen to match directions that were previously being tested (i.e. so it was clear those were not changing) - but that did made it confusing. (and I went with the array since it felt like too much repetition, so I'd like to keep that.. didn't even have the // NW. comments in the array initializer until clang-format decided to collapse lines, but I think they are useful anyway). https://codereview.chromium.org/2903193003/diff/60001/ui/views/controls/label... ui/views/controls/label_unittest.cc:1068: // For multiline, N* extends right, S* extends left. On 2017/05/25 18:11:55, msw wrote: > I think you have this backwards? Whoops - yes. Thanks. https://codereview.chromium.org/2903193003/diff/60001/ui/views/controls/label... ui/views/controls/label_unittest.cc:1085: label()->SetText(ASCIIToUTF16("abcdnefgh")); On 2017/05/25 18:11:55, msw wrote: > nit: shorten to 5 or 6 (like rtl) chars and remove the 'n'? Done. https://codereview.chromium.org/2903193003/diff/60001/ui/views/controls/label... ui/views/controls/label_unittest.cc:1100: // For single line, extends right unless west. On 2017/05/25 18:11:55, msw wrote: > nit: // For single line, western directions extend left, all others extend > right. Done. https://codereview.chromium.org/2903193003/diff/60001/ui/views/controls/label... ui/views/controls/label_unittest.cc:1185: // For single line, extends right unless west. On 2017/05/25 18:11:55, msw wrote: > ditto rephrasing Done. https://codereview.chromium.org/2903193003/diff/60001/ui/views/controls/textf... File ui/views/controls/textfield/textfield_unittest.cc (right): https://codereview.chromium.org/2903193003/diff/60001/ui/views/controls/textf... ui/views/controls/textfield/textfield_unittest.cc:1576: const int kStartX = GetCursorPositionX(5); On 2017/05/25 18:11:56, msw wrote: > optional nit: inline this? Done. https://codereview.chromium.org/2903193003/diff/60001/ui/views/controls/textf... ui/views/controls/textfield/textfield_unittest.cc:1577: const int kRightX = GetCursorPositionX(7); On 2017/05/25 18:11:56, msw wrote: > optional nit: unix_hacker naming style to match other consts here? Done. https://codereview.chromium.org/2903193003/diff/60001/ui/views/controls/textf... ui/views/controls/textfield/textfield_unittest.cc:1590: // Click at |end|. This is important because drags do not count as clicks On 2017/05/25 18:11:56, msw wrote: > Since these all have the same start point, we should be able to put the > MoveMouseTo(start_point); and PressLeftMouseButton(); before the lambda > [executions] and then just make the lambda body { DragMouseTo(end); return > textfield_->GetSelectedText(); } (without any mouse release, press, long > comment, or restart of move and press) We could also just match what the label > tests do and repeat the commands inline: > DragMouseTo(gfx::Point(kLeftX, -500)); // NW > EXPECT_EQ(expected_left, textfield_->GetSelectedText()); > DragMouseTo... > EXPECT_EQ... Done. https://codereview.chromium.org/2903193003/diff/60001/ui/views/controls/textf... ui/views/controls/textfield/textfield_unittest.cc:1600: EXPECT_EQ(expected_left, SelectionAfterDrag(end_point[NW])); On 2017/05/25 18:11:56, msw wrote: > nit: I'd rather have the coordinates inlined here and just comment " // NW" or > similar, then get rid of the enum and |end_point| array. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by tapted@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": 110001, "attempt_start_ts": 1495953962154990,
"parent_rev": "85c36f9e4420de87c3b5cff31d090f158a5252d0", "commit_rev":
"4a1a83be8f109eb727dd9e8724cce49801279faf"}
Message was sent while issue was closed.
Description was changed from ========== MacViews: Tweak kDragToEndIfOutsideVerticalBounds for single line fields. Currently the direction to extend is based off the y-position. Dragging above extends to the logical start and dragging below extends to the logical end. This matches <textarea> and single-line <input> fields in webcontents, but does not match single line Cocoa text fields such as the current Chrome Mac omnibox. Instead, compare x positions of the selection start and the mouse cursor. For single line RenderText, extend to the end that is in the visual direction towards the mouse cursor. BUG=600166 ========== to ========== MacViews: Tweak kDragToEndIfOutsideVerticalBounds for single line fields. Currently the direction to extend is based off the y-position. Dragging above extends to the logical start and dragging below extends to the logical end. This matches <textarea> and single-line <input> fields in webcontents, but does not match single line Cocoa text fields such as the current Chrome Mac omnibox. Instead, compare x positions of the selection start and the mouse cursor. For single line RenderText, extend to the end that is in the visual direction towards the mouse cursor. BUG=600166 Review-Url: https://codereview.chromium.org/2903193003 Cr-Commit-Position: refs/heads/master@{#475260} Committed: https://chromium.googlesource.com/chromium/src/+/4a1a83be8f109eb727dd9e8724cc... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:110001) as https://chromium.googlesource.com/chromium/src/+/4a1a83be8f109eb727dd9e8724cc... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
