Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
962453002:
Update permission bubble anchor when omnibar is hidden (Closed)
|
Created:
5 years, 9 months ago by hcarmona Modified:
5 years, 9 months ago CC:
chromium-reviews, msw+watch_chromium.org, alicet1, tfarina, ainslie, Finnur, Mike Wittman, Greg Billock, Dan Beam Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUpdate permission bubble anchor when omnibar is hidden
The permission bubble is created once. It now chooses an appropriate
anchor if the omnibar is not visible.
The bubble's anchor is updated when fullscreen is toggled.
Screenshots of change:
http://imgur.com/a/PmEQz
BUG=440403, 440401
Committed: https://crrev.com/32a02419d38bf802329767c8bee375cc446e6756
Cr-Commit-Position: refs/heads/master@{#321864}
Patch Set 1 : Do Not Commit #Patch Set 2 : Fix issue #
Total comments: 19
Patch Set 3 : Attempted Trybot Fix #Patch Set 4 : Update comment and don't CHECK(anchor_view) #
Total comments: 2
Patch Set 5 : Center open bubble #Patch Set 6 : Update Fullscreen Parent #
Total comments: 14
Patch Set 7 : Apply Feedback #
Total comments: 4
Patch Set 8 : Move anchor logic to PermissionBubbleViewViews #
Total comments: 20
Patch Set 9 : Apply Feedback #
Total comments: 10
Patch Set 10 : Apply Feedback #Patch Set 11 : Attempt to fix trybots #Patch Set 12 : Revert to UpdateAnchorPosition in BrowserView #
Total comments: 2
Patch Set 13 : Add comment #
Messages
Total messages: 33 (3 generated)
hcarmona@chromium.org changed reviewers: + felt@chromium.org, groby@chromium.org, msw@chromium.org
This change will make the permission bubble centered across the top of the window if there is no omnibar.
https://codereview.chromium.org/962453002/diff/20001/ui/views/bubble/bubble_d... File ui/views/bubble/bubble_delegate.cc (right): https://codereview.chromium.org/962453002/diff/20001/ui/views/bubble/bubble_d... ui/views/bubble/bubble_delegate.cc:99: views::View* anchor = bubble_delegate->GetAnchorView(); Trybots hated this check. I'm removing it. Are there cases where we should not be showing the anchor view or do we have a lot of potential bugs?
I might be seriously misunderstanding how the PermissionBubble happens, so lots of questions. https://codereview.chromium.org/962453002/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/962453002/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/frame/browser_view.cc:543: permission_bubble_view_.reset(new PermissionBubbleViewViews( I'm still not sure why we need to pre-create |permission_bubble_view_|. I suppose that's a different question, but I'd like to figure it out. https://codereview.chromium.org/962453002/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/frame/browser_view.cc:2306: UpdatePermissionBubbleView(); Doesn't fullscreen mode imply a focus change, and so this undoes the bubble anyways? https://codereview.chromium.org/962453002/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/frame/browser_view.h (right): https://codereview.chromium.org/962453002/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/frame/browser_view.h:130: // fullscreen. Technically, no. The bubble gets dismissed on a window transition, no? https://codereview.chromium.org/962453002/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/website_settings/permissions_bubble_view.cc (right): https://codereview.chromium.org/962453002/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:373: anchor_view_ = view; This seems not right. doesn't the BubbleDelegateView already know the arrow? (And anchor, for that matter?) Why are we duplicating this data? https://codereview.chromium.org/962453002/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:387: CHECK(anchor_view_); Really? CHECK? We want this to crash instead of displaying the bubble in the wrong place? https://codereview.chromium.org/962453002/diff/20001/ui/views/bubble/bubble_d... File ui/views/bubble/bubble_delegate.cc (right): https://codereview.chromium.org/962453002/diff/20001/ui/views/bubble/bubble_d... ui/views/bubble/bubble_delegate.cc:105: DCHECK(arrow == BubbleBorder::NONE || arrow == BubbleBorder::FLOAT || Why verify this here, and not in the bubble where we draw it and need to access both arrow and anchor anyways?
https://codereview.chromium.org/962453002/diff/20001/ui/views/bubble/bubble_d... File ui/views/bubble/bubble_delegate.cc (right): https://codereview.chromium.org/962453002/diff/20001/ui/views/bubble/bubble_d... ui/views/bubble/bubble_delegate.cc:99: views::View* anchor = bubble_delegate->GetAnchorView(); On 2015/02/28 00:24:53, Hector Carmona wrote: > Trybots hated this check. I'm removing it. > > Are there cases where we should not be showing the anchor view or do we have a > lot of potential bugs? This statement does not make sense to me. What check are you removing? Which bugs are you referring to?
https://codereview.chromium.org/962453002/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/962453002/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/frame/browser_view.cc:543: permission_bubble_view_.reset(new PermissionBubbleViewViews( On 2015/02/28 00:25:12, groby wrote: > I'm still not sure why we need to pre-create |permission_bubble_view_|. I > suppose that's a different question, but I'd like to figure it out. We can probably update the code to create it only when it's needed and reuse it if it needs to be shown again. https://codereview.chromium.org/962453002/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/frame/browser_view.cc:2306: UpdatePermissionBubbleView(); On 2015/02/28 00:25:12, groby wrote: > Doesn't fullscreen mode imply a focus change, and so this undoes the bubble > anyways? The permission bubbles remain visible even when focus changes. This change is intended to update the anchor for the next time a bubble is shown and doesn't affect the current bubble. Should this re-position the current bubble? Should we hide the permissions bubbles when they lose focus? https://codereview.chromium.org/962453002/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/frame/browser_view.h (right): https://codereview.chromium.org/962453002/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/frame/browser_view.h:130: // fullscreen. On 2015/02/28 00:25:12, groby wrote: > Technically, no. The bubble gets dismissed on a window transition, no? The permission bubble remains visible even on a window transition. This comment is not clear enough that it will only update for the next time a new bubble is shown. Updated the comment, but you bring up a good point: should the permission bubbles be dismissed? https://codereview.chromium.org/962453002/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/website_settings/permissions_bubble_view.cc (right): https://codereview.chromium.org/962453002/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:373: anchor_view_ = view; On 2015/02/28 00:25:12, groby wrote: > This seems not right. doesn't the BubbleDelegateView already know the arrow? > (And anchor, for that matter?) > > Why are we duplicating this data? The BubbleDelegateView is created each time that |Show| is called, so we need to preserve the anchor/arrow. It's necessary to update the values since the PermissionBubbleViewViews can live through a fullscreen toggle. https://codereview.chromium.org/962453002/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:387: CHECK(anchor_view_); On 2015/02/28 00:25:12, groby wrote: > Really? CHECK? We want this to crash instead of displaying the bubble in the > wrong place? No, crash would be worse than displaying the bubble in the wrong place. What does it mean for a bubble to not have an anchor? Can that happen? https://codereview.chromium.org/962453002/diff/20001/ui/views/bubble/bubble_d... File ui/views/bubble/bubble_delegate.cc (right): https://codereview.chromium.org/962453002/diff/20001/ui/views/bubble/bubble_d... ui/views/bubble/bubble_delegate.cc:99: views::View* anchor = bubble_delegate->GetAnchorView(); On 2015/02/28 00:26:22, groby wrote: > On 2015/02/28 00:24:53, Hector Carmona wrote: > > Trybots hated this check. I'm removing it. > > > > Are there cases where we should not be showing the anchor view or do we have a > > lot of potential bugs? > > This statement does not make sense to me. What check are you removing? Which > bugs are you referring to? I added the DCHECK on line 105 to avoid having a bubble with an arrow when the anchor is invisible. This condition seems like something undesirable because we'll have a bubble with an arrow pointing at an invisible view. I removed the change to this file to avoid breaking a lot of tests. My question is: will we be missing UI bugs by not verifying that all bubbles with an anchor have an arrow? An example of an issue that would be caught by this is crbug.com/462087 https://codereview.chromium.org/962453002/diff/20001/ui/views/bubble/bubble_d... ui/views/bubble/bubble_delegate.cc:105: DCHECK(arrow == BubbleBorder::NONE || arrow == BubbleBorder::FLOAT || On 2015/02/28 00:25:12, groby wrote: > Why verify this here, and not in the bubble where we draw it and need to access > both arrow and anchor anyways? We can verify when drawing the bubble, but I imagine the trybots will be equally upset.
https://codereview.chromium.org/962453002/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/962453002/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/frame/browser_view.cc:2306: UpdatePermissionBubbleView(); On 2015/02/28 01:14:16, Hector Carmona wrote: > On 2015/02/28 00:25:12, groby wrote: > > Doesn't fullscreen mode imply a focus change, and so this undoes the bubble > > anyways? > > The permission bubbles remain visible even when focus changes. > > This change is intended to update the anchor for the next time a bubble is shown > and doesn't affect the current bubble. > > Should this re-position the current bubble? Should we hide the permissions > bubbles when they lose focus? No, we should not hide or move the permission bubbles when they lose focus. https://codereview.chromium.org/962453002/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/frame/browser_view.h (right): https://codereview.chromium.org/962453002/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/frame/browser_view.h:130: // fullscreen. On 2015/02/28 01:14:16, Hector Carmona wrote: > On 2015/02/28 00:25:12, groby wrote: > > Technically, no. The bubble gets dismissed on a window transition, no? > > The permission bubble remains visible even on a window transition. This comment > is not clear enough that it will only update for the next time a new bubble is > shown. > > Updated the comment, but you bring up a good point: should the permission > bubbles be dismissed? Good catch, I hadn't tested this. It should remain visible, but moved into the center (like what your current CL is doing otherwise if the omnibox is not visible). The tricky thing here is that there also will be a full screen ephemeral prompt in the same / nearby place, and it doesn't make sense to put them on top of each other. BTW you can test here: https://adrifelt.github.io/demos/all-permissions.html it has both notifications (for prompt) and full screen mode. https://codereview.chromium.org/962453002/diff/60001/chrome/browser/ui/views/... File chrome/browser/ui/views/frame/browser_view.h (right): https://codereview.chromium.org/962453002/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/frame/browser_view.h:131: void UpdatePermissionBubbleView(); Should this be renamed something more literal, like UpdatePermissionBubbleViewAnchor? Would remove the need for part of the comment.
https://codereview.chromium.org/962453002/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/frame/browser_view.h (right): https://codereview.chromium.org/962453002/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/frame/browser_view.h:130: // fullscreen. On 2015/03/03 01:04:35, felt wrote: > On 2015/02/28 01:14:16, Hector Carmona wrote: > > On 2015/02/28 00:25:12, groby wrote: > > > Technically, no. The bubble gets dismissed on a window transition, no? > > > > The permission bubble remains visible even on a window transition. This > comment > > is not clear enough that it will only update for the next time a new bubble is > > shown. > > > > Updated the comment, but you bring up a good point: should the permission > > bubbles be dismissed? > > Good catch, I hadn't tested this. > > It should remain visible, but moved into the center (like what your current CL > is doing otherwise if the omnibox is not visible). > > The tricky thing here is that there also will be a full screen ephemeral prompt > in the same / nearby place, and it doesn't make sense to put them on top of each > other. > > BTW you can test here: https://adrifelt.github.io/demos/all-permissions.html it > has both notifications (for prompt) and full screen mode. Where should we move the promt to? Centering the prompt does make it look very weird because it overlaps the full screen prompt. https://codereview.chromium.org/962453002/diff/60001/chrome/browser/ui/views/... File chrome/browser/ui/views/frame/browser_view.h (right): https://codereview.chromium.org/962453002/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/frame/browser_view.h:131: void UpdatePermissionBubbleView(); On 2015/03/03 01:04:35, felt wrote: > Should this be renamed something more literal, like > UpdatePermissionBubbleViewAnchor? Would remove the need for part of the comment. Done.
https://codereview.chromium.org/962453002/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/frame/browser_view.h (right): https://codereview.chromium.org/962453002/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/frame/browser_view.h:130: // fullscreen. On 2015/03/06 23:24:16, Hector Carmona wrote: > On 2015/03/03 01:04:35, felt wrote: > > On 2015/02/28 01:14:16, Hector Carmona wrote: > > > On 2015/02/28 00:25:12, groby wrote: > > > > Technically, no. The bubble gets dismissed on a window transition, no? > > > > > > The permission bubble remains visible even on a window transition. This > > comment > > > is not clear enough that it will only update for the next time a new bubble > is > > > shown. > > > > > > Updated the comment, but you bring up a good point: should the permission > > > bubbles be dismissed? > > > > Good catch, I hadn't tested this. > > > > It should remain visible, but moved into the center (like what your current CL > > is doing otherwise if the omnibox is not visible). > > > > The tricky thing here is that there also will be a full screen ephemeral > prompt > > in the same / nearby place, and it doesn't make sense to put them on top of > each > > other. > > > > BTW you can test here: https://adrifelt.github.io/demos/all-permissions.html > it > > has both notifications (for prompt) and full screen mode. > > Where should we move the promt to? Centering the prompt does make it look very > weird because it overlaps the full screen prompt. Is it possible to tell if the full screen prompt is also displayed, and if so, push it down below it?
On 2015/03/07 03:31:55, felt wrote: > https://codereview.chromium.org/962453002/diff/20001/chrome/browser/ui/views/... > File chrome/browser/ui/views/frame/browser_view.h (right): > > https://codereview.chromium.org/962453002/diff/20001/chrome/browser/ui/views/... > chrome/browser/ui/views/frame/browser_view.h:130: // fullscreen. > On 2015/03/06 23:24:16, Hector Carmona wrote: > > On 2015/03/03 01:04:35, felt wrote: > > > On 2015/02/28 01:14:16, Hector Carmona wrote: > > > > On 2015/02/28 00:25:12, groby wrote: > > > > > Technically, no. The bubble gets dismissed on a window transition, no? > > > > > > > > The permission bubble remains visible even on a window transition. This > > > comment > > > > is not clear enough that it will only update for the next time a new > bubble > > is > > > > shown. > > > > > > > > Updated the comment, but you bring up a good point: should the permission > > > > bubbles be dismissed? > > > > > > Good catch, I hadn't tested this. > > > > > > It should remain visible, but moved into the center (like what your current > CL > > > is doing otherwise if the omnibox is not visible). > > > > > > The tricky thing here is that there also will be a full screen ephemeral > > prompt > > > in the same / nearby place, and it doesn't make sense to put them on top of > > each > > > other. > > > > > > BTW you can test here: https://adrifelt.github.io/demos/all-permissions.html > > it > > > has both notifications (for prompt) and full screen mode. > > > > Where should we move the promt to? Centering the prompt does make it look very > > weird because it overlaps the full screen prompt. > > Is it possible to tell if the full screen prompt is also displayed, and if so, > push it down below it? Done. I've updated the UI so that the permission bubble is bellow the full screen prompt. I've updated the description to include some screenshots that show how it will look.
You should probably add somebody to the reviewers who is an OWNER :)
msw@, you're owner at the views level. Would you be the right reviewer?
I'm not sure if this is a good idea as a one-off solution with no follow up. Some folks are working on better general practices for bubbles, including how to handle multiple bubbles at the same time... fullscreen behavior is an interesting case for them to consider. +CC ainslie, finnur, wittman, gbillock, dbeam (not sure who's really working on this...) You might want to help with related http://crbug.com/165799 I wonder if this matches the zoom bubble: http://crbug.com/167367 https://codereview.chromium.org/962453002/diff/100001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/962453002/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_view.cc:548: void BrowserView::UpdatePermissionBubbleViewAnchor() { Other bubbles (bookmark, global error, avatar/profile, etc.) don't have this special behavior in fullscreen (no arrow, anchor to the fullscreen bubble or top container). I think we ought to be consistent about the behavior of these bubbles... please ping UX about the right behavior. I'd generally support your solution here, but only if the same behavior were recommended for the other bubbles (with bugs filed to follow up), and we had some mechanism [planned] to handle overlapping bubbles (what if the permission bubble is showing and the user hits CTRL+D for the bookmark bubble, CTRL+M for the avatar/profile bubble, or gets a global error bubble, etc.)... https://codereview.chromium.org/962453002/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_view.cc:554: views::View* view = NULL; nit: init |view| to |top_container_| to avoid the else below. https://codereview.chromium.org/962453002/diff/100001/chrome/browser/ui/views... File chrome/browser/ui/views/website_settings/permissions_bubble_view.cc (right): https://codereview.chromium.org/962453002/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:366: views::BubbleFrameView* frame = static_cast<views::BubbleFrameView*>( nit: use BubbleDelegateView::GetBubbleFrameView() https://codereview.chromium.org/962453002/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:372: new views::BubbleBorder(adjusted_arrow, shadow(), color()))); nit: outdent 2 spaces. https://codereview.chromium.org/962453002/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:378: nit: remove extra blank line. https://codereview.chromium.org/962453002/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:384: : anchor_view_(NULL), nit: nullptr here and elsewhere.
msw: I'm currently the person trying to corral various bubble efforts. I asked hector to fix this as a one-off, since permission bubbles are somewhat critical (and a generic solution might take a while) https://codereview.chromium.org/962453002/diff/100001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/962453002/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_view.cc:548: void BrowserView::UpdatePermissionBubbleViewAnchor() { On 2015/03/11 22:25:30, msw wrote: > Other bubbles (bookmark, global error, avatar/profile, etc.) don't have this > special behavior in fullscreen (no arrow, anchor to the fullscreen bubble or top > container). I think we ought to be consistent about the behavior of these > bubbles... please ping UX about the right behavior. > > I'd generally support your solution here, but only if the same behavior were > recommended for the other bubbles (with bugs filed to follow up), and we had > some mechanism [planned] to handle overlapping bubbles (what if the permission > bubble is showing and the user hits CTRL+D for the bookmark bubble, CTRL+M for > the avatar/profile bubble, or gets a global error bubble, etc.)... Hector: The OSX version manages without modifications to the BrowserView. Can we take the same approach on views? If we can't, can the bubble view listen to a fullscreen change notification and just re-query its anchor point, as opposed to hard-coding the dependency here?
https://codereview.chromium.org/962453002/diff/100001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/962453002/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_view.cc:548: void BrowserView::UpdatePermissionBubbleViewAnchor() { On 2015/03/12 00:44:49, groby wrote: > On 2015/03/11 22:25:30, msw wrote: > > Other bubbles (bookmark, global error, avatar/profile, etc.) don't have this > > special behavior in fullscreen (no arrow, anchor to the fullscreen bubble or > top > > container). I think we ought to be consistent about the behavior of these > > bubbles... please ping UX about the right behavior. Is there a specific person I should ping, or should I send a message to chrome-ux@? > > I'd generally support your solution here, but only if the same behavior were > > recommended for the other bubbles (with bugs filed to follow up), and we had > > some mechanism [planned] to handle overlapping bubbles (what if the permission > > bubble is showing and the user hits CTRL+D for the bookmark bubble, CTRL+M for > > the avatar/profile bubble, or gets a global error bubble, etc.)... > > Hector: The OSX version manages without modifications to the BrowserView. Can we > take the same approach on views? Unfortunately, no because in OSX when fullscreen toggles the bubbles still have an anchor. This is necessary in order to re-anchor the permission bubbles. Most bubbles are dismissed in the transition, but permission bubbles are special because users should interact with them. > If we can't, can the bubble view listen to a fullscreen change notification and > just re-query its anchor point, as opposed to hard-coding the dependency here? I was thinking that this would be a good place to send the notification because BrowserView already knows about both the anchor point and the permission bubble. https://codereview.chromium.org/962453002/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_view.cc:554: views::View* view = NULL; On 2015/03/11 22:25:30, msw wrote: > nit: init |view| to |top_container_| to avoid the else below. Done. https://codereview.chromium.org/962453002/diff/100001/chrome/browser/ui/views... File chrome/browser/ui/views/website_settings/permissions_bubble_view.cc (right): https://codereview.chromium.org/962453002/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:366: views::BubbleFrameView* frame = static_cast<views::BubbleFrameView*>( On 2015/03/11 22:25:30, msw wrote: > nit: use BubbleDelegateView::GetBubbleFrameView() Done. https://codereview.chromium.org/962453002/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:372: new views::BubbleBorder(adjusted_arrow, shadow(), color()))); On 2015/03/11 22:25:31, msw wrote: > nit: outdent 2 spaces. Done. https://codereview.chromium.org/962453002/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:378: On 2015/03/11 22:25:31, msw wrote: > nit: remove extra blank line. Done. https://codereview.chromium.org/962453002/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:384: : anchor_view_(NULL), On 2015/03/11 22:25:30, msw wrote: > nit: nullptr here and elsewhere. Done.
https://codereview.chromium.org/962453002/diff/100001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/962453002/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_view.cc:548: void BrowserView::UpdatePermissionBubbleViewAnchor() { On 2015/03/12 18:43:33, Hector Carmona wrote: > On 2015/03/12 00:44:49, groby wrote: > > On 2015/03/11 22:25:30, msw wrote: > > > Other bubbles (bookmark, global error, avatar/profile, etc.) don't have this > > > special behavior in fullscreen (no arrow, anchor to the fullscreen bubble or > > top > > > container). I think we ought to be consistent about the behavior of these > > > bubbles... please ping UX about the right behavior. > > Is there a specific person I should ping, or should I send a message to > chrome-ux@? ainslie@ is looking at all things bubble. felt@ can tell you how important it is to push this for permissions, even if we don't unify all the bubbles yet. (My take is that it's important enough to justify a one-off) > > > > I'd generally support your solution here, but only if the same behavior were > > > recommended for the other bubbles (with bugs filed to follow up), and we had > > > some mechanism [planned] to handle overlapping bubbles (what if the > permission > > > bubble is showing and the user hits CTRL+D for the bookmark bubble, CTRL+M > for > > > the avatar/profile bubble, or gets a global error bubble, etc.)... > > > > Hector: The OSX version manages without modifications to the BrowserView. Can > we > > take the same approach on views? > > Unfortunately, no because in OSX when fullscreen toggles the bubbles still have > an anchor. This is necessary in order to re-anchor the permission bubbles. Most > bubbles are dismissed in the transition, but permission bubbles are special > because users should interact with them. > > > If we can't, can the bubble view listen to a fullscreen change notification > and > > just re-query its anchor point, as opposed to hard-coding the dependency here? > > I was thinking that this would be a good place to send the notification because > BrowserView already knows about both the anchor point and the permission bubble. I'll defer to msw@ on that - he's more familiar with the views side of things.
This bubble has non-trivial one-off behavior that strikes me as odd. 1) From the start I expressed concern that this bubble is browser-modal, but simulates content-modality by re-appearing after switching to another tab and back (in BrowserView::OnActiveTabChanged). No other browser-modal bubble or dialog works that way. This bubble should be a content-modal dialog like payments, close on loss of activation, or we ought to consider proper support for persistent content-modal bubbles. 2) See my comment about initializing this bubble with the browser frame. 3) I'd like to see consistent behavior for bubbles opened in windows without location bars, opened in fullscreen, and persisting across fullscreen toggles, etc. I suppose this one-off will be fine since it's deemed so important, but then the logic shouldn't live in BrowserView, it should live in PermissionBubbleViewViews, the manager, or something like that. https://codereview.chromium.org/962453002/diff/120001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/962453002/diff/120001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_view.cc:548: void BrowserView::UpdatePermissionBubbleViewAnchor() { For a one-off like this without imminent plans to generalize the behavior, I'd like to see this code moved to PermissionBubbleViewViews, as groby suggests. Everything here looks publicly accessible on BrowserView. https://codereview.chromium.org/962453002/diff/120001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_view.h (right): https://codereview.chromium.org/962453002/diff/120001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_view.h:130: void InitPermissionBubbleView(); We should probably remove InitPermissionBubbleView altogether, this isn't browser frame UI that needs to be initialized with the browser frame, it's transient UI that should be created when/if needed (and destroyed on close).
Applied feedback but running into a weird issue: 1. Enter Fullscreen 2. Ask for Geo permission 3. Fullscreen bubble fades 4. Geo bubble fades But if permission is requested first, everything works as expected: 1. Ask for Geo permission 2. Enter Fullscreen 3. Fullscreen bubble fades 4. Geo bubble remains I will continue looking into this, but I'm wondering if anyone knows what would cause this behavior? https://codereview.chromium.org/962453002/diff/120001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/962453002/diff/120001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_view.cc:548: void BrowserView::UpdatePermissionBubbleViewAnchor() { On 2015/03/12 20:44:05, msw - on vacation - back tue wrote: > For a one-off like this without imminent plans to generalize the behavior, I'd > like to see this code moved to PermissionBubbleViewViews, as groby suggests. > Everything here looks publicly accessible on BrowserView. Done. https://codereview.chromium.org/962453002/diff/120001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_view.h (right): https://codereview.chromium.org/962453002/diff/120001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_view.h:130: void InitPermissionBubbleView(); On 2015/03/12 20:44:05, msw - on vacation - back tue wrote: > We should probably remove InitPermissionBubbleView altogether, this isn't > browser frame UI that needs to be initialized with the browser frame, it's > transient UI that should be created when/if needed (and destroyed on close). Done.
Please try to use widget observation (but not if it's a big hassle). On 2015/03/17 01:29:27, Hector Carmona wrote: > Applied feedback but running into a weird issue: > > 1. Enter Fullscreen > 2. Ask for Geo permission > 3. Fullscreen bubble fades > 4. Geo bubble fades > > But if permission is requested first, everything works as expected: > > 1. Ask for Geo permission > 2. Enter Fullscreen > 3. Fullscreen bubble fades > 4. Geo bubble remains > > I will continue looking into this, but I'm wondering if anyone knows what would > cause this behavior? This is likely because the first case uses the fullscreen bubble as its anchor view/widget for the initial construction of the bubble. That gets used as the Widget::InitParams::parent in CreateBubbleWidget, and later, when that parent widget is hidden or destroyed, the child follows suit. You can work around this by always setting the BubbleDelegateView::parent_window_ to the browser's native window (and still use the fullscreen bubble as the anchor view/widget). https://codereview.chromium.org/962453002/diff/140001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/962453002/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_view.cc:2293: permission_bubble_view_->UpdateAnchorPosition(); As a views::BubbleDelegateView, PermissionsBubbleDelegateView is already observing its |anchor_widget_| via the WidgetObserver pattern (see BubbleDelegateView::SetAnchorView). You may be able to remove this explicit anchor update and override PermissionsBubbleDelegateView::OnWidgetBoundsChanged to check if the anchor_widget_->IsFullscreen() has changed and update as needed. https://codereview.chromium.org/962453002/diff/140001/chrome/browser/ui/views... File chrome/browser/ui/views/website_settings/permissions_bubble_view.cc (right): https://codereview.chromium.org/962453002/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:394: languages_(browser->profile()->GetPrefs()->GetString( Inline this in PermissionBubbleViewViews::Show and nix |languages_|. https://codereview.chromium.org/962453002/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:404: return browser_view_->GetLocationBarView()->location_icon_view(); Use BrowserView::GetBrowserViewForBrowser in this function, then you can drop the BrowserView member and constructor argument. https://codereview.chromium.org/962453002/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:406: return browser_view_->exclusive_access_bubble()->GetView(); nit: no else after return. https://codereview.chromium.org/962453002/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:408: return browser_view_->top_container(); nit: no else after return. https://codereview.chromium.org/962453002/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:414: else nit: no else after return. https://codereview.chromium.org/962453002/diff/140001/chrome/browser/ui/views... File chrome/browser/ui/views/website_settings/permissions_bubble_view.h (right): https://codereview.chromium.org/962453002/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permissions_bubble_view.h:12: #include "chrome/browser/ui/browser.h" nit: forward declare Browser for this header, include this in the cc. https://codereview.chromium.org/962453002/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permissions_bubble_view.h:13: #include "chrome/browser/ui/views/frame/browser_view.h" nit: forward declare BrowserView for this header, include this in the cc. https://codereview.chromium.org/962453002/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permissions_bubble_view.h:25: explicit PermissionBubbleViewViews(Browser* browser, The explicit keyword is only needed for constructors with one parameter, but you will need it here if you remove the BrowserView* parameter as I suggest. https://codereview.chromium.org/962453002/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permissions_bubble_view.h:29: views::View* GetAnchorView(); These three functions should be private, since they're not used externally.
Previous weird issue has been resolved thanks to msw's feedback. Moved the update anchor code to the widget observer, however this introduces a new issue I'm investing: the permission bubble isn't placed correctly when a maximized window is toggled to/from fullscreen. The placement is correct when the bubble is created in either state, but fails to transition to the new location on toggle. https://codereview.chromium.org/962453002/diff/140001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/962453002/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_view.cc:2293: permission_bubble_view_->UpdateAnchorPosition(); On 2015/03/17 21:13:15, msw wrote: > As a views::BubbleDelegateView, PermissionsBubbleDelegateView is already > observing its |anchor_widget_| via the WidgetObserver pattern (see > BubbleDelegateView::SetAnchorView). You may be able to remove this explicit > anchor update and override PermissionsBubbleDelegateView::OnWidgetBoundsChanged > to check if the anchor_widget_->IsFullscreen() has changed and update as needed. Done, with some differences: rather than updating on fullscreen change, I'm updating when the anchor should be different. This is because the fullscreen toggle happens before the fullscreen bubble is visible and the permission bubble appears underneath it. https://codereview.chromium.org/962453002/diff/140001/chrome/browser/ui/views... File chrome/browser/ui/views/website_settings/permissions_bubble_view.cc (right): https://codereview.chromium.org/962453002/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:394: languages_(browser->profile()->GetPrefs()->GetString( On 2015/03/17 21:13:15, msw wrote: > Inline this in PermissionBubbleViewViews::Show and nix |languages_|. Done. https://codereview.chromium.org/962453002/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:404: return browser_view_->GetLocationBarView()->location_icon_view(); On 2015/03/17 21:13:15, msw wrote: > Use BrowserView::GetBrowserViewForBrowser in this function, then you can drop > the BrowserView member and constructor argument. Done. https://codereview.chromium.org/962453002/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:406: return browser_view_->exclusive_access_bubble()->GetView(); On 2015/03/17 21:13:15, msw wrote: > nit: no else after return. Done. https://codereview.chromium.org/962453002/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:408: return browser_view_->top_container(); On 2015/03/17 21:13:15, msw wrote: > nit: no else after return. Done. https://codereview.chromium.org/962453002/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:414: else On 2015/03/17 21:13:15, msw wrote: > nit: no else after return. Done. https://codereview.chromium.org/962453002/diff/140001/chrome/browser/ui/views... File chrome/browser/ui/views/website_settings/permissions_bubble_view.h (right): https://codereview.chromium.org/962453002/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permissions_bubble_view.h:12: #include "chrome/browser/ui/browser.h" On 2015/03/17 21:13:15, msw wrote: > nit: forward declare Browser for this header, include this in the cc. Done. https://codereview.chromium.org/962453002/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permissions_bubble_view.h:13: #include "chrome/browser/ui/views/frame/browser_view.h" On 2015/03/17 21:13:15, msw wrote: > nit: forward declare BrowserView for this header, include this in the cc. Forward declaration no longer needed. https://codereview.chromium.org/962453002/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permissions_bubble_view.h:25: explicit PermissionBubbleViewViews(Browser* browser, On 2015/03/17 21:13:15, msw wrote: > The explicit keyword is only needed for constructors with one parameter, but you > will need it here if you remove the BrowserView* parameter as I suggest. Done. https://codereview.chromium.org/962453002/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permissions_bubble_view.h:29: views::View* GetAnchorView(); On 2015/03/17 21:13:15, msw wrote: > These three functions should be private, since they're not used externally. Done.
On 2015/03/18 18:00:06, Hector Carmona wrote: > Moved the update anchor code to the widget observer, however this > introduces a new issue I'm investing: the permission bubble isn't placed > correctly when a maximized window is toggled to/from fullscreen. The > placement is correct when the bubble is created in either state, but > fails to transition to the new location on toggle. Hmm, perhaps OnWidgetBoundsChanged isn't called when transitioning a browser window between fullscreen and maximized. Can you try to investigate that? It should be called, otherwise you can: 1) Attempt to fix that defect in a separate pre-requisite CL. 2) Revert to calling permission_bubble_view_->UpdateAnchorPosition() in BrowserView::ProcessFullscreen. 3) Use some other notification mechanism to observe and respond to fullscreen changes. https://codereview.chromium.org/962453002/diff/160001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/962453002/diff/160001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_view.cc:851: permission_bubble_view_.reset( optional nit: rename the member to |permission_bubble_|. (this would fit on one line and not need curly braces, etc.) https://codereview.chromium.org/962453002/diff/160001/chrome/browser/ui/views... File chrome/browser/ui/views/website_settings/permissions_bubble_view.cc (right): https://codereview.chromium.org/962453002/diff/160001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:348: views::Widget* widget, const gfx::Rect& new_bounds) { nit: one parameter per line, if the whole signature doesn't fit on one line. https://codereview.chromium.org/962453002/diff/160001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:350: owner_->UpdateAnchorPosition(); Is this really necessary on every OnWidgetBoundsChanged? OnWidgetDestroying might be sufficient. https://codereview.chromium.org/962453002/diff/160001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:412: bubble_delegate_(NULL) {} nit: nullptr here and elsewhere. https://codereview.chromium.org/962453002/diff/160001/chrome/browser/ui/views... File chrome/browser/ui/views/website_settings/permissions_bubble_view.h (right): https://codereview.chromium.org/962453002/diff/160001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permissions_bubble_view.h:47: views::View* GetAnchorView_(); List functions before member variables, remove the underscores from their names. Try to follow similar/surrounding code styles and follow our style guides: https://sites.google.com/a/chromium.org/dev/developers/coding-style
On 2015/03/18 18:30:09, msw wrote: > On 2015/03/18 18:00:06, Hector Carmona wrote: > > Moved the update anchor code to the widget observer, however this > > introduces a new issue I'm investing: the permission bubble isn't placed > > correctly when a maximized window is toggled to/from fullscreen. The > > placement is correct when the bubble is created in either state, but > > fails to transition to the new location on toggle. > > Hmm, perhaps OnWidgetBoundsChanged isn't called when transitioning a browser > window between fullscreen and maximized. Can you try to investigate that? It > should be called, otherwise you can: > 1) Attempt to fix that defect in a separate pre-requisite CL. > 2) Revert to calling permission_bubble_view_->UpdateAnchorPosition() in > BrowserView::ProcessFullscreen. > 3) Use some other notification mechanism to observe and respond to fullscreen > changes. Looking into #1. We get 4 extra OnWidgetBoundsChanged events when not maximized. Trying to find the cause. https://codereview.chromium.org/962453002/diff/160001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/962453002/diff/160001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_view.cc:851: permission_bubble_view_.reset( On 2015/03/18 18:30:09, msw wrote: > optional nit: rename the member to |permission_bubble_|. > (this would fit on one line and not need curly braces, etc.) Done. https://codereview.chromium.org/962453002/diff/160001/chrome/browser/ui/views... File chrome/browser/ui/views/website_settings/permissions_bubble_view.cc (right): https://codereview.chromium.org/962453002/diff/160001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:348: views::Widget* widget, const gfx::Rect& new_bounds) { On 2015/03/18 18:30:09, msw wrote: > nit: one parameter per line, if the whole signature doesn't fit on one line. Done. https://codereview.chromium.org/962453002/diff/160001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:350: owner_->UpdateAnchorPosition(); On 2015/03/18 18:30:09, msw wrote: > Is this really necessary on every OnWidgetBoundsChanged? OnWidgetDestroying > might be sufficient. OnWidgetDestroying is only called when the bubble is anchored to the fullscreen bubble and we exit fullscreen. https://codereview.chromium.org/962453002/diff/160001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permissions_bubble_view.cc:412: bubble_delegate_(NULL) {} On 2015/03/18 18:30:09, msw wrote: > nit: nullptr here and elsewhere. Done: changed NULL to nullptr in this file and exclusive_access_bubble_views.cc https://codereview.chromium.org/962453002/diff/160001/chrome/browser/ui/views... File chrome/browser/ui/views/website_settings/permissions_bubble_view.h (right): https://codereview.chromium.org/962453002/diff/160001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permissions_bubble_view.h:47: views::View* GetAnchorView_(); On 2015/03/18 18:30:09, msw wrote: > List functions before member variables, remove the underscores from their names. > Try to follow similar/surrounding code styles and follow our style guides: > https://sites.google.com/a/chromium.org/dev/developers/coding-style Done.
On 2015/03/18 23:50:39, Hector Carmona wrote: > On 2015/03/18 18:30:09, msw wrote: > > On 2015/03/18 18:00:06, Hector Carmona wrote: > > > Moved the update anchor code to the widget observer, however this > > > introduces a new issue I'm investing: the permission bubble isn't placed > > > correctly when a maximized window is toggled to/from fullscreen. The > > > placement is correct when the bubble is created in either state, but > > > fails to transition to the new location on toggle. > > > > Hmm, perhaps OnWidgetBoundsChanged isn't called when transitioning a browser > > window between fullscreen and maximized. Can you try to investigate that? It > > should be called, otherwise you can: > > 1) Attempt to fix that defect in a separate pre-requisite CL. > > 2) Revert to calling permission_bubble_view_->UpdateAnchorPosition() in > > BrowserView::ProcessFullscreen. > > 3) Use some other notification mechanism to observe and respond to fullscreen > > changes. > > Looking into #1. We get 4 extra OnWidgetBoundsChanged events when not maximized. > Trying to find the cause. Looks pretty good, let me know when you resolve this issue.
On 2015/03/19 18:26:07, msw wrote: > On 2015/03/18 23:50:39, Hector Carmona wrote: > > On 2015/03/18 18:30:09, msw wrote: > > > On 2015/03/18 18:00:06, Hector Carmona wrote: > > > > Moved the update anchor code to the widget observer, however this > > > > introduces a new issue I'm investing: the permission bubble isn't placed > > > > correctly when a maximized window is toggled to/from fullscreen. The > > > > placement is correct when the bubble is created in either state, but > > > > fails to transition to the new location on toggle. > > > > > > Hmm, perhaps OnWidgetBoundsChanged isn't called when transitioning a browser > > > window between fullscreen and maximized. Can you try to investigate that? It > > > should be called, otherwise you can: > > > 1) Attempt to fix that defect in a separate pre-requisite CL. > > > 2) Revert to calling permission_bubble_view_->UpdateAnchorPosition() in > > > BrowserView::ProcessFullscreen. > > > 3) Use some other notification mechanism to observe and respond to > fullscreen > > > changes. > > > > Looking into #1. We get 4 extra OnWidgetBoundsChanged events when not > maximized. > > Trying to find the cause. > > Looks pretty good, let me know when you resolve this issue. Emailed with pkotwicz@ after investigating #1 a bit. The issue seems to happen in DesktopWindowTreeHostX11, but I've also learned that OnWidgetBoundsChanged is not guaranteed to be called if the window doesn't change size but fullscreen is toggled. I've reverted the code so that we UpdateAnchorPosition in ProcessFullScreen so that the bubble is placed correctly.
lgtm with a comment nit, thanks! https://codereview.chromium.org/962453002/diff/220001/chrome/browser/ui/views... File chrome/browser/ui/views/website_settings/permissions_bubble_view.h (right): https://codereview.chromium.org/962453002/diff/220001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permissions_bubble_view.h:27: void UpdateAnchorPosition(); nit: add a simple comment like "Updates the anchor on fullscreen changes, etc."
Applied msw@'s feedback. felt@, groby@ any additional feedback before committing this CL? https://codereview.chromium.org/962453002/diff/220001/chrome/browser/ui/views... File chrome/browser/ui/views/website_settings/permissions_bubble_view.h (right): https://codereview.chromium.org/962453002/diff/220001/chrome/browser/ui/views... chrome/browser/ui/views/website_settings/permissions_bubble_view.h:27: void UpdateAnchorPosition(); On 2015/03/23 19:34:53, msw wrote: > nit: add a simple comment like "Updates the anchor on fullscreen changes, etc." Done.
looks good to me, thank you!
LGTM as well
The CQ bit was checked by hcarmona@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org Link to the patchset: https://codereview.chromium.org/962453002/#ps240001 (title: "Add comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/962453002/240001
Message was sent while issue was closed.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/32a02419d38bf802329767c8bee375cc446e6756 Cr-Commit-Position: refs/heads/master@{#321864} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
