Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1912993002:
MacViews: Implement NSResponder deletion action messages. (Closed)
|
Created:
4 years, 8 months ago by karandeepb Modified:
4 years, 7 months ago Reviewers:
tapted CC:
chromium-reviews, tfarina, chrome-apps-syd-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMacViews: Implement NSResponder deletion action messages.
This CL adds support for the following deletion editing commands:
-deleteToBeginningOfLine:
-deleteToBeginningOfParagraph:
-deleteToEndOfParagraph:
-deleteToEndOfLine:
Tests are added for the same as well as for the commands deleteWordForward:
and deleteWordBackward:.
BUG=586985
Committed: https://crrev.com/e0d60fbe0459693abc293f4093ed802513855b29
Cr-Commit-Position: refs/heads/master@{#390892}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Rebase. #Patch Set 3 : Address review comments. #
Total comments: 12
Patch Set 4 : Address nits. #Patch Set 5 : Change "Depends on Patchset". #
Depends on Patchset: Messages
Total messages: 16 (8 generated)
Description was changed from ========== -- Format Second commit Initial commit. BUG= ========== to ========== MacViews: Implement NSResponder deletion action messages. This CL adds support for the following deletion editing commands: -deleteToBeginningOfLine: -deleteToBeginningOfParagraph: -deleteToEndOfParagraph: -deleteToEndOfLine: This CL also fixes the behavior of deleteWordForward: and deleteWordBackward: when an active selection is in place. BUG=586985,605492 ==========
karandeepb@chromium.org changed reviewers: + tapted@chromium.org
PTAL Trent. Thanks. https://codereview.chromium.org/1912993002/diff/1/ui/views/cocoa/bridged_cont... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/1912993002/diff/1/ui/views/cocoa/bridged_cont... ui/views/cocoa/bridged_content_view.mm:697: int command = [self selectedRange].length ? IDS_DELETE_FORWARD For other platforms, this case is handled in Textfield::GetCommandForKeyEvent(..). But since on Linux and Mac, we may bypass GetCommandForKeyEvent(), it causes crbug.com/605492. Another approach can be to have GetCommandForKeyEvent just map the event to appropriate command, without considering selection etc. Then ExecuteCommand can take care of the selection case. This should also fix this bug on Linux.
https://codereview.chromium.org/1912993002/diff/1/ui/views/cocoa/bridged_cont... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/1912993002/diff/1/ui/views/cocoa/bridged_cont... ui/views/cocoa/bridged_content_view.mm:697: int command = [self selectedRange].length ? IDS_DELETE_FORWARD On 2016/04/22 02:02:46, karandeepb wrote: > For other platforms, this case is handled in > Textfield::GetCommandForKeyEvent(..). But since on Linux and Mac, we may bypass > GetCommandForKeyEvent(), it causes crbug.com/605492. Another approach can be to > have GetCommandForKeyEvent just map the event to appropriate command, without > considering selection etc. Then ExecuteCommand can take care of the selection > case. This should also fix this bug on Linux. Yeah I like the idea of moving this into ExecuteCommand - it would simplify things here, and in GetCommandForKeyEvent, as well as fix this bug on linux. It should be a separate CL though. https://codereview.chromium.org/1912993002/diff/1/ui/views/cocoa/bridged_cont... ui/views/cocoa/bridged_content_view.mm:736: // Since we currently only support single line textfields, forward the action Generally it's bad to repeat comments - just like code - since updating multiple places when things get out of date is annoying. I'd go for something like // views::Textfields are single-line only, map Paragraph and Document commands to Line. "Document" you say? Yep! we should include ..Beginning/End of Document as well. We should also include the moveTo* and the moveTo..AndModifySelection commands with this CL, since they all fall under the bag of "must map them to line" https://codereview.chromium.org/1912993002/diff/1/ui/views/cocoa/bridged_nati... File ui/views/cocoa/bridged_native_widget_unittest.mm (right): https://codereview.chromium.org/1912993002/diff/1/ui/views/cocoa/bridged_nati... ui/views/cocoa/bridged_native_widget_unittest.mm:652: TEST_F(BridgedNativeWidgetTest, TextInput_DeleteWordForward) { I think we should try to make these cross-platform -- moving them in to textfield_unittests.cc, and figure out where the gaps are (i.e. on what platforms). GTK has a rich set of keybindings, which Chrome probably ignores a lot of. See e.g. GtkMovementStep at https://developer.gnome.org/gtk3/stable/gtk3-Standard-Enumerations.html#GtkMo... -- (i.e. Chrome on Linux should have a lot of these behaviours as well). It's also nice where possible to show that adding all this MacViews stuff is improving things for all platforms. i.e. not just tacking on a lot of mac-specific functionality, but bringing a mac-like level of UI "excellence" to all platforms.
Description was changed from ========== MacViews: Implement NSResponder deletion action messages. This CL adds support for the following deletion editing commands: -deleteToBeginningOfLine: -deleteToBeginningOfParagraph: -deleteToEndOfParagraph: -deleteToEndOfLine: This CL also fixes the behavior of deleteWordForward: and deleteWordBackward: when an active selection is in place. BUG=586985,605492 ========== to ========== MacViews: Implement NSResponder deletion action messages. This CL adds support for the following deletion editing commands: -deleteToBeginningOfLine: -deleteToBeginningOfParagraph: -deleteToEndOfParagraph: -deleteToEndOfLine: Tests are added for the same as well as for the commands deleteWordForward: and deleteWordBackward:. BUG=586985 ==========
Patchset #4 (id:60001) has been deleted
PTAL Trent. Have made this dependent on CL https://codereview.chromium.org/1923793002 https://codereview.chromium.org/1912993002/diff/1/ui/views/cocoa/bridged_cont... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/1912993002/diff/1/ui/views/cocoa/bridged_cont... ui/views/cocoa/bridged_content_view.mm:697: int command = [self selectedRange].length ? IDS_DELETE_FORWARD On 2016/04/26 05:46:54, tapted wrote: > On 2016/04/22 02:02:46, karandeepb wrote: > > For other platforms, this case is handled in > > Textfield::GetCommandForKeyEvent(..). But since on Linux and Mac, we may > bypass > > GetCommandForKeyEvent(), it causes crbug.com/605492. Another approach can be > to > > have GetCommandForKeyEvent just map the event to appropriate command, without > > considering selection etc. Then ExecuteCommand can take care of the selection > > case. This should also fix this bug on Linux. > > Yeah I like the idea of moving this into ExecuteCommand - it would simplify > things here, and in GetCommandForKeyEvent, as well as fix this bug on linux. It > should be a separate CL though. Done. CL here - https://codereview.chromium.org/1923793002 https://codereview.chromium.org/1912993002/diff/1/ui/views/cocoa/bridged_cont... ui/views/cocoa/bridged_content_view.mm:736: // Since we currently only support single line textfields, forward the action On 2016/04/26 05:46:54, tapted wrote: > Generally it's bad to repeat comments - just like code - since updating multiple > places when things get out of date is annoying. > > I'd go for something like > > // views::Textfields are single-line only, map Paragraph and Document commands > to Line. > > "Document" you say? Yep! we should include ..Beginning/End of Document as well. > > We should also include the moveTo* and the moveTo..AndModifySelection commands > with this CL, since they all fall under the bag of "must map them to line" Done. If it's ok, will add the movement commands in a separate CL, since there are a lot of them. https://codereview.chromium.org/1912993002/diff/1/ui/views/cocoa/bridged_nati... File ui/views/cocoa/bridged_native_widget_unittest.mm (right): https://codereview.chromium.org/1912993002/diff/1/ui/views/cocoa/bridged_nati... ui/views/cocoa/bridged_native_widget_unittest.mm:652: TEST_F(BridgedNativeWidgetTest, TextInput_DeleteWordForward) { On 2016/04/26 05:46:55, tapted wrote: > I think we should try to make these cross-platform -- moving them in to > textfield_unittests.cc, and figure out where the gaps are (i.e. on what > platforms). > > GTK has a rich set of keybindings, which Chrome probably ignores a lot of. See > e.g. GtkMovementStep at > https://developer.gnome.org/gtk3/stable/gtk3-Standard-Enumerations.html#GtkMo... > -- (i.e. Chrome on Linux should have a lot of these behaviours as well). It's > also nice where possible to show that adding all this MacViews stuff is > improving things for all platforms. i.e. not just tacking on a lot of > mac-specific functionality, but bringing a mac-like level of UI "excellence" to > all platforms. Added a cross-platform test here-https://codereview.chromium.org/1923793002. However I think we should also add tests to BridgedNativeWidgetTest. Thanks for the link to the GTK keybindings. Will keep it in mind, when I add new commands.
lgtm % nits https://codereview.chromium.org/1912993002/diff/1/ui/views/cocoa/bridged_nati... File ui/views/cocoa/bridged_native_widget_unittest.mm (right): https://codereview.chromium.org/1912993002/diff/1/ui/views/cocoa/bridged_nati... ui/views/cocoa/bridged_native_widget_unittest.mm:652: TEST_F(BridgedNativeWidgetTest, TextInput_DeleteWordForward) { On 2016/04/29 01:52:48, karandeepb wrote: > On 2016/04/26 05:46:55, tapted wrote: > > I think we should try to make these cross-platform -- moving them in to > > textfield_unittests.cc, and figure out where the gaps are (i.e. on what > > platforms). > > > > GTK has a rich set of keybindings, which Chrome probably ignores a lot of. See > > e.g. GtkMovementStep at > > > https://developer.gnome.org/gtk3/stable/gtk3-Standard-Enumerations.html#GtkMo... > > -- (i.e. Chrome on Linux should have a lot of these behaviours as well). It's > > also nice where possible to show that adding all this MacViews stuff is > > improving things for all platforms. i.e. not just tacking on a lot of > > mac-specific functionality, but bringing a mac-like level of UI "excellence" > to > > all platforms. > > Added a cross-platform test here-https://codereview.chromium.org/1923793002. > However I think we should also add tests to BridgedNativeWidgetTest. That's not really testing the beginning/end of line stuff. But since it's actually hard to find precisely how delete beginning/end stuff should be mapped to keys on other platforms, this approach seems reasonable, since it's only got a clear keymapping on Mac. (on Linux I think delete-to-end might be Ctrl+U but that's also "view source") https://codereview.chromium.org/1912993002/diff/40001/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/1912993002/diff/40001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:681: // views::Textfields are single-line only, map Paragraph and Document commands Move this up to the section comment in line 569, since it will apply to the movement stuff above line 681 as well https://codereview.chromium.org/1912993002/diff/40001/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_native_widget_unittest.mm (right): https://codereview.chromium.org/1912993002/diff/40001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget_unittest.mm:301: void BridgedNativeWidgetTest::TestDeleteBeginning(SEL sel) { nit: add a comment describing what |sel| can be (i.e. DeleteToBeginningOfSomething) https://codereview.chromium.org/1912993002/diff/40001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget_unittest.mm:327: void BridgedNativeWidgetTest::TestDeleteEnd(SEL sel) { nit: add comment https://codereview.chromium.org/1912993002/diff/40001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget_unittest.mm:652: TEST_F(BridgedNativeWidgetTest, TextInput_DeleteWordForward) { nit: add broader test comment https://codereview.chromium.org/1912993002/diff/40001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget_unittest.mm:676: TEST_F(BridgedNativeWidgetTest, TextInput_DeleteWordBackward) { nit: add comment https://codereview.chromium.org/1912993002/diff/40001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget_unittest.mm:702: TEST_F(BridgedNativeWidgetTest, TextInput_DeleteToBeginningOfLine) { a comment here can probably cover all four tests below
https://codereview.chromium.org/1912993002/diff/40001/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_content_view.mm (right): https://codereview.chromium.org/1912993002/diff/40001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_content_view.mm:681: // views::Textfields are single-line only, map Paragraph and Document commands On 2016/04/29 04:15:39, tapted wrote: > Move this up to the section comment in line 569, since it will apply to the > movement stuff above line 681 as well Done. https://codereview.chromium.org/1912993002/diff/40001/ui/views/cocoa/bridged_... File ui/views/cocoa/bridged_native_widget_unittest.mm (right): https://codereview.chromium.org/1912993002/diff/40001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget_unittest.mm:301: void BridgedNativeWidgetTest::TestDeleteBeginning(SEL sel) { On 2016/04/29 04:15:39, tapted wrote: > nit: add a comment describing what |sel| can be (i.e. > DeleteToBeginningOfSomething) Done. https://codereview.chromium.org/1912993002/diff/40001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget_unittest.mm:327: void BridgedNativeWidgetTest::TestDeleteEnd(SEL sel) { On 2016/04/29 04:15:39, tapted wrote: > nit: add comment Done. https://codereview.chromium.org/1912993002/diff/40001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget_unittest.mm:652: TEST_F(BridgedNativeWidgetTest, TextInput_DeleteWordForward) { On 2016/04/29 04:15:39, tapted wrote: > nit: add broader test comment Done. https://codereview.chromium.org/1912993002/diff/40001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget_unittest.mm:676: TEST_F(BridgedNativeWidgetTest, TextInput_DeleteWordBackward) { On 2016/04/29 04:15:39, tapted wrote: > nit: add comment Done. https://codereview.chromium.org/1912993002/diff/40001/ui/views/cocoa/bridged_... ui/views/cocoa/bridged_native_widget_unittest.mm:702: TEST_F(BridgedNativeWidgetTest, TextInput_DeleteToBeginningOfLine) { On 2016/04/29 04:15:39, tapted wrote: > a comment here can probably cover all four tests below Done.
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 Link to the patchset: https://codereview.chromium.org/1912993002/#ps100001 (title: "Change "Depends on Patchset".")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1912993002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1912993002/100001
Message was sent while issue was closed.
Description was changed from ========== MacViews: Implement NSResponder deletion action messages. This CL adds support for the following deletion editing commands: -deleteToBeginningOfLine: -deleteToBeginningOfParagraph: -deleteToEndOfParagraph: -deleteToEndOfLine: Tests are added for the same as well as for the commands deleteWordForward: and deleteWordBackward:. BUG=586985 ========== to ========== MacViews: Implement NSResponder deletion action messages. This CL adds support for the following deletion editing commands: -deleteToBeginningOfLine: -deleteToBeginningOfParagraph: -deleteToEndOfParagraph: -deleteToEndOfLine: Tests are added for the same as well as for the commands deleteWordForward: and deleteWordBackward:. BUG=586985 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== MacViews: Implement NSResponder deletion action messages. This CL adds support for the following deletion editing commands: -deleteToBeginningOfLine: -deleteToBeginningOfParagraph: -deleteToEndOfParagraph: -deleteToEndOfLine: Tests are added for the same as well as for the commands deleteWordForward: and deleteWordBackward:. BUG=586985 ========== to ========== MacViews: Implement NSResponder deletion action messages. This CL adds support for the following deletion editing commands: -deleteToBeginningOfLine: -deleteToBeginningOfParagraph: -deleteToEndOfParagraph: -deleteToEndOfLine: Tests are added for the same as well as for the commands deleteWordForward: and deleteWordBackward:. BUG=586985 Committed: https://crrev.com/e0d60fbe0459693abc293f4093ed802513855b29 Cr-Commit-Position: refs/heads/master@{#390892} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/e0d60fbe0459693abc293f4093ed802513855b29 Cr-Commit-Position: refs/heads/master@{#390892} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
