Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
165853002:
[OSX, OmniTheatre] URL hiding for Origin Chip. (Closed)
|
Created:
6 years, 10 months ago by groby-ooo-7-16 Modified:
6 years, 10 months ago CC:
chromium-reviews, James Su, Greg Billock Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[OSX, OmniTheatre] URL hiding for Origin Chip.
Hide/Show the URL based on various triggers if origin chip is enabled.
Specifically:
* Hide URL if location bar loses focus.
* Hide URL if location bar is focused via mouse click.
* Hide URL on navigation.
* Display URL after click on origin chip. (NOT IMPLEMENTED YET)
* Display URL after Cmd-L.
Also, show a hint text if no text is currently entered.
BUG=338563
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=251826
Patch Set 1 #
Total comments: 6
Patch Set 2 : Review nit fixes. #Patch Set 3 : Rebase to HEAD #Patch Set 4 : Remove file that depends on different CL #
Messages
Total messages: 24 (0 generated)
shess: Please look at this from Cocoa/Omnibar OWNERs perspective jdonelly: For general OmniTheatre goodness.
shess: to elaborate on the TODO I need to disable the origin chip whenever the control has focus. The problem is that if I hide it on firstResponder, the origin chip vanishes before mouseDown events can be checked against it, so I can't detect clicks on the chip. Debating various increasingly absurd strategies to handle that. Best idea so far: in becomeFirstResponder, check [NSApp currentEvent]. If it's an left-mouse-down, postpone the OnSetFocus call until mouseDown is completed. Slightly worried that that will affect other decorations though - so if there's a better way to handle this, please LMK.
https://codereview.chromium.org/165853002/diff/1/chrome/browser/ui/cocoa/omni... File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right): https://codereview.chromium.org/165853002/diff/1/chrome/browser/ui/cocoa/omni... chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:208: if (chrome::ShouldDisplayOriginChip() || Any reason not to check just for V2 here like you do below?
On 2014/02/14 03:53:17, groby wrote: > shess: to elaborate on the TODO > > I need to disable the origin chip whenever the control has focus. The problem is > that if I hide it on firstResponder, the origin chip vanishes before mouseDown > events can be checked against it, so I can't detect clicks on the chip. > > Debating various increasingly absurd strategies to handle that. Best idea so > far: in becomeFirstResponder, check [NSApp currentEvent]. If it's an > left-mouse-down, postpone the OnSetFocus call until mouseDown is completed. > > Slightly worried that that will affect other decorations though - so if there's > a better way to handle this, please LMK. LGTM as a first step. Are you planning to do the rest in this CL or a follow-on?
Will add the V2 stuff. I'll probably add a bit more in this CL, depending on when shess reviews - but overall, at least for the hide-on-click, this is almost workable, so landing would be good.
On 2014/02/14 03:53:17, groby wrote: > shess: to elaborate on the TODO > > I need to disable the origin chip whenever the control has focus. The problem is > that if I hide it on firstResponder, the origin chip vanishes before mouseDown > events can be checked against it, so I can't detect clicks on the chip. > > Debating various increasingly absurd strategies to handle that. Best idea so > far: in becomeFirstResponder, check [NSApp currentEvent]. If it's an > left-mouse-down, postpone the OnSetFocus call until mouseDown is completed. > > Slightly worried that that will affect other decorations though - so if there's > a better way to handle this, please LMK. I worry about deferring focus stuff until something else, just because I have no idea who expects what at what point. You could override -hitTest: and record whether it was in the chip or not, then check that record in -mouseDown: Unfortunately, -hitTest: doesn't pass the event, so you'd have to assume the current event is the one you want. I'd be somewhat surprised if anything other than left or right mouse causes -hitTest:. Or -becomeFirstResponder could record the chip's rect for -mouseDown: to manually test later.
lgtm as-is, let me know if you decide to bite off the chip-click case in this CL. https://codereview.chromium.org/165853002/diff/1/chrome/browser/ui/cocoa/omni... File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right): https://codereview.chromium.org/165853002/diff/1/chrome/browser/ui/cocoa/omni... chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:794: // to ShowURL(). If the Omnibox acheived focus by other means, the calls to achieved. https://codereview.chromium.org/165853002/diff/1/chrome/browser/ui/cocoa/omni... chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:796: // required (a call to OnChanged would be sufficient) but do no harm. My general take on things is that calls which scale with user actions (tab, click, command-L) just have to be fast enough, but have a different correctness/speed tradeoff. I think it's fine to over-call here, since the controller and model don't necessarily bake in cocoa assumptions.
No, I'll bite that off later. FWIW, here's what I'm planning: * Have a separate SetFocus - let's call it DidSetFocus, for now. It will only contain the OmniTheatre stuff, as to not impact the current behavior. * In becomeFirstResponder, only call DidSetFocus if [NSApp currentEvent] is not a mouse event. * in mouseDown, call DidSetFocus. After all, we only get one mouseDown in NSTextField, the activating one. The rest goes to the field editor - acceptFirstResponder ensures that is true even when the field decoration is clicked. It's disgustingly hacky, and I think the proper long-term answer is to make the button a control, not a cell.
https://codereview.chromium.org/165853002/diff/1/chrome/browser/ui/cocoa/omni... File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right): https://codereview.chromium.org/165853002/diff/1/chrome/browser/ui/cocoa/omni... chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:208: if (chrome::ShouldDisplayOriginChip() || On 2014/02/14 14:57:14, Justin Donnelly wrote: > Any reason not to check just for V2 here like you do below? That was based on Views, which does the same thing. I'll take it that was an oversight there, so removed here. https://codereview.chromium.org/165853002/diff/1/chrome/browser/ui/cocoa/omni... chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:794: // to ShowURL(). If the Omnibox acheived focus by other means, the calls to On 2014/02/15 00:55:57, shess wrote: > achieved. Done. https://codereview.chromium.org/165853002/diff/1/chrome/browser/ui/cocoa/omni... chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:796: // required (a call to OnChanged would be sufficient) but do no harm. On 2014/02/15 00:55:57, shess wrote: > My general take on things is that calls which scale with user actions (tab, > click, command-L) just have to be fast enough, but have a different > correctness/speed tradeoff. I think it's fine to over-call here, since the > controller and model don't necessarily bake in cocoa assumptions. This is taken verbatim from Views, so there's certainly no Cocoaness :) I'm not sure these are even needed there. ShowURL already does call UpdatePermanentText, which leaves RevertAll. I'll follow up w/ jdonelly outside the CL
The CQ bit was checked by groby@chromium.org
On 2014/02/15 01:04:53, groby wrote: > No, I'll bite that off later. FWIW, here's what I'm planning: > > * Have a separate SetFocus - let's call it DidSetFocus, for now. It will only > contain the OmniTheatre stuff, as to not impact the current behavior. > * In becomeFirstResponder, only call DidSetFocus if [NSApp currentEvent] is not > a mouse event. > * in mouseDown, call DidSetFocus. After all, we only get one mouseDown in > NSTextField, the activating one. The rest goes to the field editor - > acceptFirstResponder ensures that is true even when the field decoration is > clicked. > > It's disgustingly hacky, and I think the proper long-term answer is to make the > button a control, not a cell. From another CL, I'm intuiting that clicks on an icon within the chip do a thing which seems like it wouldn't want to focus the field. Which would break my approach. I'm not sure what is a good long-term solution. I think we're past the point where arrangement of existing controls provides quite what we need, since it's pretty specific. For instance, a button+field thing will need to be able to do sizing tricks. I think a reasonable option once the look is nailed down is to just make the chip part of the containing field rather than part of the cell, and then you can be more subtle. Or it might have to layer something above to catch the image-click independent of the focus majick. I wonder what -validateProposedFirstResponder:forEvent: gives me. Maybe it just ends up with us more thoroughly intercepting the first-responder stuff and manually invoking the field editor on our terms rather than letting Cocoa handle it. That's super fun stuff, with potential shenanigans on every OS upgrade :-).
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/groby@chromium.org/165853002/110001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm
Hunk #1 FAILED at 523.
Hunk #2 succeeded at 546 (offset -7 lines).
1 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm.rej
Patch: chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm
Index: chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm
diff --git a/chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm
b/chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm
index
09a0e997f808346dc4142bc51e278066408a9758..5261fe7a1a23db057cd40db8aec72896a071a7a1
100644
--- a/chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm
+++ b/chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm
@@ -523,7 +523,6 @@ void LocationBarViewMac::Update(const WebContents* contents)
{
RefreshContentSettingsDecorations();
UpdateMicSearchDecorationVisibility();
UpdateGeneratedCreditCardView();
- UpdateOriginChipDecoration();
if (contents)
omnibox_view_->OnTabChanged(contents);
else
@@ -553,6 +552,7 @@ void LocationBarViewMac::OnChanged() {
(resource_id == IDR_OMNIBOX_SEARCH) ?
IDR_OMNIBOX_SEARCH_BUTTON_LOUPE : IDR_OMNIBOX_SEARCH_BUTTON_ARROW);
+ UpdateOriginChipDecoration();
Layout();
InstantService* instant_service =
On 2014/02/15 01:28:19, shess wrote:
> On 2014/02/15 01:04:53, groby wrote:
> > No, I'll bite that off later. FWIW, here's what I'm planning:
> >
> > * Have a separate SetFocus - let's call it DidSetFocus, for now. It will
only
> > contain the OmniTheatre stuff, as to not impact the current behavior.
> > * In becomeFirstResponder, only call DidSetFocus if [NSApp currentEvent] is
> not
> > a mouse event.
> > * in mouseDown, call DidSetFocus. After all, we only get one mouseDown in
> > NSTextField, the activating one. The rest goes to the field editor -
> > acceptFirstResponder ensures that is true even when the field decoration is
> > clicked.
> >
> > It's disgustingly hacky, and I think the proper long-term answer is to make
> the
> > button a control, not a cell.
>
> From another CL, I'm intuiting that clicks on an icon within the chip do a
thing
> which seems like it wouldn't want to focus the field. Which would break my
> approach.
Clicks on the button display a URL, do "Select All", and focus the control - so
that's still good :)
> I'm not sure what is a good long-term solution. I think we're past the point
> where arrangement of existing controls provides quite what we need, since it's
> pretty specific. For instance, a button+field thing will need to be able to
do
> sizing tricks.
I don't see this as a particular problem if we keep them contained in one view
which stays fixed size. Am I missing something?
> I think a reasonable option once the look is nailed down is to
> just make the chip part of the containing field rather than part of the cell,
> and then you can be more subtle. Or it might have to layer something above to
> catch the image-click independent of the focus majick.
I think it goes beyond that - since the OriginChip is clickable, it should
probably be able to participate in the KeyView cycle, for accessibility reasons.
> I wonder what -validateProposedFirstResponder:forEvent: gives me.
Mostly, a dependency on 10.7 :) I'm also confuzzled since the documentation
suggests calling it from mouseDown:, which happens _after_ becomeFirstResponder.
I think the docs are mistaken.
> Maybe it just ends up with us more thoroughly intercepting the first-responder
> stuff and manually invoking the field editor on our terms rather than letting
> Cocoa handle it. That's super fun stuff, with potential shenanigans on every
OS
> upgrade :-).
Please, ${DEITY}, no! :)
These really are two different, if linked, controls. Unless unrelated AppKit
madness forces us down a different route, I'd prefer two NSControls to do the
job of two NSControls :)
The CQ bit was checked by groby@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/groby@chromium.org/165853002/260001
On 2014/02/15 01:49:46, groby wrote: > These really are two different, if linked, controls. Unless unrelated AppKit > madness forces us down a different route, I'd prefer two NSControls to do the > job of two NSControls :) It's one control in the sense that relative sizes change and it has a single visual look. So you can't just drop two adjacent controls and be done. But having the field have a view on top of it to do the button-like things isn't much different from swapping in a field editor. After that, it's just rearranging the deck chairs. For instance, it's probably not _just_ a button, because it probably needs to intercept mousedown and rearrange the view hierarchy, rather than doing it in the target/action. Easy enough to work around, but whether it's a button or a cell or some kind of hit-testing and first-responder magic is mostly going to be a matter of which thing works without being needlessly obscure, probably.
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
The CQ bit was checked by groby@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/groby@chromium.org/165853002/260001
On 2014/02/15 01:49:46, groby wrote:
> On 2014/02/15 01:28:19, shess wrote:
> > On 2014/02/15 01:04:53, groby wrote:
> > > No, I'll bite that off later. FWIW, here's what I'm planning:
> > >
> > > * Have a separate SetFocus - let's call it DidSetFocus, for now. It will
> only
> > > contain the OmniTheatre stuff, as to not impact the current behavior.
> > > * In becomeFirstResponder, only call DidSetFocus if [NSApp currentEvent]
is
> > not
> > > a mouse event.
> > > * in mouseDown, call DidSetFocus. After all, we only get one mouseDown in
> > > NSTextField, the activating one. The rest goes to the field editor -
> > > acceptFirstResponder ensures that is true even when the field decoration
is
> > > clicked.
> > >
> > > It's disgustingly hacky, and I think the proper long-term answer is to
make
> > the
> > > button a control, not a cell.
> >
> > From another CL, I'm intuiting that clicks on an icon within the chip do a
> thing
> > which seems like it wouldn't want to focus the field. Which would break my
> > approach.
>
> Clicks on the button display a URL, do "Select All", and focus the control -
so
> that's still good :)
The only exception to that is that clicking the _icon_ within the origin chip
will reveal the permissions bubble (instead of showing the URL). That's not
implemented yet though, but it shouldn't focus the omnibox.
>
> > I'm not sure what is a good long-term solution. I think we're past the
point
> > where arrangement of existing controls provides quite what we need, since
it's
> > pretty specific. For instance, a button+field thing will need to be able to
> do
> > sizing tricks.
>
> I don't see this as a particular problem if we keep them contained in one view
> which stays fixed size. Am I missing something?
>
> > I think a reasonable option once the look is nailed down is to
> > just make the chip part of the containing field rather than part of the
cell,
> > and then you can be more subtle. Or it might have to layer something above
to
> > catch the image-click independent of the focus majick.
>
> I think it goes beyond that - since the OriginChip is clickable, it should
> probably be able to participate in the KeyView cycle, for accessibility
reasons.
>
> > I wonder what -validateProposedFirstResponder:forEvent: gives me.
>
> Mostly, a dependency on 10.7 :) I'm also confuzzled since the documentation
> suggests calling it from mouseDown:, which happens _after_
becomeFirstResponder.
> I think the docs are mistaken.
>
> > Maybe it just ends up with us more thoroughly intercepting the
first-responder
> > stuff and manually invoking the field editor on our terms rather than
letting
> > Cocoa handle it. That's super fun stuff, with potential shenanigans on
every
> OS
> > upgrade :-).
>
> Please, ${DEITY}, no! :)
>
> These really are two different, if linked, controls. Unless unrelated AppKit
> madness forces us down a different route, I'd prefer two NSControls to do the
> job of two NSControls :)
Message was sent while issue was closed.
Change committed as 251826 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
