Issue 413433002: Refactor BubbleDelegateView::use_focuseless()

Issue 413433002: Refactor BubbleDelegateView::use_focuseless() (Closed)

Created:
6 years, 5 months ago by vasilii

Modified:
6 years, 4 months ago

Reviewers:
msw, sky, Dan Beam

CC:
chromium-reviews, msw+watch_chromium.org, sadrul, mkwst+watchlist_chromium.org, benquan, tfarina, Dane Wallinga, dyu1, estade+watch_chromium.org, rouslan+autofillwatch_chromium.org, kalyank, gcasto+watchlist_chromium.org, alicet1, Ilya Sherman, ben+ash_chromium.org

Base URL:
svn://svn.chromium.org/chrome/trunk/src

Project:
chromium

Visibility:
Public.

More Reviews

Description

Refactor BubbleDelegateView::use_focuseless(). It's confusing because it prevents the bubble activation. As a side effect on Windows the bubble doesn't get LBUTTON_DOWN messages. The reason for that is in HWNDMessageHandler::OnMouseActivate(). It asks the bubble if it can be activated. It answers "no" due to implementation in BubbleDelegateView::CanActivate(). Windows gets MA_NOACTIVATEANDEAT and MouseDown event isn't dispatched. This is definitely unexpected. The bubbles which indeed always inactive should use set_can_activate(false) at construction time. The bubbles which can be active but created without focus by default should use ShowInactive() instead. BUG=392734 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285682

Patch Set 1 #

Patch Set 2 : cros compilation #

Total comments: 12

Patch Set 3 : Fixed typo #

Total comments: 6

Patch Set 4 : Don't reopen the bubble without good reason #

Total comments: 2

Patch Set 5 : nit + rebase #

Created: 6 years, 4 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+29 lines, -46 lines)			Patch
M	ash/ime/candidate_window_view.cc	View		1 chunk	+1 line, -1 line	0 comments	Download
M	ash/ime/infolist_window.cc	View		1 chunk	+1 line, -1 line	0 comments	Download
M	ash/ime/mode_indicator_view.cc	View		1 chunk	+1 line, -1 line	0 comments	Download
M	ash/popup_message.cc	View		1 chunk	+1 line, -1 line	0 comments	Download
M	ash/shelf/overflow_bubble_view.cc	View		1 chunk	+1 line, -1 line	0 comments	Download
M	ash/shelf/shelf_tooltip_manager.cc	View		1 chunk	+1 line, -1 line	0 comments	Download
M	ash/system/chromeos/network/network_state_list_detailed_view.cc	View	1 2 3 4	2 chunks	+1 line, -3 lines	0 comments	Download
M	ash/wm/immersive_fullscreen_controller_unittest.cc	View	1 2 3 4	1 chunk	+2 lines, -2 lines	0 comments	Download
M	chrome/browser/ui/views/autofill/info_bubble.cc	View		1 chunk	+1 line, -1 line	0 comments	Download
M	chrome/browser/ui/views/autofill/tooltip_icon.cc	View		1 chunk	+1 line, -1 line	0 comments	Download
M	chrome/browser/ui/views/location_bar/zoom_bubble_view.cc	View	1 2 3 4	5 chunks	+8 lines, -15 lines	0 comments	Download
M	chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc	View	1 2 3 4	2 chunks	+1 line, -9 lines	0 comments	Download
M	chrome/browser/ui/views/validation_message_bubble_delegate.cc	View		1 chunk	+1 line, -1 line	0 comments	Download
M	ui/views/bubble/bubble_delegate.h	View		2 chunks	+4 lines, -4 lines	0 comments	Download
M	ui/views/bubble/bubble_delegate.cc	View		3 chunks	+3 lines, -3 lines	0 comments	Download
M	ui/views/touchui/touch_editing_menu.cc	View		1 chunk	+1 line, -1 line	0 comments	Download

Messages

Total messages: 23 (0 generated)

Expand Messages | Collapse Messages

msw

+dbeam for ZoomBubbleView, +vasilii for ManagePasswordsBubbleView. https://codereview.chromium.org/413433002/diff/40001/chrome/browser/ui/views/location_bar/zoom_bubble_view.cc File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (left): https://codereview.chromium.org/413433002/diff/40001/chrome/browser/ui/views/location_bar/zoom_bubble_view.cc#oldcode142 chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:142: set_use_focusless(auto_close); You've removed ...

6 years, 5 months ago (2014-07-23 18:57:00 UTC) #2

Dan Beam

> +dbeam for ZoomBubbleView, +vasilii for ManagePasswordsBubbleView. i'd guess vasilii@ agrees with the author's code ...

6 years, 5 months ago (2014-07-23 23:03:34 UTC) #3

> +dbeam for ZoomBubbleView, +vasilii for ManagePasswordsBubbleView.

i'd guess vasilii@ agrees with the author's code ;)

https://codereview.chromium.org/413433002/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (left):

https://codereview.chromium.org/413433002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:142:
set_use_focusless(auto_close);
On 2014/07/23 18:57:00, msw wrote:
> You've removed all the calls to set_use_focusless / set_can_activate for this
> bubble, which is likely problematic. I think the zoom bubble needs to
> ShowInactive on the [Ctrl]-[+] accelerator (to avoid altering focus), but it
> needs to take activation when the user clicks that timed bubble, and when the
> user invokes the magnifying glass page action icon by click or keyboard.

correct, ctrl+/- should not move focus away from the web contents

https://codereview.chromium.org/413433002/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right):

https://codereview.chromium.org/413433002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:78: // If the bubble is
already showing but its |auto_close_| value is not equal
On 2014/07/23 18:57:00, msw wrote:
> I wonder if this is still true/necessary?

this only happens on ctrl+ and a click on the zoom icon in 2s (while the
auto-close button is still open).  i think it's possible you could just change
|zoom_bubble_->auto_close_| to false and cancel the close timer but it's a
fairly rare case and the user would have no indication that the click or tap
worked.  also, focus might differ.

https://codereview.chromium.org/413433002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:104: if
(zoom_bubble_->can_activate())
On 2014/07/23 18:57:00, msw wrote:
> nit: I think this can just call Show and it'll respect can_activate.
> (that's if can_activate is set properly, see related comments)

the last time i checked, can_active() { return false; } + Show() still hijacked
focus on some platforms (or maybe it was some cases?).  i can't remember exactly
why.

vasilii

There was some confusion about ZoomBubbleView. It can be shown with or without focus. However, ...

6 years, 4 months ago (2014-07-24 16:32:14 UTC) #4

There was some confusion about ZoomBubbleView. It can be shown with or without
focus. However, it always can become active. Otherwise, user won't be able to
click the button. Thus, I removed the calls to set_can_activate().

https://codereview.chromium.org/413433002/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (left):

https://codereview.chromium.org/413433002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:142:
set_use_focusless(auto_close);
On 2014/07/23 23:03:34, Dan Beam wrote:
> On 2014/07/23 18:57:00, msw wrote:
> > You've removed all the calls to set_use_focusless / set_can_activate for
this
> > bubble, which is likely problematic. I think the zoom bubble needs to
> > ShowInactive on the [Ctrl]-[+] accelerator (to avoid altering focus), but it
> > needs to take activation when the user clicks that timed bubble, and when
the
> > user invokes the magnifying glass page action icon by click or keyboard.
> 
> correct, ctrl+/- should not move focus away from the web contents

ctrl+ doesn't remove focus, I checked manually.

https://codereview.chromium.org/413433002/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right):

https://codereview.chromium.org/413433002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:78: // If the bubble is
already showing but its |auto_close_| value is not equal
On 2014/07/23 23:03:34, Dan Beam wrote:
> On 2014/07/23 18:57:00, msw wrote:
> > I wonder if this is still true/necessary?
> 
> this only happens on ctrl+ and a click on the zoom icon in 2s (while the
> auto-close button is still open).  i think it's possible you could just change
> |zoom_bubble_->auto_close_| to false and cancel the close timer but it's a
> fairly rare case and the user would have no indication that the click or tap
> worked.  also, focus might differ.

I reproduced this case.

https://codereview.chromium.org/413433002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:104: if
(zoom_bubble_->can_activate())
On 2014/07/23 23:03:34, Dan Beam wrote:
> On 2014/07/23 18:57:00, msw wrote:
> > nit: I think this can just call Show and it'll respect can_activate.
> > (that's if can_activate is set properly, see related comments)
> 
> the last time i checked, can_active() { return false; } + Show() still
hijacked
> focus on some platforms (or maybe it was some cases?).  i can't remember
exactly
> why.

This line is my typo for sure.
Dan, I touched this problem in the description.

msw

https://codereview.chromium.org/413433002/diff/40001/chrome/browser/ui/views/location_bar/zoom_bubble_view.cc File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right): https://codereview.chromium.org/413433002/diff/40001/chrome/browser/ui/views/location_bar/zoom_bubble_view.cc#newcode78 chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:78: // If the bubble is already showing but its ...

6 years, 4 months ago (2014-07-24 16:52:28 UTC) #5

Dan Beam

On 2014/07/24 16:52:28, msw wrote: > https://codereview.chromium.org/413433002/diff/40001/chrome/browser/ui/views/location_bar/zoom_bubble_view.cc > File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right): > > https://codereview.chromium.org/413433002/diff/40001/chrome/browser/ui/views/location_bar/zoom_bubble_view.cc#newcode78 > ...

6 years, 4 months ago (2014-07-24 17:32:02 UTC) #6

msw

On 2014/07/24 17:32:02, Dan Beam wrote: > On 2014/07/24 16:52:28, msw wrote: > > > ...

6 years, 4 months ago (2014-07-24 18:02:54 UTC) #7

On 2014/07/24 17:32:02, Dan Beam wrote:
> On 2014/07/24 16:52:28, msw wrote:
> >
>
https://codereview.chromium.org/413433002/diff/40001/chrome/browser/ui/views/...
> > File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right):
> > 
> >
>
https://codereview.chromium.org/413433002/diff/40001/chrome/browser/ui/views/...
> > chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:78: // If the
bubble
> is
> > already showing but its |auto_close_| value is not equal
> > On 2014/07/24 16:32:13, vasilii wrote:
> > > On 2014/07/23 23:03:34, Dan Beam wrote:
> > > > On 2014/07/23 18:57:00, msw wrote:
> > > > > I wonder if this is still true/necessary?
> > > > 
> > > > this only happens on ctrl+ and a click on the zoom icon in 2s (while the
> > > > auto-close button is still open).  i think it's possible you could just
> > change
> > > > |zoom_bubble_->auto_close_| to false and cancel the close timer but it's
a
> > > > fairly rare case and the user would have no indication that the click or
> tap
> > > > worked.  also, focus might differ.
> > > 
> > > I reproduced this case.
> > 
> > Why do we need to recreate the bubble on click after [Ctrl]+[+]? Now that
the
> > original bubble can activate, can we just call zoom_bubble_->StopTimer()?
> > 
> >
>
https://codereview.chromium.org/413433002/diff/60001/chrome/browser/ui/views/...
> > File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right):
> > 
> >
>
https://codereview.chromium.org/413433002/diff/60001/chrome/browser/ui/views/...
> > chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:265: // If an
> > auto-closing bubble was tapped, show a non-auto-closing bubble in
> > This is the culprit, can this just call StopTimer like OnMouseEntered?
> 
> in both these cases I think we still need to show some indication the click
did
> something... (IMO)

What sort of indication do you want? In this case you're clicking the contents
of a bubble that's already open, right? The user is likely either clicking a
focusable control in the bubble (which should gain and indicate focus), or
clicking a non-focusable label or blank area of the bubble, which should give
activation to the bubble and stop the timer to keep the bubble open, not close
and re-open the bubble, right? Am I confused?

Dan Beam

On 2014/07/24 18:02:54, msw wrote: > On 2014/07/24 17:32:02, Dan Beam wrote: > > On ...

6 years, 4 months ago (2014-07-24 18:26:52 UTC) #8

On 2014/07/24 18:02:54, msw wrote:
> On 2014/07/24 17:32:02, Dan Beam wrote:
> > On 2014/07/24 16:52:28, msw wrote:
> > >
> >
>
https://codereview.chromium.org/413433002/diff/40001/chrome/browser/ui/views/...
> > > File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/413433002/diff/40001/chrome/browser/ui/views/...
> > > chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:78: // If the
> bubble
> > is
> > > already showing but its |auto_close_| value is not equal
> > > On 2014/07/24 16:32:13, vasilii wrote:
> > > > On 2014/07/23 23:03:34, Dan Beam wrote:
> > > > > On 2014/07/23 18:57:00, msw wrote:
> > > > > > I wonder if this is still true/necessary?
> > > > > 
> > > > > this only happens on ctrl+ and a click on the zoom icon in 2s (while
the
> > > > > auto-close button is still open).  i think it's possible you could
just
> > > change
> > > > > |zoom_bubble_->auto_close_| to false and cancel the close timer but
it's
> a
> > > > > fairly rare case and the user would have no indication that the click
or
> > tap
> > > > > worked.  also, focus might differ.
> > > > 
> > > > I reproduced this case.
> > > 
> > > Why do we need to recreate the bubble on click after [Ctrl]+[+]? Now that
> the
> > > original bubble can activate, can we just call zoom_bubble_->StopTimer()?
> > > 
> > >
> >
>
https://codereview.chromium.org/413433002/diff/60001/chrome/browser/ui/views/...
> > > File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/413433002/diff/60001/chrome/browser/ui/views/...
> > > chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:265: // If an
> > > auto-closing bubble was tapped, show a non-auto-closing bubble in
> > > This is the culprit, can this just call StopTimer like OnMouseEntered?
> > 
> > in both these cases I think we still need to show some indication the click
> did
> > something... (IMO)
> 
> What sort of indication do you want? In this case you're clicking the contents
> of a bubble that's already open, right? The user is likely either clicking a
> focusable control in the bubble (which should gain and indicate focus), or
> clicking a non-focusable label or blank area of the bubble, which should give
> activation to the bubble and stop the timer to keep the bubble open, not close
> and re-open the bubble, right? Am I confused?

yes, i meant clicking the anchor

msw

On 2014/07/24 18:26:52, Dan Beam wrote: > On 2014/07/24 18:02:54, msw wrote: > > On ...

6 years, 4 months ago (2014-07-24 18:42:19 UTC) #9

On 2014/07/24 18:26:52, Dan Beam wrote:
> On 2014/07/24 18:02:54, msw wrote:
> > On 2014/07/24 17:32:02, Dan Beam wrote:
> > > On 2014/07/24 16:52:28, msw wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/413433002/diff/40001/chrome/browser/ui/views/...
> > > > File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/413433002/diff/40001/chrome/browser/ui/views/...
> > > > chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:78: // If the
> > bubble
> > > is
> > > > already showing but its |auto_close_| value is not equal
> > > > On 2014/07/24 16:32:13, vasilii wrote:
> > > > > On 2014/07/23 23:03:34, Dan Beam wrote:
> > > > > > On 2014/07/23 18:57:00, msw wrote:
> > > > > > > I wonder if this is still true/necessary?
> > > > > > 
> > > > > > this only happens on ctrl+ and a click on the zoom icon in 2s (while
> the
> > > > > > auto-close button is still open).  i think it's possible you could
> just
> > > > change
> > > > > > |zoom_bubble_->auto_close_| to false and cancel the close timer but
> it's
> > a
> > > > > > fairly rare case and the user would have no indication that the
click
> or
> > > tap
> > > > > > worked.  also, focus might differ.
> > > > > 
> > > > > I reproduced this case.
> > > > 
> > > > Why do we need to recreate the bubble on click after [Ctrl]+[+]? Now
that
> > the
> > > > original bubble can activate, can we just call
zoom_bubble_->StopTimer()?
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/413433002/diff/60001/chrome/browser/ui/views/...
> > > > File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/413433002/diff/60001/chrome/browser/ui/views/...
> > > > chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:265: // If an
> > > > auto-closing bubble was tapped, show a non-auto-closing bubble in
> > > > This is the culprit, can this just call StopTimer like OnMouseEntered?
> > > 
> > > in both these cases I think we still need to show some indication the
click
> > did
> > > something... (IMO)
> > 
> > What sort of indication do you want? In this case you're clicking the
contents
> > of a bubble that's already open, right? The user is likely either clicking a
> > focusable control in the bubble (which should gain and indicate focus), or
> > clicking a non-focusable label or blank area of the bubble, which should
give
> > activation to the bubble and stop the timer to keep the bubble open, not
close
> > and re-open the bubble, right? Am I confused?
> 
> yes, i meant clicking the anchor

That's not relevant to ZoomBubbleView::OnGestureEvent calling Show to recreate
the bubble, is it? I don't even think we'd need to keep the recreation logic in
Show to support quickly closing and reopening the bubble (I think you want) on
(1) [Ctrl]+[+] (2) click zoom icon. I get that same behavior when I click the
icon twice in a row (where the recreate logic shouldn't apply).

Dan Beam

On 2014/07/24 18:42:19, msw wrote: > On 2014/07/24 18:26:52, Dan Beam wrote: > > On ...

6 years, 4 months ago (2014-07-24 19:20:54 UTC) #10

On 2014/07/24 18:42:19, msw wrote:
> On 2014/07/24 18:26:52, Dan Beam wrote:
> > On 2014/07/24 18:02:54, msw wrote:
> > > On 2014/07/24 17:32:02, Dan Beam wrote:
> > > > On 2014/07/24 16:52:28, msw wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/413433002/diff/40001/chrome/browser/ui/views/...
> > > > > File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/413433002/diff/40001/chrome/browser/ui/views/...
> > > > > chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:78: // If the
> > > bubble
> > > > is
> > > > > already showing but its |auto_close_| value is not equal
> > > > > On 2014/07/24 16:32:13, vasilii wrote:
> > > > > > On 2014/07/23 23:03:34, Dan Beam wrote:
> > > > > > > On 2014/07/23 18:57:00, msw wrote:
> > > > > > > > I wonder if this is still true/necessary?
> > > > > > > 
> > > > > > > this only happens on ctrl+ and a click on the zoom icon in 2s
(while
> > the
> > > > > > > auto-close button is still open).  i think it's possible you could
> > just
> > > > > change
> > > > > > > |zoom_bubble_->auto_close_| to false and cancel the close timer
but
> > it's
> > > a
> > > > > > > fairly rare case and the user would have no indication that the
> click
> > or
> > > > tap
> > > > > > > worked.  also, focus might differ.
> > > > > > 
> > > > > > I reproduced this case.
> > > > > 
> > > > > Why do we need to recreate the bubble on click after [Ctrl]+[+]? Now
> that
> > > the
> > > > > original bubble can activate, can we just call
> zoom_bubble_->StopTimer()?
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/413433002/diff/60001/chrome/browser/ui/views/...
> > > > > File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/413433002/diff/60001/chrome/browser/ui/views/...
> > > > > chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:265: // If an
> > > > > auto-closing bubble was tapped, show a non-auto-closing bubble in
> > > > > This is the culprit, can this just call StopTimer like OnMouseEntered?
> > > > 
> > > > in both these cases I think we still need to show some indication the
> click
> > > did
> > > > something... (IMO)
> > > 
> > > What sort of indication do you want? In this case you're clicking the
> contents
> > > of a bubble that's already open, right? The user is likely either clicking
a
> > > focusable control in the bubble (which should gain and indicate focus), or
> > > clicking a non-focusable label or blank area of the bubble, which should
> give
> > > activation to the bubble and stop the timer to keep the bubble open, not
> close
> > > and re-open the bubble, right? Am I confused?
> > 
> > yes, i meant clicking the anchor
> 
> That's not relevant to ZoomBubbleView::OnGestureEvent calling Show to recreate
> the bubble, is it?

s/clicking/tapping/

> I don't even think we'd need to keep the recreation logic in
> Show to support quickly closing and reopening the bubble (I think you want) on

what does "(I think you want)" mean here?

here's an ASCII art flow chart of the [desired] behavior:

                     ctrl-/+
                       |                      anchor showing
                     show                            |
      ------------>focusless                 anchor click/tap
     /            /    |    \                        |
    /      mousein   anchor  1s                   activate
   /            /  click/tap  \                     / \
   |       allow       |       close      deactivate   reset
   |     activation  close**                        \ /
   |         / ^  \                                close
   \ mouseout  |   reset
    \  /   \   |    \
    .2s    mousein   close

s == seconds

** desired behavior, not currently implemented

> (1) [Ctrl]+[+] (2) click zoom icon. I get that same behavior when I click the
> icon twice in a row (where the recreate logic shouldn't apply).

clicking the icon with a bubble open should probably just close the bubble
instead (like the bookmark bubble)

msw

On 2014/07/24 19:20:54, Dan Beam wrote: > On 2014/07/24 18:42:19, msw wrote: > > I ...

6 years, 4 months ago (2014-07-24 19:46:12 UTC) #11

On 2014/07/24 19:20:54, Dan Beam wrote:
> On 2014/07/24 18:42:19, msw wrote:
> > I don't even think we'd need to keep the recreation logic in
> > Show to support quickly closing and reopening the bubble (I think you want)
on
> 
> what does "(I think you want)" mean here?

In comment #6 you said "in both these cases I think we still need to show some
indication the click did
something... (IMO)", can you explain what you mean? I thought you wanted some
visual indication of the bubble taking activation when the user clicks/taps the
bubble after pressing [Ctrl]+[+].

> here's an ASCII art flow chart of the [desired] behavior:
> 
>                      ctrl-/+
>                        |                      anchor showing
>                      show                            |
>       ------------>focusless                 anchor click/tap
>      /            /    |    \                        |
>     /      mousein   anchor  1s                   activate
>    /            /  click/tap  \                     / \
>    |       allow       |       close      deactivate   reset
>    |     activation  close**                        \ /
>    |         / ^  \                                close
>    \ mouseout  |   reset
>     \  /   \   |    \
>     .2s    mousein   close
> 
> s == seconds
> 
> ** desired behavior, not currently implemented
> 
> > (1) [Ctrl]+[+] (2) click zoom icon. I get that same behavior when I click
the
> > icon twice in a row (where the recreate logic shouldn't apply).

Nice diagram, but *why* does activation requires a bubble reset? Why do we have
to *recreate the bubble* when the users presses [Ctrl]+[+] and then taps the
bubble? Afaict, ZoomBubbleView::OnGestureEvent should call StopTimer() and
BubbleDelegateView::OnGestureEvent() to let the existing bubble activate, not
call Show to recreate the bubble with a different auto_close setting.

There may be other cases where Show legitimately needs to recreate the existing
bubble, so perhaps that code can stay without the condition and comment about
auto_close.

> clicking the icon with a bubble open should probably just close the bubble
> instead (like the bookmark bubble)

Yes, that's Issue 366092 and I'll happily review patches for that.

Dan Beam

On 2014/07/24 19:46:12, msw wrote: > On 2014/07/24 19:20:54, Dan Beam wrote: > > On ...

6 years, 4 months ago (2014-07-24 21:45:11 UTC) #12

On 2014/07/24 19:46:12, msw wrote:
> On 2014/07/24 19:20:54, Dan Beam wrote:
> > On 2014/07/24 18:42:19, msw wrote:
> > > I don't even think we'd need to keep the recreation logic in
> > > Show to support quickly closing and reopening the bubble (I think you
want)
> on
> > 
> > what does "(I think you want)" mean here?
> 
> In comment #6 you said "in both these cases I think we still need to show some
> indication the click did
> something... (IMO)", can you explain what you mean? I thought you wanted some
> visual indication of the bubble taking activation when the user clicks/taps
the
> bubble after pressing [Ctrl]+[+].

i meant when the user taps/clicks the *anchor* while the bubble is showing. but
we're just gonna close instead.

re: user clicks a showing bubble (which I didn't address until now and in that
diagram):
there's really no control-like things in the bubble other than the reset button
(and +/- on mac?), and clicking on text doesn't usually do much... so i'd vote
we do nothing special for mousedown in the bubble (there had to be a mousein
already anyways).

when a user *taps* inside the bubble, maybe it's the equivalent of a mousein and
we should cancel the hide timer.

> 
> > here's an ASCII art flow chart of the [desired] behavior:
> > 
> >                      ctrl-/+
> >                        |                      anchor showing
> >                      show                            |
> >       ------------>focusless                 anchor click/tap
> >      /            /    |    \                        |
> >     /      mousein   anchor  1s                   activate
> >    /            /  click/tap  \                     / \
> >    |       allow       |       close      deactivate   reset
> >    |     activation  close**                        \ /
> >    |         / ^  \                                close
> >    \ mouseout  |   reset
> >     \  /   \   |    \
> >     .2s    mousein   close
> > 
> > s == seconds
> > 
> > ** desired behavior, not currently implemented
> > 
> > > (1) [Ctrl]+[+] (2) click zoom icon. I get that same behavior when I click
> the
> > > icon twice in a row (where the recreate logic shouldn't apply).
> 
> Nice diagram, but *why* does activation requires a bubble reset? Why do we
have
> to *recreate the bubble* when the users presses [Ctrl]+[+] and then taps the
> bubble? Afaict, ZoomBubbleView::OnGestureEvent should call StopTimer() and
> BubbleDelegateView::OnGestureEvent() to let the existing bubble activate, not
> call Show to recreate the bubble with a different auto_close setting.
> 
> There may be other cases where Show legitimately needs to recreate the
existing
> bubble, so perhaps that code can stay without the condition and comment about
> auto_close.
> 
> > clicking the icon with a bubble open should probably just close the bubble
> > instead (like the bookmark bubble)
> 
> Yes, that's Issue 366092 and I'll happily review patches for that.

https://codereview.chromium.org/420533002

Dan Beam

On 2014/07/24 21:45:11, Dan Beam wrote: > On 2014/07/24 19:46:12, msw wrote: > > On ...

6 years, 4 months ago (2014-07-24 21:46:34 UTC) #13

On 2014/07/24 21:45:11, Dan Beam wrote:
> On 2014/07/24 19:46:12, msw wrote:
> > On 2014/07/24 19:20:54, Dan Beam wrote:
> > > On 2014/07/24 18:42:19, msw wrote:
> > > > I don't even think we'd need to keep the recreation logic in
> > > > Show to support quickly closing and reopening the bubble (I think you
> want)
> > on
> > > 
> > > what does "(I think you want)" mean here?
> > 
> > In comment #6 you said "in both these cases I think we still need to show
some
> > indication the click did
> > something... (IMO)", can you explain what you mean? I thought you wanted
some
> > visual indication of the bubble taking activation when the user clicks/taps
> the
> > bubble after pressing [Ctrl]+[+].
> 
> i meant when the user taps/clicks the *anchor* while the bubble is showing.
but
> we're just gonna close instead.
> 
> re: user clicks a showing bubble (which I didn't address until now and in that
> diagram):
> there's really no control-like things in the bubble other than the reset
button
> (and +/- on mac?), and clicking on text doesn't usually do much... so i'd vote
> we do nothing special for mousedown in the bubble (there had to be a mousein
> already anyways).

of course I meant ignore mousedowns __not on the "Reset" button__

> 
> when a user *taps* inside the bubble, maybe it's the equivalent of a mousein
and
> we should cancel the hide timer.
> 
> > 
> > > here's an ASCII art flow chart of the [desired] behavior:
> > > 
> > >                      ctrl-/+
> > >                        |                      anchor showing
> > >                      show                            |
> > >       ------------>focusless                 anchor click/tap
> > >      /            /    |    \                        |
> > >     /      mousein   anchor  1s                   activate
> > >    /            /  click/tap  \                     / \
> > >    |       allow       |       close      deactivate   reset
> > >    |     activation  close**                        \ /
> > >    |         / ^  \                                close
> > >    \ mouseout  |   reset
> > >     \  /   \   |    \
> > >     .2s    mousein   close
> > > 
> > > s == seconds
> > > 
> > > ** desired behavior, not currently implemented
> > > 
> > > > (1) [Ctrl]+[+] (2) click zoom icon. I get that same behavior when I
click
> > the
> > > > icon twice in a row (where the recreate logic shouldn't apply).
> > 
> > Nice diagram, but *why* does activation requires a bubble reset? Why do we
> have
> > to *recreate the bubble* when the users presses [Ctrl]+[+] and then taps the
> > bubble? Afaict, ZoomBubbleView::OnGestureEvent should call StopTimer() and
> > BubbleDelegateView::OnGestureEvent() to let the existing bubble activate,
not
> > call Show to recreate the bubble with a different auto_close setting.
> > 
> > There may be other cases where Show legitimately needs to recreate the
> existing
> > bubble, so perhaps that code can stay without the condition and comment
about
> > auto_close.
> > 
> > > clicking the icon with a bubble open should probably just close the bubble
> > > instead (like the bookmark bubble)
> > 
> > Yes, that's Issue 366092 and I'll happily review patches for that.
> 
> https://codereview.chromium.org/420533002

msw

https://codereview.chromium.org/413433002/diff/60001/chrome/browser/ui/views/location_bar/zoom_bubble_view.cc File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right): https://codereview.chromium.org/413433002/diff/60001/chrome/browser/ui/views/location_bar/zoom_bubble_view.cc#newcode66 chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:66: // If the bubble is already showing in this ...

6 years, 4 months ago (2014-07-24 22:16:33 UTC) #14

Dan Beam

https://codereview.chromium.org/413433002/diff/40001/chrome/browser/ui/views/location_bar/zoom_bubble_view.cc File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (left): https://codereview.chromium.org/413433002/diff/40001/chrome/browser/ui/views/location_bar/zoom_bubble_view.cc#oldcode142 chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:142: set_use_focusless(auto_close); On 2014/07/24 16:32:13, vasilii wrote: > On 2014/07/23 ...

6 years, 4 months ago (2014-07-24 23:24:51 UTC) #15

vasilii

https://codereview.chromium.org/413433002/diff/40001/chrome/browser/ui/views/location_bar/zoom_bubble_view.cc File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (left): https://codereview.chromium.org/413433002/diff/40001/chrome/browser/ui/views/location_bar/zoom_bubble_view.cc#oldcode142 chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:142: set_use_focusless(auto_close); On 2014/07/24 23:24:51, Dan Beam wrote: > On ...

6 years, 4 months ago (2014-07-25 15:27:43 UTC) #16

https://codereview.chromium.org/413433002/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (left):

https://codereview.chromium.org/413433002/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:142:
set_use_focusless(auto_close);
On 2014/07/24 23:24:51, Dan Beam wrote:
> On 2014/07/24 16:32:13, vasilii wrote:
> > On 2014/07/23 23:03:34, Dan Beam wrote:
> > > On 2014/07/23 18:57:00, msw wrote:
> > > > You've removed all the calls to set_use_focusless / set_can_activate for
> > this
> > > > bubble, which is likely problematic. I think the zoom bubble needs to
> > > > ShowInactive on the [Ctrl]-[+] accelerator (to avoid altering focus),
but
> it
> > > > needs to take activation when the user clicks that timed bubble, and
when
> > the
> > > > user invokes the magnifying glass page action icon by click or keyboard.
> > > 
> > > correct, ctrl+/- should not move focus away from the web contents
> > 
> > ctrl+ doesn't remove focus, I checked manually.
> 
> did you check on linux chromeos, linux aura, and windows?

Yeah.

https://codereview.chromium.org/413433002/diff/60001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right):

https://codereview.chromium.org/413433002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:66: // If the bubble is
already showing in this window, its |auto_close_| value
On 2014/07/24 22:16:33, msw wrote:
> If you update ZoomBubbleView::OnGestureEvent as I've suggested, you should be
> able to remove the comment about |auto_close| here, the condition on line 72,
> and rewrite the comment about |auto_close| on line 78 (to something about
> recreating the bubble for whatever other cases need that).

Done.

https://codereview.chromium.org/413433002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:265: // If an
auto-closing bubble was tapped, show a non-auto-closing bubble in
On 2014/07/24 23:24:51, Dan Beam wrote:
> On 2014/07/24 22:16:33, msw wrote:
> > On 2014/07/24 16:52:28, msw wrote:
> > > This is the culprit, can this just call StopTimer like OnMouseEntered?
> > 
> > Sounds like Dan tentatively agrees with my suggestion here.
> 
> yes

Done.

msw

lgtm with a nit; thanks for bearing with our discussion. https://codereview.chromium.org/413433002/diff/80001/chrome/browser/ui/views/location_bar/zoom_bubble_view.cc File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right): https://codereview.chromium.org/413433002/diff/80001/chrome/browser/ui/views/location_bar/zoom_bubble_view.cc#newcode76 ...

6 years, 4 months ago (2014-07-25 15:38:38 UTC) #17

vasilii

No problem. Scott, Dan, please review. https://codereview.chromium.org/413433002/diff/80001/chrome/browser/ui/views/location_bar/zoom_bubble_view.cc File chrome/browser/ui/views/location_bar/zoom_bubble_view.cc (right): https://codereview.chromium.org/413433002/diff/80001/chrome/browser/ui/views/location_bar/zoom_bubble_view.cc#newcode76 chrome/browser/ui/views/location_bar/zoom_bubble_view.cc:76: // If the ...

6 years, 4 months ago (2014-07-25 15:48:20 UTC) #18

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vasilii@chromium.org/413433002/100001

6 years, 4 months ago (2014-07-25 20:27:14 UTC) #22

Message was sent while issue was closed.

Change committed as 285682

Expand Messages | Collapse Messages