BrowserView permission bubble anchor is too low (non-harmony) incorrectly offset (harmony) |
||||||||||||
Issue descriptionAs pointed out in this blog post: https://textslashplain.com/2017/01/14/the-line-of-death/ Steps to reproduce: 1) Load https://caturday-post.herokuapp.com/ and click the lock icon in the url bar 2) Observe that the chevron and popup cover the entire bottom of the omnibox 3) On the same page, click the slider to "enable notifications" 4) Observe that the permissions prompt is significantly lower, with only part of the chevron overlapping the omnibox Read the above blog post for reasons why this in dangerous, beyond just being inconsistent.
,
Jan 18 2017
I don't think those pixels do much for danger, as long as *something* is pointing outside the viewport. But we should definitely stay consistent. In any case, this alignment depends on your platform and your flags. Could you include those? (And if you have more time, report for both values of chrome://flags/#secondary-ui-md)
,
Jan 18 2017
The attached screenshot is directly from the article; I'm not sure what platform and flags they're using. I replicate the same exact behavior on my own machine, however, and this is what I've got: Google Chrome: 55.0.2883.75 (Official Build) beta (64-bit) OS: Linux secondary-ui-md is currently off Screenshot of the rest: https://screenshot.googleplex.com/9XswbYJpL9h I also reproduce the behavior on my Pixel 2 with the secondary-ui-md off: Google Chrome: 55.0.2883.82 (Official Build) beta (64-bit) Platform: 8872.67.0 (Official Build) beta-channel samus With the flag on, neither popup has a chevron, but both cover the same (small) amount of the Chrome UI / Omnibox.
,
Feb 10 2017
ellyjones@, could you triage?
,
Feb 10 2017
tapted: you worked on bubble positioning before, I think - what do here?
,
Feb 12 2017
This only affects Win/Linux/CrOS - the Mac permission bubble code is fine both with and without harmony (i.e. chrome://flags/#secondary-ui-md). I haven't adjusted the way bubbles are positioned on the toolkit-views browsers - only the Cocoa browser ( Issue 657263 ). Site settings / OIB seems to anchor fine in all cases. With --secondary-ui-md, the bubble vertical position is good, but the left side doesn't align. That needs to be fixed, and fixed in a way that doesn't break the non-harmony bubble (nor the positioning logic in fullscreen). r395850 plumbed through the permissions bubbles for mac - I don't think it moved the anchor for non-mac. There were tweaks done for fullscreen in r402133 To fix non-harmony I think views::View* PermissionPromptImpl::GetAnchorView() needs to invoke the same snippet as the OIB: // Compensate for built-in vertical padding in the anchor view's image. set_anchor_view_insets(gfx::Insets( GetLayoutConstant(LOCATION_BAR_BUBBLE_ANCHOR_VERTICAL_INSET), 0)); and then "not break" all the other things (fullscreen, mac, harmony)
,
Feb 13 2017
Urp - there's one more dimension to consider, which is for the chrome:// internal pages -- there's some logic for those which is not shared, and is currently doing the wrong thing under harmony (on Mac and non-Mac).
Attaching what Windows does currently (it's too far to the left). The relevant snippet from the OIB is probably
views::NonClientFrameView* InternalPageInfoPopupView::CreateNonClientFrameView(
views::Widget* widget) {
views::BubbleFrameView* frame = static_cast<views::BubbleFrameView*>(
BubbleDialogDelegateView::CreateNonClientFrameView(widget));
// Padding around icon + half of icon width.
frame->bubble_border()->set_arrow_offset(
kSpacing + 8 + frame->bubble_border()->GetBorderThickness());
return frame;
}
So, in the current framework, some combination of BubbleBorder::set_arrow_offset(int) and BubbleDialogDelegate::set_anchor_view_insets(..) is probably able to handle all the permutations.
(and note this comment is about a bug in the OIB -- not the permission bubble -- but I'm mentioning it here since there may be some scope for these to share some code)
,
Feb 13 2017
https://codereview.chromium.org/2693803002/ That doesn't fix #c7, but it looks orthogonal - I think the fix is just to delete that code snippet in #c7 which I'll make a separate patch for.
,
Feb 13 2017
"after" screenshots The "remember this decision" checkbox appears to be new in ToT (and is clipped in Harmony), but that's not related to the CL in #c8
,
Feb 13 2017
Investigated #c7 in Issue 691445 (the weird offset there needs to be fixed for Harmony at the same time as we remove the Chrome logo - that's Issue 652028 )
,
Feb 13 2017
And the checkbox is a finch experiment -- chrome://flags/#permission-prompt-persistence-toggle and Issue 656129 (closed WontFix) -> or duped into Issue 661707 (still open)
,
Feb 14 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6ef9d93c9b63ff3169f2c3c13e1ef2b4113ae1a1 commit 6ef9d93c9b63ff3169f2c3c13e1ef2b4113ae1a1 Author: tapted <tapted@chromium.org> Date: Tue Feb 14 05:33:16 2017 Fix position of permission bubbles anchored off the padlock. Allows permission request bubbles to share some of the anchoring logic used by the site settings bubble. Under non-MD/Harmony, this will shift the permissions request popup up slightly so that the arrow points to the icon. Under MD/Harmony, the bubble will be anchored to the bottom of the location bar itself, with edges aligned. BUG= 682073 Review-Url: https://codereview.chromium.org/2693803002 Cr-Commit-Position: refs/heads/master@{#450268} [modify] https://crrev.com/6ef9d93c9b63ff3169f2c3c13e1ef2b4113ae1a1/chrome/browser/ui/views/frame/browser_view.cc [modify] https://crrev.com/6ef9d93c9b63ff3169f2c3c13e1ef2b4113ae1a1/chrome/browser/ui/views/location_bar/location_bar_view.cc [modify] https://crrev.com/6ef9d93c9b63ff3169f2c3c13e1ef2b4113ae1a1/chrome/browser/ui/views/location_bar/location_bar_view.h [modify] https://crrev.com/6ef9d93c9b63ff3169f2c3c13e1ef2b4113ae1a1/chrome/browser/ui/views/website_settings/permission_prompt_impl.cc [modify] https://crrev.com/6ef9d93c9b63ff3169f2c3c13e1ef2b4113ae1a1/chrome/browser/ui/views/website_settings/permission_prompt_impl_views.cc
,
Feb 14 2017
I made Issue 691454 for the clipped checkbox. Issue 652028 is the ticket for updating the site info bubble for chrome:// sites under Harmony. |
||||||||||||
►
Sign in to add a comment |
||||||||||||
Comment 1 by jww@chromium.org
, Jan 18 2017Owner: lgar...@chromium.org
Status: Assigned (was: Untriaged)