Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(421)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 1989143002: MacViews: Correct behavior of move and select commands when selection direction changes. (Closed)

Created:
4 years, 7 months ago by karandeepb
Modified:
4 years, 4 months ago
Reviewers:
tapted
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@move_commands
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

MacViews: Correct 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 the following four commands ids to views::Textfield to account for the different kinds of behavior textfields on Cocoa demonstrate- -IDS_MOVE_TO_BEGINNING_OF_LINE_AND_EXTEND_SELECTION" -IDS_MOVE_TOWARDS_BEGINNING_OF_LINE_AND_MODIFY_SELECTION -IDS_MOVE_TO_END_OF_LINE_AND_EXTEND_SELECTION -IDS_MOVE_TOWARDS_END_OF_LINE_AND_MODIFY_SELECTION RenderText::MoveCursor() is augmented with an enum gfx::SelectionReversedBehavior which accounts for the behavior to follow in case the selection direction is reversed. This CL should have no behavior change for non-Mac platforms. BUG=613438, 586985

Patch Set 1 : #

Total comments: 15

Patch Set 2 : Address review comments. #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+175 lines, -37 lines) Patch
M ui/gfx/render_text.h View 1 chunk +10 lines, -1 line 0 comments Download
M ui/gfx/render_text.cc View 1 3 chunks +41 lines, -2 lines 2 comments Download
M ui/gfx/text_constants.h View 1 1 chunk +28 lines, -0 lines 2 comments Download
M ui/strings/ui_strings.grd View 1 1 chunk +12 lines, -0 lines 0 comments Download
M ui/views/cocoa/bridged_content_view.mm View 3 chunks +6 lines, -6 lines 0 comments Download
M ui/views/cocoa/bridged_native_widget_unittest.mm View 5 chunks +28 lines, -19 lines 0 comments Download
M ui/views/controls/textfield/textfield.cc View 1 3 chunks +29 lines, -4 lines 0 comments Download
M ui/views/controls/textfield/textfield_model.h View 1 chunk +8 lines, -1 line 0 comments Download
M ui/views/controls/textfield/textfield_model.cc View 1 chunk +6 lines, -4 lines 0 comments Download
M ui/views/controls/textfield/textfield_unittest.cc View 1 chunk +7 lines, -0 lines 0 comments Download

Messages

Total messages: 14 (8 generated)
karandeepb
PTAL Trent. Thanks. https://codereview.chromium.org/1989143002/diff/60001/ui/strings/ui_strings.grd File ui/strings/ui_strings.grd (right): https://codereview.chromium.org/1989143002/diff/60001/ui/strings/ui_strings.grd#newcode448 ui/strings/ui_strings.grd:448: <message name="IDS_MOVE_TOWARDS_BEGINNING_OF_LINE_AND_MODIFY_SELECTION" desc="A command to move ...
4 years, 7 months ago (2016-05-20 04:41:48 UTC) #7
karandeepb
Will add some more tests, if this looks ok.
4 years, 7 months ago (2016-05-20 05:02:29 UTC) #8
tapted
https://codereview.chromium.org/1989143002/diff/60001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/1989143002/diff/60001/ui/gfx/render_text.cc#newcode657 ui/gfx/render_text.cc:657: if (select_behavior == SELECTION_EXTEND) nit: needs curlies https://codereview.chromium.org/1989143002/diff/60001/ui/gfx/text_constants.h File ...
4 years, 7 months ago (2016-05-20 06:47:36 UTC) #10
karandeepb
PTAL Trent, thanks. https://codereview.chromium.org/1989143002/diff/60001/ui/gfx/render_text.cc File ui/gfx/render_text.cc (right): https://codereview.chromium.org/1989143002/diff/60001/ui/gfx/render_text.cc#newcode657 ui/gfx/render_text.cc:657: if (select_behavior == SELECTION_EXTEND) On 2016/05/20 ...
4 years, 7 months ago (2016-05-24 07:47:15 UTC) #12
tapted
https://codereview.chromium.org/1989143002/diff/60001/ui/strings/ui_strings.grd File ui/strings/ui_strings.grd (right): https://codereview.chromium.org/1989143002/diff/60001/ui/strings/ui_strings.grd#newcode445 ui/strings/ui_strings.grd:445: <message name="IDS_MOVE_TO_BEGINNING_OF_LINE_AND_EXTEND_SELECTION" desc="A command to move the cursor to ...
4 years, 7 months ago (2016-05-25 04:31:06 UTC) #13
karandeepb
4 years, 7 months ago (2016-05-25 05:04:11 UTC) #14 
https://codereview.chromium.org/1989143002/diff/60001/ui/strings/ui_strings.grd
File ui/strings/ui_strings.grd (right):

https://codereview.chromium.org/1989143002/diff/60001/ui/strings/ui_strings.g...
ui/strings/ui_strings.grd:445: <message
name="IDS_MOVE_TO_BEGINNING_OF_LINE_AND_EXTEND_SELECTION" desc="A command to
move the cursor to the beginning of the line and extend the existing
selection.">
On 2016/05/25 04:31:06, tapted wrote:
> On 2016/05/24 07:47:15, karandeepb wrote:
> > On 2016/05/20 06:47:36, tapted wrote:
> > > I think this is the point where we really need to stop using IDS_* for
> these,
> > > and switch over to an enum. It doesn't make sense to send these to
> > translators,
> > > since the string doesn't appear in the UI anywhere. I think these were
once
> > just
> > > a "convenient" way to represent editing commands, since some of the
actions
> > > appeared in menu items as well (e.g. cut / copy / paste, etc.).
> > > 
> > > A lot of the others should be removed from this file :/
> > 
> > Have started working on a patch for this
> > https://codereview.chromium.org/2007503002/. But it may be some time before
> it's
> > ready. Since this and the transpose patch are ready, can they land first?
> 
> That approach isn't widely used for chrome development. There's not really any
> rush, so we should ensure we're only adding stuff that we'll still need at the
> endpoint. And, e.g., ui_strings.grd gets sent to translators - there's no
reason
> to have "Move To Beginning Of Line And Extend Selection" translated into
dozens
> of languages, so these really do not belong.
> 
> But if it's a big divergence, I don't want to send you down a path that OWNERS
> disagree with. It's probably worth looping in, e.g., msw@ for an opinion about
> what to do here. Maybe there's an underlying reason why these have
historically
> appeared in ui_strings.grd that I don't know about.

These commands were added here - https://codereview.chromium.org/211593006.
Can't really see any significant reason for it. Will hold this patch till the
refactoring is done.

https://codereview.chromium.org/1989143002/diff/60001/ui/strings/ui_strings.g...
ui/strings/ui_strings.grd:448: <message
name="IDS_MOVE_TOWARDS_BEGINNING_OF_LINE_AND_MODIFY_SELECTION" desc="A command
to move the cursor towards the beginning of the line and modify the selection.
If the selection direction changes, the existing selection reduces to a caret.">
On 2016/05/25 04:31:06, tapted wrote:
> On 2016/05/20 04:41:47, karandeepb wrote:
> > I can't think of a good name for this command. Suggestions?
> 
> IDS_MOVE_TO_BEGINNING_OF_LINE_OR_CURSOR_BOUNDARY_AND_MODIFY_SELECTION ?
> 
> But would the same logic apply to DOCUMENT and PARAGRAPH? We don't want a
> combinatorial explosion of these.
> 
> Once we have an enum, or a better encapsulation of the edit command, it might
> make sense to mask one of the bytes in the edit command with a selection
> behaviour. Or to pass two arguments through the TextInputClient, or attach
> selection behaviour on to the KeyEvent itself.

It wouldn't for the time being since Document and Paragraph commands are being
mapped to line commands. However, when we start supporting multi line
textfields, then of course, we'll have to add more commands (and instead of
using SelectionReversedBehavior enum, we can add #ifdef code as you suggested,
and as is done by Blink.)

https://codereview.chromium.org/1989143002/diff/100001/ui/gfx/text_constants.h
File ui/gfx/text_constants.h (right):

https://codereview.chromium.org/1989143002/diff/100001/ui/gfx/text_constants....
ui/gfx/text_constants.h:40: // moveWordLeftAndModifySelection (Alt + Shift +
Left).
On 2016/05/25 04:31:06, tapted wrote:
> Is there a rule we can follow for these without having to pass through
> SelectionReversedBehavior or the new set of commandIds?
> 
> E.g. (somewhat hypothetical - not sure if this will look good) Can we just #if
> defined(OS_MACOSX) inside render_text.cc then, instead of passing through
> |select_behavior|, pass through |associated_command_id|. Then map a command_id
> to its selection behaviour inside render_text.cc
> 
> OR (for Mac) could SELECTION_EXTEND be inferred purely from BreakType ==
> LINE_BREAK, and SELECTION_CARET be inferred purely from BreakType ==
WORD_BREAK?
>  (if not, would adding a new break type like WORD_OR_CARET_BREAK help?)
> 
> 
> This approach may have the benefit of not having to modify ui_strings.grd at
> all, which would cancel out some of my other objections :)

Yes SELECTION_CARET can be inferred from WORD_BREAK, in fact I am doing this
currently. See the #ifdef(OS_MACOSX) I have added to Textfield::ExecuteCommand.
However LINE_BREAK can correspond to all the three different kinds of selection
behaviors on Mac. Blink is able to get around this since it has a mapping for
each of the Mac action messages.

Powered by Google App Engine
This is Rietveld 408576698