Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2228833002:
MacViews: Fix behavior of move and select commands when selection direction changes. (Closed)
|
Created:
4 years, 4 months ago by karandeepb Modified:
4 years, 4 months ago CC:
chromium-reviews, tfarina, James Su, shuchen+watch_chromium.org, yusukes+watch_chromium.org, nona+watch_chromium.org, chrome-apps-syd-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@use_text_commands Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMacViews: Fix behavior of move and select commands when selection direction changes.
Currently the following commands do not behave similarly to Cocoa textfields
when the selection direction changes-
-moveWordForwardAndModifySelection
-moveWordBackwardAndModifySelection
-moveToBeginningOfLineAndModifySelection
-moveToEndOfLineAndModifySelection
-moveToBeginningOfParagraphAndModifySelection
-moveToEndOfParagraphAndModifySelection
-moveToEndOfDocumentAndModifySelection
-moveToBeginningOfDocumentAndModifySelection
-moveParagraphForwardAndModifySelection
-moveParagraphBackwardAndModifySelection
-moveWordRightAndModifySelection
-moveWordLeftAndModifySelection
-moveToLeftEndOfLineAndModifySelection
-moveToRightEndOfLineAndModifySelection
This CL adds a new enum gfx::SelectionBehavior which specifies whether a
selection is to be made and how. RenderText::MoveCursor() is modified to take
this enum as a parameter. Also, two new text edit commands are added to the
ui::TextEditCommand enum-
-MOVE_PARAGRAPH_BACKWARD_AND_MODIFY_SELECTION
-MOVE_PARAGRAPH_FORWARD_AND_MODIFY_SELECTION
Textfield::IsTextEditCommandEnabled is modified to enable the following commands on
Mac-
-MOVE_DOWN:
-MOVE_DOWN_AND_MODIFY_SELECTION:
-MOVE_PAGE_DOWN:
-MOVE_PAGE_DOWN_AND_MODIFY_SELECTION:
-MOVE_PAGE_UP:
-MOVE_PAGE_UP_AND_MODIFY_SELECTION:
-MOVE_UP:
-MOVE_UP_AND_MODIFY_SELECTION:
Further, Textfield::ExecuteEditCommand is modified to set the correct
SelectionBehavior value for the different text editing commands. Also, lots of
tests are added.
This CL should have no behavior change for non-Mac platforms.
BUG=613438, 586985
Committed: https://crrev.com/84ab6fac6177ca421e01391de629bcfa36fda381
Cr-Commit-Position: refs/heads/master@{#412702}
Patch Set 1 #
Total comments: 32
Patch Set 2 : Change move cursor logic. #Patch Set 3 : Change MoveCursor API. #Patch Set 4 : Some tests. #Patch Set 5 : Add tests. #Patch Set 6 : More tests. #Patch Set 7 : Fix compile. #
Total comments: 9
Patch Set 8 : Change synthetic event for pageUp/Down and style improvements. #Patch Set 9 : Rename SELECTION_DEFAULT to SELECTION_RETAIN #Patch Set 10 : Add apple rdar in comment. #
Total comments: 27
Patch Set 11 : Rebase. #Patch Set 12 : Address review comments. #Dependent Patchsets: Messages
Total messages: 68 (51 generated)
Description was changed from ========== -- Enabled moveAndSelect tests Tests pass Extend commands Word commands BUG= ========== to ========== -- Enabled moveAndSelect tests Tests pass Extend commands Word commands BUG= ==========
The CQ bit was checked by karandeepb@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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
The CQ bit was checked by karandeepb@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 ========== -- Enabled moveAndSelect tests Tests pass Extend commands Word commands BUG= ========== to ========== MacViews: Fix behavior of move and select commands when selection direction changes. Currently the following commands do not behave similarly to Cocoa textfields when the selection direction changes- -moveWordForwardAndModifySelection -moveWordBackwardAndModifySelection -moveToBeginningOfLineAndModifySelection -moveToEndOfLineAndModifySelection -moveToBeginningOfParagraphAndModifySelection -moveToEndOfParagraphAndModifySelection -moveToEndOfDocumentAndModifySelection -moveToBeginningOfDocumentAndModifySelection -moveParagraphForwardAndModifySelection -moveParagraphBackwardAndModifySelection -moveWordRightAndModifySelection -moveWordLeftAndModifySelection -moveToLeftEndOfLineAndModifySelection -moveToRightEndOfLineAndModifySelection This CL adds a new enum gfx::SelectionReversedBehavior which specifies how a move and select command behaves if the selection direction is reversed. RenderText::MoveCursor() is modified to take this enum as a parameter. Also, two new text edit commands are added to the ui::TextEditCommand enum- -MOVE_PARAGRAPH_BACKWARD_AND_MODIFY_SELECTION -MOVE_PARAGRAPH_FORWARD_AND_MODIFY_SELECTION Textfield::IsTextEditCommandEnabled is modified to enable the following commands on Mac- -MOVE_DOWN: -MOVE_DOWN_AND_MODIFY_SELECTION: -MOVE_PAGE_DOWN: -MOVE_PAGE_DOWN_AND_MODIFY_SELECTION: -MOVE_PAGE_UP: -MOVE_PAGE_UP_AND_MODIFY_SELECTION: -MOVE_UP: -MOVE_UP_AND_MODIFY_SELECTION: Further, Textfield::ExecuteEditCommand is modified to set the correct SelectionReversedBehavior value for the different text editing commands. This CL should have no behavior change for non-Mac platforms. BUG=613438, 586985 ==========
karandeepb@chromium.org changed reviewers: + tapted@chromium.org
PTAL Trent. This is similar to https://codereview.chromium.org/1989143002/ which I had held off due to text editing refactoring.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The API doesn't feel right - I think the CL is trying too hard to avoid refactoring fallout. Suggestions below, but we should consult with an OWNER too Also, the RenderText changes should have a corresponding test in render_text_unittest.cc .. it's actually weird - there are some tests disabled on Mac because they don't work on RenderTextMac, but the tests should just be changed to invoke RenderText::CreateInstanceForEditing() rather than RenderText::CreateInstance(). You can probably loop in http://crbug.com/131618 https://codereview.chromium.org/2228833002/diff/60001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/2228833002/diff/60001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:213: DCHECK(!(range1.GetMax() < range2.GetMin() || DCHECK(range1.Intersects(range2))? https://codereview.chromium.org/2228833002/diff/60001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:222: } ui/gfx OWNERS might be interested in a Range::Union() method (to go with Intersects..), but I wouldn't add it unless they suggest it https://codereview.chromium.org/2228833002/diff/60001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:658: DCHECK(select_behavior != SELECTION_EXTEND || break_type == LINE_BREAK); What's the behaviour if these DCHECKs fail? (is it bad? can we just ignore?). If we put these assertions in, they need to be documented in the header too with a reason (and the use-case that explains why we can't just infer |select_behaviour| from |break_type| itself. Also don't be scared of putting DCHECKs in statements. E.g. I think this would be clearer with something like switch (break_type) { case CHARACTER_BREAK: // Direction can't change when moving by single characters. EXPECT_EQ(SELECTION_DEFAULT, select_behavior) break; case WORD_BREAK: // <Explain why it's not relevant - not just that it isn't - is it just because Mac doesn't do it?>. EXPECT_NE(SELECTION_EXTEND, select_behavior); break; case LINE_BREAK: // Anything goes. } and... _sometimes_ (maybe not here) for cases like this there's a way to design the API so that the compiler can enforce the precondition. E.g. by extending the BreakType enum or turning it into a bitfield (and keeping `bool select`). E.g. CHARACTER_BREAK WORD_BREAK_SWAP_SELECTION WORD_BREAK_COLLAPSE_SELECTION LINE_BREAK_SWAP_SELECTION LINE_BREAK_EXTEND_SELECTION LINE_BREAK_COLLAPSE_SELECTION https://codereview.chromium.org/2228833002/diff/60001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:662: // cursor now corresponds to the new selection. Take |select_behavior| into cursor -> Cursor, or "The cursor" https://codereview.chromium.org/2228833002/diff/60001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/2228833002/diff/60001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:338: SelectionReversedBehavior select_behavior = SELECTION_DEFAULT); This isn't a good use of default arguments - if |select| is true we want the caller to think about how it should behave - see later comment. https://codereview.chromium.org/2228833002/diff/60001/ui/gfx/text_constants.h File ui/gfx/text_constants.h (right): https://codereview.chromium.org/2228833002/diff/60001/ui/gfx/text_constants.h... ui/gfx/text_constants.h:25: enum SelectionReversedBehavior { What about calling this just `SelectionBehavior` and removing the separate `bool select` argument. bool arguments that need explainers in the function comments are usually discouraged (and mixing bool arguments with default arguments can lead to a lot of confusion since so many things convert to bool -- e.g. you'd get no compile error passing SELECTION_START_NEW as the `bool select` argument, but it will convert to `false` and do the wrong thing). We'd just need a new enum for SELECTION_NONE (or CLEAR/CANCEL/COLLAPSE) - https://codereview.chromium.org/2228833002/diff/60001/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_native_widget_unittest.mm (right): https://codereview.chromium.org/2228833002/diff/60001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget_unittest.mm:331: void TestEditingCommands(NSArray* selectors, bool test_rtl = true); nit(optional): bool arguments with a default can be hard to refactor. Here it's probably fine, but in a public API we'd go for something like enum TestCase { ALL, LTR_ONLY }; https://codereview.chromium.org/2228833002/diff/60001/ui/views/controls/textf... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2228833002/diff/60001/ui/views/controls/textf... ui/views/controls/textfield/textfield.cc:1657: gfx::SelectionReversedBehavior::SELECTION_START_NEW); nit: `::SelectionReversedBehavior` not needed, more below
karandeepb@chromium.org changed reviewers: + msw@chromium.org
I like the idea of renaming SelectionReversedBehavior to SelectionBehavior and removing the `bool select` argument. Will work on the patch. Also adding msw@ in case he has some thoughts regarding this.
Sorry, perhaps I'm missing something, but this seems overly complex. https://codereview.chromium.org/2228833002/diff/60001/ui/gfx/text_constants.h File ui/gfx/text_constants.h (right): https://codereview.chromium.org/2228833002/diff/60001/ui/gfx/text_constants.h... ui/gfx/text_constants.h:41: SELECTION_CARET, Why do we need explicit behavior for this? The existing behavior should work as: "a |bc d" -> ALT+SHIFT+RIGHT -> "a |bc| d" -> ALT+SHIFT+LEFT -> "a |bc d" The selection should already collapse to a cursor when the range is empty. Do you want the selection to collapse in this case? That seems very wrong: "a |bc de| f" -> ALT+SHIFT+LEFT -> "a |bc de f" If that's actually what you'd want, why not supply false for |modify_selection|? Sorry if I'm not understanding this correctly! https://codereview.chromium.org/2228833002/diff/60001/ui/views/controls/textf... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2228833002/diff/60001/ui/views/controls/textf... ui/views/controls/textfield/textfield.cc:1652: PlatformStyle::kLineSelectionBehavior); I'd like to avoid the behavior enum if possible. Could we encapsulate this behavior within RenderText, using an OS_MAC preprocessor block (or some ui/gfx-level platform-specific flags) to simply extend the selection for |break_type == LINE_BREAK && select|? From my quick testing, it really seems like Mac does the magic selection-extending for all move-by-line/para/page/doc and select commands; it seems like all the (gfx::LINE_BREAK, ..., true) calls here should be supplying PlatformStyle::kLineSelectionBehavior. https://codereview.chromium.org/2228833002/diff/60001/ui/views/controls/textf... ui/views/controls/textfield/textfield.cc:1657: gfx::SelectionReversedBehavior::SELECTION_START_NEW); Shouldn't these also extend the selection on mac? (afaict from blink's behavior) https://codereview.chromium.org/2228833002/diff/60001/ui/views/controls/textf... ui/views/controls/textfield/textfield.cc:1670: PlatformStyle::kLineSelectionBehavior); Can you define the platform-specific constants here? This extra layer of indirection (with a platform-specific value) makes the code even more difficult to understand, especially when it's defined elsewhere. https://codereview.chromium.org/2228833002/diff/60001/ui/views/controls/textf... ui/views/controls/textfield/textfield.cc:1675: gfx::SelectionReversedBehavior::SELECTION_START_NEW); Shouldn't these also extend the selection on mac? (afaict from blink's behavior) https://codereview.chromium.org/2228833002/diff/60001/ui/views/controls/textf... ui/views/controls/textfield/textfield.cc:1678: model_->MoveCursor(gfx::LINE_BREAK, begin, true, gfx::SELECTION_CARET); ditto: Why don't these (para fwd/back) extend the selection on mac? Why would they collapse the selection into a cursor instead of extending or simply modifying the existing selection? If you want a cursor instead, why not just pass false for |select| here on Mac?
I think encapsulating in gfx::RenderText would be a big win too, but it might mean passing the edit command itself through and having a map in gfx::RenderText There was a question about this on the pre-EditCommand CL at https://codereview.chromium.org/1989143002/diff/100001/ui/gfx/text_constants.... - maybe things are in a better shape to explore that now that there isn't a dependency on ui_string.grd https://codereview.chromium.org/2228833002/diff/60001/ui/gfx/text_constants.h File ui/gfx/text_constants.h (right): https://codereview.chromium.org/2228833002/diff/60001/ui/gfx/text_constants.h... ui/gfx/text_constants.h:41: SELECTION_CARET, On 2016/08/11 23:46:37, msw wrote: > Why do we need explicit behavior for this? The existing behavior should work as: > "a |bc d" -> ALT+SHIFT+RIGHT -> "a |bc| d" -> ALT+SHIFT+LEFT -> "a |bc d" > The selection should already collapse to a cursor when the range is empty. > > Do you want the selection to collapse in this case? That seems very wrong: > "a |bc de| f" -> ALT+SHIFT+LEFT -> "a |bc de f" > > If that's actually what you'd want, why not supply false for |modify_selection|? > Sorry if I'm not understanding this correctly! Yeah this tripped me up too :). Karan can probably explain better, but I think the problem is that SELECTION_CARET only comes into play when the selection gets reversed. e.g. one t|wo three| four // selected left-to-right Alt+Shift+Left one t|wo |three four // selected left-to-right - still selected so |modify_selection| can't be false Alt+Shift+Left one t|wo three four // would reverse: SELECTION_CARET Alt+Shift+Left one |t|wo three four // selected right-to-left
https://codereview.chromium.org/2228833002/diff/60001/ui/gfx/text_constants.h File ui/gfx/text_constants.h (right): https://codereview.chromium.org/2228833002/diff/60001/ui/gfx/text_constants.h... ui/gfx/text_constants.h:41: SELECTION_CARET, On 2016/08/11 23:46:37, msw wrote: > Do you want the selection to collapse in this case? That seems very wrong: > "a |bc de| f" -> ALT+SHIFT+LEFT -> "a |bc de f" No we don't. As Trent said, this only comes into play, when the selection direction is reversed. In this case, the result currently is "a |bc |de f", so the selection direction is the same and the enum value won't come into play. > If that's actually what you'd want, why not supply false for |modify_selection|? > Sorry if I'm not understanding this correctly! It's ok. I should probably work on making the code clearer. Here's the need for this. Consider- abc|def Alt+Shift+Right abc|def| Alt+Shift+Left |abc|def -> Current behavior abc|def -> What we want on Mac. Also see the example given by Trent. https://codereview.chromium.org/2228833002/diff/60001/ui/views/controls/textf... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2228833002/diff/60001/ui/views/controls/textf... ui/views/controls/textfield/textfield.cc:1652: PlatformStyle::kLineSelectionBehavior); On 2016/08/11 23:46:37, msw wrote: > I'd like to avoid the behavior enum if possible. Could we encapsulate this > behavior within RenderText, using an OS_MAC preprocessor block (or some > ui/gfx-level platform-specific flags) to simply extend the selection for > |break_type == LINE_BREAK && select|? From my quick testing, it really seems > like Mac does the magic selection-extending for all move-by-line/para/page/doc > and select commands; it seems like all the (gfx::LINE_BREAK, ..., true) calls > here should be supplying PlatformStyle::kLineSelectionBehavior. For CHARACTER_BREAK, the enum is not really relevant. For WORD_BREAK, its possible to derive the behavior based on the platform. But for LINE_BREAK, Mac does exhibit all the 3 kinds of behavior- Eg consider -SELECTION_EXTEND one tw|o three Command+Shift+Right one tw|o three| Command+Shift+Left |one two three| -SELECTION_START_NEW one tw|o three Shift+Down one tw|o three| Shift+Up |one tw|o three -SELECTION_CARET one tw|o three Alt+Shift+Up one tw|o three| Alt+Shift+Down one tw|o three Alt+Shift+Down |one tw|o three Now for the LINE_BREAK case, you are correct that most of the Mac commands use the SELECTION_EXTEND behavior. SELECTION_START_NEW behavior is used by moveUp/DownAndModifySelection and movePageUp/DownAndModifySelection. SELECTION_CARET is used by moveParagraphForward/BackwardAndModifySelection. A case can be made that these commands are not that common and thus not worth the added complexity, in which case we can make the SELECTION_EXTEND behavior the default on Mac for LINE_BREAK commands. Over to Trent, if we really need the different behaviors on Mac. The commands affected would be- moveUp/DownAndModifySelection movePageUp/DownAndModifySelection moveParagraphForward/BackwardAndModifySelection and the behavior will be incorrect only for the case when the selection direction is reversed. https://codereview.chromium.org/2228833002/diff/60001/ui/views/controls/textf... ui/views/controls/textfield/textfield.cc:1657: gfx::SelectionReversedBehavior::SELECTION_START_NEW); On 2016/08/11 23:46:38, msw wrote: > Shouldn't these also extend the selection on mac? (afaict from blink's behavior) Nope, try yourself with Shift+Up/Down. And the Blink behavior is also correct. Here is a page which lists most of the default system key bindings on Mac- https://www.hcs.harvard.edu/~jrus/site/system-bindings.html https://codereview.chromium.org/2228833002/diff/60001/ui/views/controls/textf... ui/views/controls/textfield/textfield.cc:1670: PlatformStyle::kLineSelectionBehavior); On 2016/08/11 23:46:38, msw wrote: > Can you define the platform-specific constants here? This extra layer of > indirection (with a platform-specific value) makes the code even more difficult > to understand, especially when it's defined elsewhere. Will do! https://codereview.chromium.org/2228833002/diff/60001/ui/views/controls/textf... ui/views/controls/textfield/textfield.cc:1675: gfx::SelectionReversedBehavior::SELECTION_START_NEW); On 2016/08/11 23:46:38, msw wrote: > Shouldn't these also extend the selection on mac? (afaict from blink's behavior) Nope, try yourself with Shift+Up/Down. https://codereview.chromium.org/2228833002/diff/60001/ui/views/controls/textf... ui/views/controls/textfield/textfield.cc:1678: model_->MoveCursor(gfx::LINE_BREAK, begin, true, gfx::SELECTION_CARET); On 2016/08/11 23:46:38, msw wrote: > ditto: Why don't these (para fwd/back) extend the selection on mac? Why would > they collapse the selection into a cursor instead of extending or simply > modifying the existing selection? If you want a cursor instead, why not just > pass false for |select| here on Mac? >Why don't these (para fwd/back) extend the selection on mac? This is how Cocoa does it. Try with Alt+Shift+Up/Down. >Why would > they collapse the selection into a cursor instead of extending or simply > modifying the existing selection? If you want a cursor instead, why not just >pass false for |select| here on Mac? This is only relevant if the selection direction is reversed.
I was definitely wrong! Thanks for giving some more concrete examples. It would be quite nice if we didn't need a behavior enum and we could consolidate more of the logic into RenderText, but go ahead with what's needed to match Mac's behavior. Please add some RenderText and Textfield tests that cover these commands as best we can. https://codereview.chromium.org/2228833002/diff/60001/ui/gfx/text_constants.h File ui/gfx/text_constants.h (right): https://codereview.chromium.org/2228833002/diff/60001/ui/gfx/text_constants.h... ui/gfx/text_constants.h:41: SELECTION_CARET, On 2016/08/12 01:26:31, karandeepb wrote: > On 2016/08/11 23:46:37, msw wrote: > > > Do you want the selection to collapse in this case? That seems very wrong: > > "a |bc de| f" -> ALT+SHIFT+LEFT -> "a |bc de f" > No we don't. As Trent said, this only comes into play, when the selection > direction is reversed. In this case, the result currently is "a |bc |de f", so > the selection direction is the same and the enum value won't come into play. > > > If that's actually what you'd want, why not supply false for > |modify_selection|? > > Sorry if I'm not understanding this correctly! > > It's ok. I should probably work on making the code clearer. > Here's the need for this. Consider- > abc|def > > Alt+Shift+Right > abc|def| > > Alt+Shift+Left > |abc|def -> Current behavior > abc|def -> What we want on Mac. > > Also see the example given by Trent. > Ah, we need tests for this like TextfieldTest.ControlAndSelectTest. https://codereview.chromium.org/2228833002/diff/60001/ui/views/controls/textf... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2228833002/diff/60001/ui/views/controls/textf... ui/views/controls/textfield/textfield.cc:1675: gfx::SelectionReversedBehavior::SELECTION_START_NEW); On 2016/08/12 01:26:31, karandeepb wrote: > On 2016/08/11 23:46:38, msw wrote: > > Shouldn't these also extend the selection on mac? (afaict from blink's > behavior) > > Nope, try yourself with Shift+Up/Down. Wow, Mac's behavior is so odd; we definitely need tests.
The CQ bit was checked by karandeepb@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: linux_chromium_chromeos_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 karandeepb@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 karandeepb@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: linux_chromium_chromeos_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 karandeepb@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...
I think you're still going, but I'm about to disappear for ~4 weeks so here's a few notes :). Mostly lg, and ui/views/cocoa lgtm with a couple of nits. https://codereview.chromium.org/2228833002/diff/200001/ui/gfx/render_text_uni... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/2228833002/diff/200001/ui/gfx/render_text_uni... ui/gfx/render_text_unittest.cc:923: std::unique_ptr<RenderText> render_text(RenderText::CreateInstance()); Changing this to RenderText::CreateInstanceForEditing() should allow the test to run (and hopefully pass!) on Mac as well. Same for TruncatedCursorMovementRTL https://codereview.chromium.org/2228833002/diff/200001/ui/gfx/render_text_uni... ui/gfx/render_text_unittest.cc:1271: std::unique_ptr<RenderText> render_text(RenderText::CreateInstance()); same here, and a bunch of things below https://codereview.chromium.org/2228833002/diff/200001/ui/gfx/render_text_uni... ui/gfx/render_text_unittest.cc:1659: std::unique_ptr<RenderText> render_text(RenderText::CreateInstance()); and here.. https://codereview.chromium.org/2228833002/diff/200001/ui/gfx/text_constants.h File ui/gfx/text_constants.h (right): https://codereview.chromium.org/2228833002/diff/200001/ui/gfx/text_constants.... ui/gfx/text_constants.h:22: // "|ab|cde" for SELECTION_START_NEW, selection direction from right to left. update wrt START_NEW? https://codereview.chromium.org/2228833002/diff/200001/ui/gfx/text_constants.... ui/gfx/text_constants.h:30: SELECTION_DEFAULT, maybe SELECTION_RETAIN? -- seems like a better balance to SELECTION_NONE https://codereview.chromium.org/2228833002/diff/200001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_native_widget_unittest.mm (right): https://codereview.chromium.org/2228833002/diff/200001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget_unittest.mm:1120: // The behavior of NSTextView for RTL strings is buggy for some move and can you file a bug about the NSTextView behavior and reference it here?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Thanks for the review Trent! >Changing this to RenderText::CreateInstanceForEditing() should allow the test to >run (and hopefully pass!) on Mac as well. Same for TruncatedCursorMovementRTL I was thinking of using parameterized tests to test both the RenderText implementations on Mac, in a followup.
The CQ bit was checked by karandeepb@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: Fix behavior of move and select commands when selection direction changes. Currently the following commands do not behave similarly to Cocoa textfields when the selection direction changes- -moveWordForwardAndModifySelection -moveWordBackwardAndModifySelection -moveToBeginningOfLineAndModifySelection -moveToEndOfLineAndModifySelection -moveToBeginningOfParagraphAndModifySelection -moveToEndOfParagraphAndModifySelection -moveToEndOfDocumentAndModifySelection -moveToBeginningOfDocumentAndModifySelection -moveParagraphForwardAndModifySelection -moveParagraphBackwardAndModifySelection -moveWordRightAndModifySelection -moveWordLeftAndModifySelection -moveToLeftEndOfLineAndModifySelection -moveToRightEndOfLineAndModifySelection This CL adds a new enum gfx::SelectionReversedBehavior which specifies how a move and select command behaves if the selection direction is reversed. RenderText::MoveCursor() is modified to take this enum as a parameter. Also, two new text edit commands are added to the ui::TextEditCommand enum- -MOVE_PARAGRAPH_BACKWARD_AND_MODIFY_SELECTION -MOVE_PARAGRAPH_FORWARD_AND_MODIFY_SELECTION Textfield::IsTextEditCommandEnabled is modified to enable the following commands on Mac- -MOVE_DOWN: -MOVE_DOWN_AND_MODIFY_SELECTION: -MOVE_PAGE_DOWN: -MOVE_PAGE_DOWN_AND_MODIFY_SELECTION: -MOVE_PAGE_UP: -MOVE_PAGE_UP_AND_MODIFY_SELECTION: -MOVE_UP: -MOVE_UP_AND_MODIFY_SELECTION: Further, Textfield::ExecuteEditCommand is modified to set the correct SelectionReversedBehavior value for the different text editing commands. This CL should have no behavior change for non-Mac platforms. BUG=613438, 586985 ========== to ========== MacViews: Fix behavior of move and select commands when selection direction changes. Currently the following commands do not behave similarly to Cocoa textfields when the selection direction changes- -moveWordForwardAndModifySelection -moveWordBackwardAndModifySelection -moveToBeginningOfLineAndModifySelection -moveToEndOfLineAndModifySelection -moveToBeginningOfParagraphAndModifySelection -moveToEndOfParagraphAndModifySelection -moveToEndOfDocumentAndModifySelection -moveToBeginningOfDocumentAndModifySelection -moveParagraphForwardAndModifySelection -moveParagraphBackwardAndModifySelection -moveWordRightAndModifySelection -moveWordLeftAndModifySelection -moveToLeftEndOfLineAndModifySelection -moveToRightEndOfLineAndModifySelection This CL adds a new enum gfx::SelectionBehavior which specifies whether a selection is to be made and how. RenderText::MoveCursor() is modified to take this enum as a parameter. Also, two new text edit commands are added to the ui::TextEditCommand enum- -MOVE_PARAGRAPH_BACKWARD_AND_MODIFY_SELECTION -MOVE_PARAGRAPH_FORWARD_AND_MODIFY_SELECTION Textfield::IsTextEditCommandEnabled is modified to enable the following commands on Mac- -MOVE_DOWN: -MOVE_DOWN_AND_MODIFY_SELECTION: -MOVE_PAGE_DOWN: -MOVE_PAGE_DOWN_AND_MODIFY_SELECTION: -MOVE_PAGE_UP: -MOVE_PAGE_UP_AND_MODIFY_SELECTION: -MOVE_UP: -MOVE_UP_AND_MODIFY_SELECTION: Further, Textfield::ExecuteEditCommand is modified to set the correct SelectionBehavior value for the different text editing commands. Also, lots of tests are added. This CL should have no behavior change for non-Mac platforms. BUG=613438, 586985 ==========
Patchset #4 (id:120001) has been deleted
The CQ bit was checked by karandeepb@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.
PTAL msw@. Have simplified the implementation in render_text.cc, added tests and changed the SelectionReversedBehavior enum to SelectionBehavior. https://codereview.chromium.org/2228833002/diff/60001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/2228833002/diff/60001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:213: DCHECK(!(range1.GetMax() < range2.GetMin() || On 2016/08/11 03:44:07, tapted (OOO until Sep 15) wrote: > DCHECK(range1.Intersects(range2))? Removed this function. Also Range::Intersects is weird. The function is not symmetric, A intersects B does not imply B intersects A. https://codereview.chromium.org/2228833002/diff/60001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:658: DCHECK(select_behavior != SELECTION_EXTEND || break_type == LINE_BREAK); On 2016/08/11 03:44:07, tapted (OOO until Sep 15) wrote: > What's the behaviour if these DCHECKs fail? (is it bad? can we just ignore?). If > we put these assertions in, they need to be documented in the header too with a > reason (and the use-case that explains why we can't just infer > |select_behaviour| from |break_type| itself. Also don't be scared of putting > DCHECKs in statements. E.g. I think this would be clearer with something like > > switch (break_type) { > case CHARACTER_BREAK: > // Direction can't change when moving by single characters. > EXPECT_EQ(SELECTION_DEFAULT, select_behavior) > break; > case WORD_BREAK: > // <Explain why it's not relevant - not just that it isn't - is it just > because Mac doesn't do it?>. > EXPECT_NE(SELECTION_EXTEND, select_behavior); > break; > case LINE_BREAK: > // Anything goes. > } > > > > and... _sometimes_ (maybe not here) for cases like this there's a way to design > the API so that the compiler can enforce the precondition. E.g. by extending the > BreakType enum or turning it into a bitfield (and keeping `bool select`). > > E.g. > > CHARACTER_BREAK > WORD_BREAK_SWAP_SELECTION > WORD_BREAK_COLLAPSE_SELECTION > LINE_BREAK_SWAP_SELECTION > LINE_BREAK_EXTEND_SELECTION > LINE_BREAK_COLLAPSE_SELECTION > Removed the DCHECK. The behavior is now correct and intuitive even for the cases not being used, like (CHARACTER_BREAK/WORD_BREAK, SELECTION_EXTEND). https://codereview.chromium.org/2228833002/diff/60001/ui/gfx/render_text.cc#n... ui/gfx/render_text.cc:662: // cursor now corresponds to the new selection. Take |select_behavior| into On 2016/08/11 03:44:07, tapted (OOO until Sep 15) wrote: > cursor -> Cursor, or "The cursor" Changed to |cursor| since I want to refer to the local variable. https://codereview.chromium.org/2228833002/diff/60001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/2228833002/diff/60001/ui/gfx/render_text.h#ne... ui/gfx/render_text.h:338: SelectionReversedBehavior select_behavior = SELECTION_DEFAULT); On 2016/08/11 03:44:07, tapted (OOO until Sep 15) wrote: > This isn't a good use of default arguments - if |select| is true we want the > caller to think about how it should behave - see later comment. Acknowledged, removed the use of default args. https://codereview.chromium.org/2228833002/diff/60001/ui/gfx/text_constants.h File ui/gfx/text_constants.h (right): https://codereview.chromium.org/2228833002/diff/60001/ui/gfx/text_constants.h... ui/gfx/text_constants.h:25: enum SelectionReversedBehavior { On 2016/08/11 03:44:07, tapted (OOO until Sep 15) wrote: > What about calling this just `SelectionBehavior` and removing the separate `bool > select` argument. bool arguments that need explainers in the function comments > are usually discouraged (and mixing bool arguments with default arguments can > lead to a lot of confusion since so many things convert to bool -- e.g. you'd > get no compile error passing SELECTION_START_NEW as the `bool select` argument, > but it will convert to `false` and do the wrong thing). > > We'd just need a new enum for SELECTION_NONE (or CLEAR/CANCEL/COLLAPSE) - Nice suggestion. Done. https://codereview.chromium.org/2228833002/diff/60001/ui/gfx/text_constants.h... ui/gfx/text_constants.h:41: SELECTION_CARET, On 2016/08/12 21:58:30, msw wrote: > On 2016/08/12 01:26:31, karandeepb wrote: > > On 2016/08/11 23:46:37, msw wrote: > > > > > Do you want the selection to collapse in this case? That seems very wrong: > > > "a |bc de| f" -> ALT+SHIFT+LEFT -> "a |bc de f" > > No we don't. As Trent said, this only comes into play, when the selection > > direction is reversed. In this case, the result currently is "a |bc |de f", so > > the selection direction is the same and the enum value won't come into play. > > > > > If that's actually what you'd want, why not supply false for > > |modify_selection|? > > > Sorry if I'm not understanding this correctly! > > > > It's ok. I should probably work on making the code clearer. > > Here's the need for this. Consider- > > abc|def > > > > Alt+Shift+Right > > abc|def| > > > > Alt+Shift+Left > > |abc|def -> Current behavior > > abc|def -> What we want on Mac. > > > > Also see the example given by Trent. > > > > Ah, we need tests for this like TextfieldTest.ControlAndSelectTest. Added tests. Was planning to add them after the initial review. https://codereview.chromium.org/2228833002/diff/60001/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_native_widget_unittest.mm (right): https://codereview.chromium.org/2228833002/diff/60001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget_unittest.mm:331: void TestEditingCommands(NSArray* selectors, bool test_rtl = true); On 2016/08/11 03:44:08, tapted (OOO until Sep 15) wrote: > nit(optional): bool arguments with a default can be hard to refactor. Here it's > probably fine, but in a public API we'd go for something like enum TestCase { > ALL, LTR_ONLY }; Done. https://codereview.chromium.org/2228833002/diff/60001/ui/views/controls/textf... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2228833002/diff/60001/ui/views/controls/textf... ui/views/controls/textfield/textfield.cc:1657: gfx::SelectionReversedBehavior::SELECTION_START_NEW); On 2016/08/11 03:44:08, tapted (OOO until Sep 15) wrote: > nit: `::SelectionReversedBehavior` not needed, more below Done. https://codereview.chromium.org/2228833002/diff/60001/ui/views/controls/textf... ui/views/controls/textfield/textfield.cc:1675: gfx::SelectionReversedBehavior::SELECTION_START_NEW); On 2016/08/12 21:58:30, msw wrote: > On 2016/08/12 01:26:31, karandeepb wrote: > > On 2016/08/11 23:46:38, msw wrote: > > > Shouldn't these also extend the selection on mac? (afaict from blink's > > behavior) > > > > Nope, try yourself with Shift+Up/Down. > > Wow, Mac's behavior is so odd; we definitely need tests. Done. https://codereview.chromium.org/2228833002/diff/200001/ui/gfx/text_constants.h File ui/gfx/text_constants.h (right): https://codereview.chromium.org/2228833002/diff/200001/ui/gfx/text_constants.... ui/gfx/text_constants.h:22: // "|ab|cde" for SELECTION_START_NEW, selection direction from right to left. On 2016/08/16 03:28:31, tapted (OOO until Sep 15) wrote: > update wrt START_NEW? Done. https://codereview.chromium.org/2228833002/diff/200001/ui/gfx/text_constants.... ui/gfx/text_constants.h:30: SELECTION_DEFAULT, On 2016/08/16 03:28:31, tapted (OOO until Sep 15) wrote: > maybe SELECTION_RETAIN? -- seems like a better balance to SELECTION_NONE Done. https://codereview.chromium.org/2228833002/diff/200001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_native_widget_unittest.mm (right): https://codereview.chromium.org/2228833002/diff/200001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget_unittest.mm:1120: // The behavior of NSTextView for RTL strings is buggy for some move and On 2016/08/16 03:28:31, tapted (OOO until Sep 15) wrote: > can you file a bug about the NSTextView behavior and reference it here? Done. https://codereview.chromium.org/2228833002/diff/260001/ui/gfx/text_constants.h File ui/gfx/text_constants.h (right): https://codereview.chromium.org/2228833002/diff/260001/ui/gfx/text_constants.... ui/gfx/text_constants.h:30: SELECTION_RETAIN, This needs a better name, maybe?
This is so much better (especially now that I more clearly understand Mac's intended behavior); very nice tests and comments! lgtm with nits. https://codereview.chromium.org/2228833002/diff/260001/ui/gfx/render_text_uni... File ui/gfx/render_text_unittest.cc (right): https://codereview.chromium.org/2228833002/diff/260001/ui/gfx/render_text_uni... ui/gfx/render_text_unittest.cc:1049: TEST_F(RenderTextTest, MoveCursor_Word) { aside: I'm glad this works on all the bots, we have some weird move-by-word behavior on windows that I tried to remove in https://codereview.chromium.org/1130683005 https://codereview.chromium.org/2228833002/diff/260001/ui/gfx/text_constants.h File ui/gfx/text_constants.h (right): https://codereview.chromium.org/2228833002/diff/260001/ui/gfx/text_constants.... ui/gfx/text_constants.h:30: SELECTION_RETAIN, On 2016/08/16 10:24:52, karandeepb wrote: > This needs a better name, maybe? I think it's okay, it retains the start position. https://codereview.chromium.org/2228833002/diff/260001/ui/gfx/text_constants.... ui/gfx/text_constants.h:32: // Use for move-and-select commands which want the existing selection to be nit: s/which/that/ https://codereview.chromium.org/2228833002/diff/260001/ui/gfx/text_constants.... ui/gfx/text_constants.h:38: // Use for move-and-select commands which want the existing selection to nit: s/which/that/ https://codereview.chromium.org/2228833002/diff/260001/ui/gfx/text_constants.... ui/gfx/text_constants.h:44: // No selection. To be used for move commands, which don't want to cause a nit: s/which/that/ and remove the comma after commands. https://codereview.chromium.org/2228833002/diff/260001/ui/gfx/text_constants.... ui/gfx/text_constants.h:45: // selection. nit: "selection, and which want to collapse any pre-existing selection." https://codereview.chromium.org/2228833002/diff/260001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/2228833002/diff/260001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:864: // The pageDown: action message is bound to the key combination nit: Are these pageDown/pageUp comments just to clarify that 'EF_ALT_DOWN' == "the option button is pressed"? If so, they're probably not necessary. https://codereview.chromium.org/2228833002/diff/260001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_native_widget_unittest.mm (right): https://codereview.chromium.org/2228833002/diff/260001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget_unittest.mm:558: if (cases == TestCase::ALL) nit: curly braces https://codereview.chromium.org/2228833002/diff/260001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget_unittest.mm:1120: TEST_F(BridgedNativeWidgetTest, TextInput_MoveAndSelectEditingCommands) { Thanks for enabling this test for LTR! https://codereview.chromium.org/2228833002/diff/260001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget_unittest.mm:1123: // rdar://27863290. q: I'm not familiar with rdar, but I found via codesearch that this might correspond to http://www.openradar.me/27863290, but that doesn't seem to work. Can you verify that this is correct and tell me how to use this? Maybe [also] use the long-form address? (ok as-is if it's meant to open a special mac app) https://codereview.chromium.org/2228833002/diff/260001/ui/views/controls/text... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2228833002/diff/260001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:1687: model_->MoveCursor(gfx::LINE_BREAK, begin, gfx::SELECTION_CARET); nit: If we ever add Win/Cros/Linux support, this shouldn't use gfx::SELECTION_CARET, afaik. Use kWordSelectionBehavior or add a kParagraphSelectionBehavior instead; ditto for MOVE_PARAGRAPH_FORWARD_AND_MODIFY_SELECTION below. https://codereview.chromium.org/2228833002/diff/260001/ui/views/controls/text... File ui/views/controls/textfield/textfield_unittest.cc (right): https://codereview.chromium.org/2228833002/diff/260001/ui/views/controls/text... ui/views/controls/textfield/textfield_unittest.cc:918: const int kStringLength = 11; optional nit: I think the test is more readable if these literals are inlined; ditto in the other fixtures below. https://codereview.chromium.org/2228833002/diff/260001/ui/views/controls/text... ui/views/controls/textfield/textfield_unittest.cc:921: EXPECT_EQ(gfx::Range(kStringLength), textfield_->GetSelectedRange()); nit: remove this (seems unnecessary) https://codereview.chromium.org/2228833002/diff/260001/ui/views/controls/text... ui/views/controls/textfield/textfield_unittest.cc:1038: EXPECT_EQ(gfx::Range(kInitialCursorPosition), textfield_->GetSelectedRange()); You'll need to update this test if you address my comment in render_text.cc about not using SELECTION_CARET for these commands.
The CQ bit was checked by karandeepb@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 karandeepb@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 #12 (id:300001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks msw@. https://codereview.chromium.org/2228833002/diff/260001/ui/gfx/text_constants.h File ui/gfx/text_constants.h (right): https://codereview.chromium.org/2228833002/diff/260001/ui/gfx/text_constants.... ui/gfx/text_constants.h:30: SELECTION_RETAIN, On 2016/08/16 18:37:58, msw wrote: > On 2016/08/16 10:24:52, karandeepb wrote: > > This needs a better name, maybe? > > I think it's okay, it retains the start position. Acknowledged. https://codereview.chromium.org/2228833002/diff/260001/ui/gfx/text_constants.... ui/gfx/text_constants.h:32: // Use for move-and-select commands which want the existing selection to be On 2016/08/16 18:37:58, msw wrote: > nit: s/which/that/ Done. https://codereview.chromium.org/2228833002/diff/260001/ui/gfx/text_constants.... ui/gfx/text_constants.h:38: // Use for move-and-select commands which want the existing selection to On 2016/08/16 18:37:58, msw wrote: > nit: s/which/that/ Done. https://codereview.chromium.org/2228833002/diff/260001/ui/gfx/text_constants.... ui/gfx/text_constants.h:44: // No selection. To be used for move commands, which don't want to cause a On 2016/08/16 18:37:58, msw wrote: > nit: s/which/that/ and remove the comma after commands. Done. https://codereview.chromium.org/2228833002/diff/260001/ui/gfx/text_constants.... ui/gfx/text_constants.h:45: // selection. On 2016/08/16 18:37:58, msw wrote: > nit: "selection, and which want to collapse any pre-existing selection." Done. https://codereview.chromium.org/2228833002/diff/260001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/2228833002/diff/260001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_content_view.mm:864: // The pageDown: action message is bound to the key combination On 2016/08/16 18:37:58, msw wrote: > nit: Are these pageDown/pageUp comments just to clarify that 'EF_ALT_DOWN' == > "the option button is pressed"? If so, they're probably not necessary. I added them since it's a bit counter-intuitive that the action pageDown: is bound to the key combination [Option+PageDown] and not [PageDown], which in turn is bound to the action scrollPageDown:, weird! https://codereview.chromium.org/2228833002/diff/260001/ui/views/cocoa/bridged... File ui/views/cocoa/bridged_native_widget_unittest.mm (right): https://codereview.chromium.org/2228833002/diff/260001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget_unittest.mm:558: if (cases == TestCase::ALL) On 2016/08/16 18:37:58, msw wrote: > nit: curly braces Done. https://codereview.chromium.org/2228833002/diff/260001/ui/views/cocoa/bridged... ui/views/cocoa/bridged_native_widget_unittest.mm:1123: // rdar://27863290. On 2016/08/16 18:37:58, msw wrote: > q: I'm not familiar with rdar, but I found via codesearch that this might > correspond to http://www.openradar.me/27863290, but that doesn't seem to work. > Can you verify that this is correct and tell me how to use this? Maybe [also] > use the long-form address? (ok as-is if it's meant to open a special mac app) OpenRadar is an external system where you can log copies of radars that you file against Apple so that other people can see them. You should be able to view the actual bug report at http://bugreport.apple.com. See the internal Google page related to Apple radars for how to login. (Don't think I should be pasting an internal URL here). As for why we use url's of the form rdar://{IssueID}, I am also not sure. This seems to be the practice in the existing code base and in the internal Google page about radars. https://codereview.chromium.org/2228833002/diff/260001/ui/views/controls/text... File ui/views/controls/textfield/textfield.cc (right): https://codereview.chromium.org/2228833002/diff/260001/ui/views/controls/text... ui/views/controls/textfield/textfield.cc:1687: model_->MoveCursor(gfx::LINE_BREAK, begin, gfx::SELECTION_CARET); On 2016/08/16 18:37:58, msw wrote: > nit: If we ever add Win/Cros/Linux support, this shouldn't use > gfx::SELECTION_CARET, afaik. Use kWordSelectionBehavior or add a > kParagraphSelectionBehavior instead; ditto for > MOVE_PARAGRAPH_FORWARD_AND_MODIFY_SELECTION below. Done. https://codereview.chromium.org/2228833002/diff/260001/ui/views/controls/text... File ui/views/controls/textfield/textfield_unittest.cc (right): https://codereview.chromium.org/2228833002/diff/260001/ui/views/controls/text... ui/views/controls/textfield/textfield_unittest.cc:918: const int kStringLength = 11; On 2016/08/16 18:37:58, msw wrote: > optional nit: I think the test is more readable if these literals are inlined; > ditto in the other fixtures below. Done. https://codereview.chromium.org/2228833002/diff/260001/ui/views/controls/text... ui/views/controls/textfield/textfield_unittest.cc:921: EXPECT_EQ(gfx::Range(kStringLength), textfield_->GetSelectedRange()); On 2016/08/16 18:37:58, msw wrote: > nit: remove this (seems unnecessary) Done. https://codereview.chromium.org/2228833002/diff/260001/ui/views/controls/text... ui/views/controls/textfield/textfield_unittest.cc:1038: EXPECT_EQ(gfx::Range(kInitialCursorPosition), textfield_->GetSelectedRange()); On 2016/08/16 18:37:58, msw wrote: > You'll need to update this test if you address my comment in render_text.cc > about not using SELECTION_CARET for these commands. Done.
karandeepb@chromium.org changed reviewers: + sky@chromium.org
PTAL sky@ for ui/base review.
ui/base LGTM
The CQ bit was checked by karandeepb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org, msw@chromium.org Link to the patchset: https://codereview.chromium.org/2228833002/#ps320001 (title: "Address review comments.")
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.
Description was changed from ========== MacViews: Fix behavior of move and select commands when selection direction changes. Currently the following commands do not behave similarly to Cocoa textfields when the selection direction changes- -moveWordForwardAndModifySelection -moveWordBackwardAndModifySelection -moveToBeginningOfLineAndModifySelection -moveToEndOfLineAndModifySelection -moveToBeginningOfParagraphAndModifySelection -moveToEndOfParagraphAndModifySelection -moveToEndOfDocumentAndModifySelection -moveToBeginningOfDocumentAndModifySelection -moveParagraphForwardAndModifySelection -moveParagraphBackwardAndModifySelection -moveWordRightAndModifySelection -moveWordLeftAndModifySelection -moveToLeftEndOfLineAndModifySelection -moveToRightEndOfLineAndModifySelection This CL adds a new enum gfx::SelectionBehavior which specifies whether a selection is to be made and how. RenderText::MoveCursor() is modified to take this enum as a parameter. Also, two new text edit commands are added to the ui::TextEditCommand enum- -MOVE_PARAGRAPH_BACKWARD_AND_MODIFY_SELECTION -MOVE_PARAGRAPH_FORWARD_AND_MODIFY_SELECTION Textfield::IsTextEditCommandEnabled is modified to enable the following commands on Mac- -MOVE_DOWN: -MOVE_DOWN_AND_MODIFY_SELECTION: -MOVE_PAGE_DOWN: -MOVE_PAGE_DOWN_AND_MODIFY_SELECTION: -MOVE_PAGE_UP: -MOVE_PAGE_UP_AND_MODIFY_SELECTION: -MOVE_UP: -MOVE_UP_AND_MODIFY_SELECTION: Further, Textfield::ExecuteEditCommand is modified to set the correct SelectionBehavior value for the different text editing commands. Also, lots of tests are added. This CL should have no behavior change for non-Mac platforms. BUG=613438, 586985 ========== to ========== MacViews: Fix behavior of move and select commands when selection direction changes. Currently the following commands do not behave similarly to Cocoa textfields when the selection direction changes- -moveWordForwardAndModifySelection -moveWordBackwardAndModifySelection -moveToBeginningOfLineAndModifySelection -moveToEndOfLineAndModifySelection -moveToBeginningOfParagraphAndModifySelection -moveToEndOfParagraphAndModifySelection -moveToEndOfDocumentAndModifySelection -moveToBeginningOfDocumentAndModifySelection -moveParagraphForwardAndModifySelection -moveParagraphBackwardAndModifySelection -moveWordRightAndModifySelection -moveWordLeftAndModifySelection -moveToLeftEndOfLineAndModifySelection -moveToRightEndOfLineAndModifySelection This CL adds a new enum gfx::SelectionBehavior which specifies whether a selection is to be made and how. RenderText::MoveCursor() is modified to take this enum as a parameter. Also, two new text edit commands are added to the ui::TextEditCommand enum- -MOVE_PARAGRAPH_BACKWARD_AND_MODIFY_SELECTION -MOVE_PARAGRAPH_FORWARD_AND_MODIFY_SELECTION Textfield::IsTextEditCommandEnabled is modified to enable the following commands on Mac- -MOVE_DOWN: -MOVE_DOWN_AND_MODIFY_SELECTION: -MOVE_PAGE_DOWN: -MOVE_PAGE_DOWN_AND_MODIFY_SELECTION: -MOVE_PAGE_UP: -MOVE_PAGE_UP_AND_MODIFY_SELECTION: -MOVE_UP: -MOVE_UP_AND_MODIFY_SELECTION: Further, Textfield::ExecuteEditCommand is modified to set the correct SelectionBehavior value for the different text editing commands. Also, lots of tests are added. This CL should have no behavior change for non-Mac platforms. BUG=613438, 586985 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:320001)
Message was sent while issue was closed.
Description was changed from ========== MacViews: Fix behavior of move and select commands when selection direction changes. Currently the following commands do not behave similarly to Cocoa textfields when the selection direction changes- -moveWordForwardAndModifySelection -moveWordBackwardAndModifySelection -moveToBeginningOfLineAndModifySelection -moveToEndOfLineAndModifySelection -moveToBeginningOfParagraphAndModifySelection -moveToEndOfParagraphAndModifySelection -moveToEndOfDocumentAndModifySelection -moveToBeginningOfDocumentAndModifySelection -moveParagraphForwardAndModifySelection -moveParagraphBackwardAndModifySelection -moveWordRightAndModifySelection -moveWordLeftAndModifySelection -moveToLeftEndOfLineAndModifySelection -moveToRightEndOfLineAndModifySelection This CL adds a new enum gfx::SelectionBehavior which specifies whether a selection is to be made and how. RenderText::MoveCursor() is modified to take this enum as a parameter. Also, two new text edit commands are added to the ui::TextEditCommand enum- -MOVE_PARAGRAPH_BACKWARD_AND_MODIFY_SELECTION -MOVE_PARAGRAPH_FORWARD_AND_MODIFY_SELECTION Textfield::IsTextEditCommandEnabled is modified to enable the following commands on Mac- -MOVE_DOWN: -MOVE_DOWN_AND_MODIFY_SELECTION: -MOVE_PAGE_DOWN: -MOVE_PAGE_DOWN_AND_MODIFY_SELECTION: -MOVE_PAGE_UP: -MOVE_PAGE_UP_AND_MODIFY_SELECTION: -MOVE_UP: -MOVE_UP_AND_MODIFY_SELECTION: Further, Textfield::ExecuteEditCommand is modified to set the correct SelectionBehavior value for the different text editing commands. Also, lots of tests are added. This CL should have no behavior change for non-Mac platforms. BUG=613438, 586985 ========== to ========== MacViews: Fix behavior of move and select commands when selection direction changes. Currently the following commands do not behave similarly to Cocoa textfields when the selection direction changes- -moveWordForwardAndModifySelection -moveWordBackwardAndModifySelection -moveToBeginningOfLineAndModifySelection -moveToEndOfLineAndModifySelection -moveToBeginningOfParagraphAndModifySelection -moveToEndOfParagraphAndModifySelection -moveToEndOfDocumentAndModifySelection -moveToBeginningOfDocumentAndModifySelection -moveParagraphForwardAndModifySelection -moveParagraphBackwardAndModifySelection -moveWordRightAndModifySelection -moveWordLeftAndModifySelection -moveToLeftEndOfLineAndModifySelection -moveToRightEndOfLineAndModifySelection This CL adds a new enum gfx::SelectionBehavior which specifies whether a selection is to be made and how. RenderText::MoveCursor() is modified to take this enum as a parameter. Also, two new text edit commands are added to the ui::TextEditCommand enum- -MOVE_PARAGRAPH_BACKWARD_AND_MODIFY_SELECTION -MOVE_PARAGRAPH_FORWARD_AND_MODIFY_SELECTION Textfield::IsTextEditCommandEnabled is modified to enable the following commands on Mac- -MOVE_DOWN: -MOVE_DOWN_AND_MODIFY_SELECTION: -MOVE_PAGE_DOWN: -MOVE_PAGE_DOWN_AND_MODIFY_SELECTION: -MOVE_PAGE_UP: -MOVE_PAGE_UP_AND_MODIFY_SELECTION: -MOVE_UP: -MOVE_UP_AND_MODIFY_SELECTION: Further, Textfield::ExecuteEditCommand is modified to set the correct SelectionBehavior value for the different text editing commands. Also, lots of tests are added. This CL should have no behavior change for non-Mac platforms. BUG=613438, 586985 Committed: https://crrev.com/84ab6fac6177ca421e01391de629bcfa36fda381 Cr-Commit-Position: refs/heads/master@{#412702} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/84ab6fac6177ca421e01391de629bcfa36fda381 Cr-Commit-Position: refs/heads/master@{#412702} |
