New issue
Advanced search Search tips

Issue 869928 link

Starred by 3 users
Status: Assigned
Owner:
tapted@chromium.org
Cc:
pbos@chromium.org
bsep@chromium.org
cyan@chromium.org
tapted@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 3
Type: Bug

Blocked on:
issue 879048


Sign in to add a comment

Deprecate calls to BubbleDialogDelegate::set_anchor_view_insets

Project Member Reported by pbos@chromium.org, Aug 1 
Usages of this API should be replaced with having the anchor views themselves override Views::GetAnchorBoundsInScreen().

See https://chromium-review.googlesource.com/c/chromium/src/+/1152525/3/ui/views/bubble/bubble_dialog_delegate.cc#210

That CL above also adds GetAnchorBoundsInScreen(), so this is not fixable before that CL lands.
Project Member

Comment 1 by bugdroid1@chromium.org, Aug 1 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6f92518b820eb652ed4d03512728f1d032042a96

commit 6f92518b820eb652ed4d03512728f1d032042a96
Author: Peter Boström <pbos@chromium.org>
Date: Wed Aug 01 18:12:37 2018

Use inset anchor bounds for toolbar buttons

Adds a Views::GetAnchorBoundsInScreen() method. This defaults to
GetBoundsInScreen() but can be overridden for views whose visual
bounds differ from their actual bounds. This is the case for toolbar
buttons on Touch (that provide larger hit targets than their visual
size).

This method is then used for the menus spawned by AppMenuButton and
ToolbarButton as well as BubbleDialogDelegate which allows these
specific menus and BubbleDialogDelegates in general to be better
aligned visually to their anchors.

Bug:  chromium:800372 , chromium:869928
Change-Id: I9c516d8a7ecb6c922e347d4db61ce1db72c71be7
Reviewed-on: https://chromium-review.googlesource.com/1152525
Commit-Queue: Peter Boström <pbos@chromium.org>
Reviewed-by: Trent Apted <tapted@chromium.org>
Reviewed-by: Bret Sepulveda <bsep@chromium.org>
Cr-Commit-Position: refs/heads/master@{#579861}
[modify] https://crrev.com/6f92518b820eb652ed4d03512728f1d032042a96/chrome/browser/ui/views/toolbar/app_menu.cc
[modify] https://crrev.com/6f92518b820eb652ed4d03512728f1d032042a96/chrome/browser/ui/views/toolbar/browser_app_menu_button.cc
[modify] https://crrev.com/6f92518b820eb652ed4d03512728f1d032042a96/chrome/browser/ui/views/toolbar/browser_app_menu_button.h
[modify] https://crrev.com/6f92518b820eb652ed4d03512728f1d032042a96/chrome/browser/ui/views/toolbar/toolbar_action_view.cc
[modify] https://crrev.com/6f92518b820eb652ed4d03512728f1d032042a96/chrome/browser/ui/views/toolbar/toolbar_action_view.h
[modify] https://crrev.com/6f92518b820eb652ed4d03512728f1d032042a96/chrome/browser/ui/views/toolbar/toolbar_button.cc
[modify] https://crrev.com/6f92518b820eb652ed4d03512728f1d032042a96/chrome/browser/ui/views/toolbar/toolbar_button.h
[modify] https://crrev.com/6f92518b820eb652ed4d03512728f1d032042a96/ui/views/bubble/bubble_dialog_delegate.cc
[modify] https://crrev.com/6f92518b820eb652ed4d03512728f1d032042a96/ui/views/bubble/bubble_dialog_delegate.h
[modify] https://crrev.com/6f92518b820eb652ed4d03512728f1d032042a96/ui/views/view.cc
[modify] https://crrev.com/6f92518b820eb652ed4d03512728f1d032042a96/ui/views/view.h
Project Member

Comment 2 by bugdroid1@chromium.org, Aug 14 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d83d2aa04e6a9f080761187900b0108820467005

commit d83d2aa04e6a9f080761187900b0108820467005
Author: Charlene Yan <cyan@chromium.org>
Date: Tue Aug 14 15:51:46 2018

Remove set_anchor_view_insets inside src/chrome/browser/ui.

Will still need to remove calls inside src/ash before it can be fully removed from src/ui/views/bubble/bubble_dialog_delegate_view.h

Bug: 869928
Change-Id: I0444a7c3d6440d7be1dfe99c26996f2524d8f29a
Reviewed-on: https://chromium-review.googlesource.com/1173321
Reviewed-by: Peter Boström <pbos@chromium.org>
Reviewed-by: Bret Sepulveda <bsep@chromium.org>
Commit-Queue: Charlene Yan <cyan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#582925}
[modify] https://crrev.com/d83d2aa04e6a9f080761187900b0108820467005/chrome/browser/ui/views/content_setting_bubble_contents.cc
[modify] https://crrev.com/d83d2aa04e6a9f080761187900b0108820467005/chrome/browser/ui/views/global_error_bubble_view.cc
[modify] https://crrev.com/d83d2aa04e6a9f080761187900b0108820467005/chrome/browser/ui/views/location_bar/location_bar_bubble_delegate_view.cc
[modify] https://crrev.com/d83d2aa04e6a9f080761187900b0108820467005/chrome/browser/ui/views/location_bar/location_bar_bubble_delegate_view.h
[modify] https://crrev.com/d83d2aa04e6a9f080761187900b0108820467005/chrome/browser/ui/views/network_profile_bubble_view.cc
[modify] https://crrev.com/d83d2aa04e6a9f080761187900b0108820467005/chrome/browser/ui/views/outdated_upgrade_bubble_view.cc
[modify] https://crrev.com/d83d2aa04e6a9f080761187900b0108820467005/chrome/browser/ui/views/page_info/page_info_bubble_view_base.cc
[modify] https://crrev.com/d83d2aa04e6a9f080761187900b0108820467005/chrome/browser/ui/views/permission_bubble/permission_prompt_impl.cc

Comment 3 by pbos@chromium.org, Aug 14

Cc: cyan@chromium.org
Owner: tapted@chromium.org
tapted@ can you or someone on the CrOS side take over removal of set_anchor_view_insets in ash/? I think they should be fairly quick for someone more familiar with ash to get rid of the last 9 instances.

Comment 4 by tapted@chromium.org, Aug 14

Labels: OS-Chrome
ooh nice cleanup in #c2 :). I'm happy to poke around in ash (although I'm currently working mostly with ChromeOS's js UI, rather than native..)

Comment 5 by pbos@chromium.org, Aug 15 

Thanks! :)

Comment 6 by tapted@chromium.org, Aug 23 

shelf_tooltip_bubble.cc changes (https://chromium-review.googlesource.com/c/chromium/src/+/1186218)

(This bubble still positions as though it has an arrow pointing to the item, but that got removed ages ago [before m61]).
before.png
42.7 KB View Download
after.png
45.0 KB View Download

Comment 7 by tapted@chromium.org, Aug 30

Blockedon: 879048
►

Sign in to add a comment