Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1011943002:
Mac: Clicking an omnibox decoration should not highlight the omnibox. (Closed)
|
Created:
5 years, 9 months ago by erikchen Modified:
5 years, 7 months ago Reviewers:
Scott Hess - ex-Googler CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMac: Clicking an omnibox decoration should not highlight the omnibox.
Prior to this CL, the AutocompleteTextField becomes first responder if the user
left-clicks a decoration. This is a bug that this CL fixes. This also fixes a
bug where ZeroSuggest did not disappear when the user drags the origin chip.
BUG=356887
Committed: https://crrev.com/9b14239654a3600c2a56dc214d277f63d7eb12cc
Cr-Commit-Position: refs/heads/master@{#329781}
Patch Set 1 #Patch Set 2 : Fix unit test. #
Total comments: 11
Patch Set 3 : Comments from shess. #Patch Set 4 : #Patch Set 5 : #
Total comments: 12
Patch Set 6 : Comments from shess. #
Total comments: 4
Patch Set 7 : Comments from shess, round three. #
Total comments: 1
Patch Set 8 : Revert a change to the unit test AutocompleteTextFieldTest.LeftDecorationMouseDown. #
Messages
Total messages: 23 (8 generated)
erikchen@chromium.org changed reviewers: + shess@google.com
shess: Please review. https://codereview.chromium.org/1011943002/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_unittest.mm (left): https://codereview.chromium.org/1011943002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_unittest.mm:796: TEST_F(AutocompleteTextFieldObserverTest, ButtonDecorationFocus) { This test was written for Omnitheatre, and is no longer relevant. I repurposed the test into a regression test for this CL. Note that the previous CL checked that clicking a decoration (which auto-hides) causes the omnibox to become focused. The auto-hide behavior is no longer relevant, and of course the omnibox should not become focused.
shess@chromium.org changed reviewers: + shess@chromium.org - shess@google.com
LGTM, with random nits which may-or-may-not be relevant any longer. LocationBarDecoration::AcceptsMousePress() implies that there exist decorations which don't accept left mouse presses, and instead should be handled as a click in the background area would be handled (either placing the cursor leftmost or rightmost, or allowing click-then-drag into the text area to start selecting). I don't know if that is relevant to any decorations, or if it was a "Seems like the right idea" type of thing. https://codereview.chromium.org/1011943002/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_unittest.mm (left): https://codereview.chromium.org/1011943002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_unittest.mm:796: TEST_F(AutocompleteTextFieldObserverTest, ButtonDecorationFocus) { On 2015/05/07 19:33:51, erikchen wrote: > Note that the previous CL checked that clicking a decoration (which auto-hides) > causes the omnibox to become focused. The auto-hide behavior is no longer > relevant, and of course the omnibox should not become focused. Would it make sense to further revise this to not be tied to button decorations? The change seems to apply to any decoration. https://codereview.chromium.org/1011943002/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_unittest.mm (right): https://codereview.chromium.org/1011943002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_unittest.mm:819: [[field_ window] firstResponder]) isDescendantOf:field_]); At this point is there any first responder at all for the window? As-is this feels like it could be FALSE for many reasons, the most specific one might be preferable. https://codereview.chromium.org/1011943002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_unittest.mm:838: inFrame:[field_ bounds]])); Alignment. https://codereview.chromium.org/1011943002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_unittest.mm:840: [[field_ window] firstResponder]) isDescendantOf:field_]); Here also.
Hmm, another thing that I've never gotten the rage to resolve is handling clicks "between" decorations. I think there's like one pixel in there which isn't live. With this kind of change it might make sense to not have the clicks between decorations fall through to the background. Likewise for clicks in the omnibox border area above and below and outside of the decorations. Just something to think on.
On 2015/05/07 20:11:41, Scott Hess wrote: > Hmm, another thing that I've never gotten the rage to resolve is handling clicks > "between" decorations. I think there's like one pixel in there which isn't > live. With this kind of change it might make sense to not have the clicks > between decorations fall through to the background. Likewise for clicks in the > omnibox border area above and below and outside of the decorations. Just > something to think on. Filed a pri-3 bug against myself. https://code.google.com/p/chromium/issues/detail?id=485828
shess: PTAL https://codereview.chromium.org/1011943002/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_unittest.mm (left): https://codereview.chromium.org/1011943002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_unittest.mm:796: TEST_F(AutocompleteTextFieldObserverTest, ButtonDecorationFocus) { On 2015/05/07 20:07:42, Scott Hess wrote: > On 2015/05/07 19:33:51, erikchen wrote: > > Note that the previous CL checked that clicking a decoration (which > auto-hides) > > causes the omnibox to become focused. The auto-hide behavior is no longer > > relevant, and of course the omnibox should not become focused. > > Would it make sense to further revise this to not be tied to button decorations? > The change seems to apply to any decoration. Good suggestion. I updated the test, and discovered a bug in my original CL. I've made 3 changes: 1. Moved logic from becomeFirstResponder -> acceptsFirstResponder. This has the same behavior, but makes more sense semantically. 2. I added a check for AcceptsMousePress(). This was a bug in the logic of the original CL. 3. I added some logic to make the text field resign first responder when a decoration is selected. This is technically not a mistake in the previous CL, but it is a bug fix in the same vein. https://codereview.chromium.org/1011943002/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_unittest.mm (right): https://codereview.chromium.org/1011943002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_unittest.mm:819: [[field_ window] firstResponder]) isDescendantOf:field_]); On 2015/05/07 20:07:42, Scott Hess wrote: > At this point is there any first responder at all for the window? As-is this > feels like it could be FALSE for many reasons, the most specific one might be > preferable. The first responder is CocoaTestHelperWindow. I'd prefer to not gain specificity at the cost of adding a dependency between this class and the default first responder of the test window. https://codereview.chromium.org/1011943002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_unittest.mm:838: inFrame:[field_ bounds]])); On 2015/05/07 20:07:42, Scott Hess wrote: > Alignment. I clang-format-ed the entire CL. https://codereview.chromium.org/1011943002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_unittest.mm:840: [[field_ window] firstResponder]) isDescendantOf:field_]); On 2015/05/07 20:07:42, Scott Hess wrote: > Here also. ditto.
https://codereview.chromium.org/1011943002/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_unittest.mm (right): https://codereview.chromium.org/1011943002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_unittest.mm:838: inFrame:[field_ bounds]])); On 2015/05/08 01:51:46, erikchen wrote: > On 2015/05/07 20:07:42, Scott Hess wrote: > > Alignment. > > I clang-format-ed the entire CL. And the offending code is no longer present anyhow... if the previous version was run through clang-format, then there's a bug somewhere. https://codereview.chromium.org/1011943002/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field.mm (right): https://codereview.chromium.org/1011943002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/location_bar/autocomplete_text_field.mm:155: // first responder. This doesn't seem right for the case where the field has first responder before the click was made. https://codereview.chromium.org/1011943002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/location_bar/autocomplete_text_field.mm:409: if (decoration and decoration->AcceptsMousePress()) gah! "and"? https://codereview.chromium.org/1011943002/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_unittest.mm (right): https://codereview.chromium.org/1011943002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_unittest.mm:55: }; Given the current code, this class seems like it is no longer relevant, and MockDecoration could be used instead. https://codereview.chromium.org/1011943002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_unittest.mm:837: // matter, as they auxiliary to the behavior being tested. probably "they are auxiliary" https://codereview.chromium.org/1011943002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_unittest.mm:856: isDescendantOf:field_]); This ties in with the comment in the implementation, I think this should verify that the interactive_decoration click does not change the first responder, whoever it was beforehand.
shess: PTAL https://codereview.chromium.org/1011943002/diff/20001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_unittest.mm (right): https://codereview.chromium.org/1011943002/diff/20001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_unittest.mm:838: inFrame:[field_ bounds]])); On 2015/05/08 19:38:28, Scott Hess wrote: > On 2015/05/08 01:51:46, erikchen wrote: > > On 2015/05/07 20:07:42, Scott Hess wrote: > > > Alignment. > > > > I clang-format-ed the entire CL. > > And the offending code is no longer present anyhow... if the previous version > was run through clang-format, then there's a bug somewhere. The previous code was not. I should have been more clear - I clang-format-ed the updated CL. https://codereview.chromium.org/1011943002/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field.mm (right): https://codereview.chromium.org/1011943002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/location_bar/autocomplete_text_field.mm:155: // first responder. On 2015/05/08 19:38:28, Scott Hess wrote: > This doesn't seem right for the case where the field has first responder before > the click was made. Why not? That's the behavior on Windows. Try this out on the latest Canary: 1. Highlight all the text in the omnibox. 2. Click the star button. Note that the omnibox text is still selected (in addition to the text in the new popup). Attempting to type does not change the omnibox text. That's very confusing. https://codereview.chromium.org/1011943002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/location_bar/autocomplete_text_field.mm:409: if (decoration and decoration->AcceptsMousePress()) On 2015/05/08 19:38:28, Scott Hess wrote: > gah! "and"? I don't even. Fixed. https://codereview.chromium.org/1011943002/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_unittest.mm (right): https://codereview.chromium.org/1011943002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_unittest.mm:55: }; On 2015/05/08 19:38:28, Scott Hess wrote: > Given the current code, this class seems like it is no longer relevant, and > MockDecoration could be used instead. Yup, removed. https://codereview.chromium.org/1011943002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_unittest.mm:837: // matter, as they auxiliary to the behavior being tested. On 2015/05/08 19:38:28, Scott Hess wrote: > probably "they are auxiliary" Done. https://codereview.chromium.org/1011943002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_unittest.mm:856: isDescendantOf:field_]); On 2015/05/08 19:38:28, Scott Hess wrote: > This ties in with the comment in the implementation, I think this should verify > that the interactive_decoration click does not change the first responder, > whoever it was beforehand. See my response to the earlier comment.
https://codereview.chromium.org/1011943002/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field.mm (right): https://codereview.chromium.org/1011943002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/location_bar/autocomplete_text_field.mm:155: // first responder. On 2015/05/08 20:02:20, erikchen wrote: > On 2015/05/08 19:38:28, Scott Hess wrote: > > This doesn't seem right for the case where the field has first responder > before > > the click was made. > > Why not? That's the behavior on Windows. Try this out on the latest Canary: > > 1. Highlight all the text in the omnibox. > 2. Click the star button. > > Note that the omnibox text is still selected (in addition to the text in the new > popup). Attempting to type does not change the omnibox text. That's very > confusing. You didn't mention platform. On OSX with the latest canary, if I visit a website, then select-all in the omnibox, then click star, focus is in the Name field of the star popup. When I click Done, focus is back in the omnibox and typing replaces the text. AFAICT it's the same on Windows Canary. Looks to me like the omnibox text is still selected, but not focus-selected (or whatever they call it), it's gray instead of blue. Like a selection in a non-editable field. So I don't think the existing code is doing anything special to accomplish this, it's just how Cocoa works when key and main windows differ. https://codereview.chromium.org/1011943002/diff/100001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_unittest.mm (right): https://codereview.chromium.org/1011943002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_unittest.mm:607: // The selection should not have changed. Without first responder the selection is ill-defined. And not being tested here. Sorry I missed that previous pass. https://codereview.chromium.org/1011943002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_unittest.mm:824: // as they auxiliary. I think it still needs an "are" in there somewhere.
Patchset #7 (id:120001) has been deleted
Patchset #7 (id:140001) has been deleted
shess: PTAL https://codereview.chromium.org/1011943002/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field.mm (right): https://codereview.chromium.org/1011943002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/location_bar/autocomplete_text_field.mm:155: // first responder. On 2015/05/08 20:34:28, Scott Hess wrote: > On 2015/05/08 20:02:20, erikchen wrote: > > On 2015/05/08 19:38:28, Scott Hess wrote: > > > This doesn't seem right for the case where the field has first responder > > before > > > the click was made. > > > > Why not? That's the behavior on Windows. Try this out on the latest Canary: > > > > 1. Highlight all the text in the omnibox. > > 2. Click the star button. > > > > Note that the omnibox text is still selected (in addition to the text in the > new > > popup). Attempting to type does not change the omnibox text. That's very > > confusing. > > You didn't mention platform. On OSX with the latest canary, if I visit a > website, then select-all in the omnibox, then click star, focus is in the Name > field of the star popup. When I click Done, focus is back in the omnibox and > typing replaces the text. AFAICT it's the same on Windows Canary. > > Looks to me like the omnibox text is still selected, but not focus-selected (or > whatever they call it), it's gray instead of blue. Like a selection in a > non-editable field. So I don't think the existing code is doing anything > special to accomplish this, it's just how Cocoa works when key and main windows > differ. Hm, you're right. I've removed this logic. https://codereview.chromium.org/1011943002/diff/100001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_unittest.mm (right): https://codereview.chromium.org/1011943002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_unittest.mm:607: // The selection should not have changed. On 2015/05/08 20:34:29, Scott Hess wrote: > Without first responder the selection is ill-defined. And not being tested > here. Sorry I missed that previous pass. Fixed. https://codereview.chromium.org/1011943002/diff/100001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_unittest.mm:824: // as they auxiliary. On 2015/05/08 20:34:29, Scott Hess wrote: > I think it still needs an "are" in there somewhere. Yup, fixed.
thanks, lgtm. https://codereview.chromium.org/1011943002/diff/150001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_unittest.mm (right): https://codereview.chromium.org/1011943002/diff/150001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_unittest.mm:835: NSResponder* firstResponder = [[field_ window] firstResponder]; This approach makes sense.
The CQ bit was checked by erikchen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1011943002/150001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by erikchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from shess@chromium.org Link to the patchset: https://codereview.chromium.org/1011943002/#ps170001 (title: "Revert a change to the unit test AutocompleteTextFieldTest.LeftDecorationMouseDown.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1011943002/170001
Message was sent while issue was closed.
Committed patchset #8 (id:170001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/9b14239654a3600c2a56dc214d277f63d7eb12cc Cr-Commit-Position: refs/heads/master@{#329781} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
