New issue
Advanced search Search tips

Issue 712347 link

Starred by 5 users

Issue metadata

Status: Fixed
Owner:
Closed: May 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 2
Type: Bug

Blocking:
issue 671916



Sign in to add a comment

[MacViewsBrowser] Can't tab into bookmarks bar

Project Member Reported by shrike@chromium.org, Apr 17 2017

Issue description

Chrome Version: 60.0.3072.0 (MacViews)
OS: macOS 10.12

What steps will reproduce the problem?
(1) Repeatedly press the Tab key until you focus the app menu button
(2) Press Tab

What is the expected result?
The leftmost bookmark item in the bookmarks bar should be focused

What happens instead?
Focus moves into web content
 

Comment 1 by tapted@chromium.org, Apr 18 2017

Blocking: 671916
The bookmarks bar uses an accessibility mode on Windows that requires `F6` rather than <tab> to move into it. MacViews uses parts of this plumbing to do its own thing but doesn't use the F6 thing.

One part of the fix for this might be for AccessiblePaneView::GetPaneFocusTraversable() to always return null on Mac: this ensures focus doesn't get "trapped" within the bookmarks bar. But something else is probably needed.

A simpler approach might simply be to completely hide AccessiblePaneView on Mac: have BookmarkBarView just inherit from views::View instead. I don't think there's a use case for AccessiblePaneView on Mac. Changing view hierarchies with #ifdefs usually feels yuck, but AccessiblePaneView encapsulates its stuff quite neatly, so I think this will work.
Labels: M-68
[Bulk Edit]
Applying M-68 milestone per email discussion with ellyjones@. Pls change it if milestone is incorrectly applied. 
Labels: -M-68 Target-68
Owner: lgrey@chromium.org
Status: Assigned (was: Available)
MacViews triage: assigning to lgrey@ for M-68.

Comment 4 by gov...@chromium.org, Mar 27 2018

Labels: M-68

Comment 5 by gov...@chromium.org, Mar 29 2018

** Bulk Edit **

FYI: Starting 04/13 M68 will be in canary, M68 Dev promotion will be on 04/26.

Comment 6 by gov...@chromium.org, Apr 25 2018

Pls mark the bug as fixed if CL is landed in trunk and nothing else is pending. Thank you.

Comment 7 by lgrey@chromium.org, Apr 30 2018

Labels: Sprint-2
Any progress here?
Project Member

Comment 9 by bugdroid1@chromium.org, May 10 2018

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

commit e3d543507e23d66d3b923670b1ac132b55dcd887
Author: Leonard Grey <lgrey@chromium.org>
Date: Thu May 10 15:56:00 2018

Implement |GetChildrenInZOrder| in browser and top container views

Currently, we ensure that the bookmark bar view is always the first
child view of its parent to ensure that toolbar inkdrops are drawn on
top of it. Unfortunately, this breaks keyboard traversal order.

This change implements |GetChildrenInZOrder| on the two views that
host the bookmark bar: browser view and top container view. This
ensures that the bookmark bar is painted first, removing the need
to manipulate child view order.

Bug:  712347 
Change-Id: If8f2aabe816a86b7ec29ff41779783c0f05a6393
Reviewed-on: https://chromium-review.googlesource.com/1052854
Commit-Queue: Leonard Grey <lgrey@chromium.org>
Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Cr-Commit-Position: refs/heads/master@{#557537}
[modify] https://crrev.com/e3d543507e23d66d3b923670b1ac132b55dcd887/chrome/browser/ui/views/frame/browser_view.cc
[modify] https://crrev.com/e3d543507e23d66d3b923670b1ac132b55dcd887/chrome/browser/ui/views/frame/browser_view.h
[modify] https://crrev.com/e3d543507e23d66d3b923670b1ac132b55dcd887/chrome/browser/ui/views/frame/browser_view_unittest.cc
[modify] https://crrev.com/e3d543507e23d66d3b923670b1ac132b55dcd887/chrome/browser/ui/views/frame/top_container_view.cc
[modify] https://crrev.com/e3d543507e23d66d3b923670b1ac132b55dcd887/chrome/browser/ui/views/frame/top_container_view.h

Project Member

Comment 10 by bugdroid1@chromium.org, May 11 2018

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

commit aed427b076ba209b24b49a72ce9d7c4637984029
Author: Leonard Grey <lgrey@chromium.org>
Date: Fri May 11 13:21:01 2018

Revert "Implement |GetChildrenInZOrder| in browser and top container views"

This reverts commit e3d543507e23d66d3b923670b1ac132b55dcd887.

Reason for revert: Broke detached bookmark bar

Original change's description:
> Implement |GetChildrenInZOrder| in browser and top container views
> 
> Currently, we ensure that the bookmark bar view is always the first
> child view of its parent to ensure that toolbar inkdrops are drawn on
> top of it. Unfortunately, this breaks keyboard traversal order.
> 
> This change implements |GetChildrenInZOrder| on the two views that
> host the bookmark bar: browser view and top container view. This
> ensures that the bookmark bar is painted first, removing the need
> to manipulate child view order.
> 
> Bug:  712347 
> Change-Id: If8f2aabe816a86b7ec29ff41779783c0f05a6393
> Reviewed-on: https://chromium-review.googlesource.com/1052854
> Commit-Queue: Leonard Grey <lgrey@chromium.org>
> Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#557537}

TBR=ellyjones@chromium.org,lgrey@chromium.org

Change-Id: I43286f3f1a771a8835f72d28c2333f67c9ef6b7b
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  712347 
Reviewed-on: https://chromium-review.googlesource.com/1054594
Reviewed-by: Leonard Grey <lgrey@chromium.org>
Commit-Queue: Leonard Grey <lgrey@chromium.org>
Cr-Commit-Position: refs/heads/master@{#557857}
[modify] https://crrev.com/aed427b076ba209b24b49a72ce9d7c4637984029/chrome/browser/ui/views/frame/browser_view.cc
[modify] https://crrev.com/aed427b076ba209b24b49a72ce9d7c4637984029/chrome/browser/ui/views/frame/browser_view.h
[modify] https://crrev.com/aed427b076ba209b24b49a72ce9d7c4637984029/chrome/browser/ui/views/frame/browser_view_unittest.cc
[modify] https://crrev.com/aed427b076ba209b24b49a72ce9d7c4637984029/chrome/browser/ui/views/frame/top_container_view.cc
[modify] https://crrev.com/aed427b076ba209b24b49a72ce9d7c4637984029/chrome/browser/ui/views/frame/top_container_view.h

Comment 11 by lgrey@chromium.org, May 11 2018

Turns out that views with layers (which BookmarkBarView is) ignore z-order!

Comment 12 by lgrey@chromium.org, May 16 2018

Cc: lgrey@chromium.org nyerramilli@chromium.org ellyjo...@chromium.org rbasuvula@chromium.org
 Issue 843087  has been merged into this issue.
Can this be marked as fixed if nothing else is pending?
Project Member

Comment 14 by bugdroid1@chromium.org, May 29 2018

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

commit d17eb6abdecdf64b2dc2d624ceba91362f4e232b
Author: Leonard Grey <lgrey@chromium.org>
Date: Tue May 29 14:28:24 2018

Don't force bookmark bar to the back in Material Refresh

Currently, we ensure that the bookmark bar is ordered behind the toolbar
in BrowserView's children array to ensure that ink drop ripples from the
toolbar can draw over the bookmark bar.

Since this is accomplished via child view order, it also affects keyboard
traversal, meaning that in the typical case, the bookmarks bar comes
*before* the tabstrip and the toolbar, rather than after as users would
expect. The ordering issue can't be resolved via |GetChildrenInZOrder|,
since layer-backed views (like the bookmark bar) always draw on top of
non-layer-backed views.

In MD refresh, ink drop ripples are masked by the buttons, so they no
longer need to draw on top of the bookmark bar. This allows us to stop
shuffling the bookmark bar to the back. In the case of a detached bookmark
bar, we will still need to order the bookmark bar behind the infobar container
to ensure the infobar container's bottom shadow can draw over the bookmark bar.
This makes for a slightly out of order traversal, but it's a special case, and
still better than the status quo.

Bug:  712347 
Change-Id: I908abdd27d6b4f102b3164e28ffae80275a97a99
Reviewed-on: https://chromium-review.googlesource.com/1066478
Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Commit-Queue: Leonard Grey <lgrey@chromium.org>
Cr-Commit-Position: refs/heads/master@{#562417}
[modify] https://crrev.com/d17eb6abdecdf64b2dc2d624ceba91362f4e232b/chrome/browser/ui/views/frame/browser_view.cc
[modify] https://crrev.com/d17eb6abdecdf64b2dc2d624ceba91362f4e232b/chrome/browser/ui/views/frame/browser_view_unittest.cc

Comment 15 by lgrey@chromium.org, May 30 2018

Status: Fixed (was: Assigned)

Sign in to add a comment