Ping? Adding immersive component to help get picked up by triage. This is a pretty bad experience for immersive mode users.

I'm not sure if there is a particular owner for immersive mode these days. Albert likely knows who can look at this.

Adding Immersive triage owners.

pkasting@ and jamescook@ any ideas about this?

Looks like the layout for the top-views of the browser frame is wrong. I think the bookmark bar is just a set of views. If you have "show bookmark bar" turned off, the NTP draws the views with a different style. I think there's some complexity around infobars (the yellow warning bars we used to use for translate, the --no-sandbox warning, etc.). It's been a while since I worked on this stuff, though -- pkasting or other omnibox folks might know more.

 Issue 859244  has been merged into this issue.

pbos@ could you please triage this?

How do I get to the location bar / top chrome to show up in fullscreen? Is that always visible on CrOS? F11 on Windows gives a fullscreen but show no top Chrome (only the detached bookmarks bar).

Hoping that our testers can repro and bisect here. +robliao@ this needs to be on our radar.

This happens only on ChromeOS. +estade@ I saw you were working on getting immersive fullscreen mode working on mash these days. It's probably related to this work. Could you please take a look at it?

I *think* the relevant code is in BrowserViewLayout::LayoutBookmarkAndInfoBars(): 

https://cs.chromium.org/chromium/src/chrome/browser/ui/views/frame/browser_view_layout.cc?type=cs&q=BrowserViewLayout::LayoutBookmarkAndInfoBars&sq=package:chromium&g=0&l=407

I have noticed this but my suspicion is that it's due to recent changes to top chrome for that UI refresh.

Issue 866670 has been merged into this issue.

Probably due to https://chromium-review.googlesource.com/c/chromium/src/+/1066478

Was the previous behavior that the bar showed below the toolbar, or that it didn't show at all? The change above should have only changed z-order, not y-position.

The detached bookmark bar should be visible. The top-views should slide down on top of the detached bookmark bar.

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

commit 043669abdd797467ab3aa37d6e9b4e28dd987372
Author: Leonard Grey <lgrey@chromium.org>
Date: Tue Jul 31 14:20:08 2018

Restore ordering detached bookmark bar to the back for ChromeOS

Background:
1. The bookmark bar has two states: "attached", where it is part of a single
block with the rest of the topchrome and "detached" (only shown on the
stock NTP) where it appears to be part of the web content.
2. In views toolkit, the logic order of a view's child views determines
both its focus traversal order and its z-order.
3. Previously, the bookmark bar was being forcibly ordered before the rest
of the topchrome to allow other topchrome views to draw elements (for example,
ink drops) on top of it.
4. The combination of 2 and 3 caused tab traversal to be out of order.
5. In Material Refresh, the issue with ink drops is no longer relevant, so
the reordering was removed.
6. Except that ChromeOS has an "immersive mode" which hides the top chrome by
default but allows it to slide down over the web content on mouse over.
7. Since the detached bookmark bar is supposed to look like web content,
it remains visible in immersive mode.
8. ...and since it's no longer z-ordered behind the rest of the topchrome,
the tabstrip and toolbar remain out of sight, since they're sliding
down behind the bookmark bar.

This change reenables reordering in the detached case for ChromeOS because
IMO there isn't a good solution to this situation in a world where traversal
order and z-order can't be separated.

Bug:  857546 
Change-Id: I4454ddb12bfcd9ab9b382c3455efb1b5ee5939ca
Reviewed-on: https://chromium-review.googlesource.com/1155656
Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Commit-Queue: Leonard Grey <lgrey@chromium.org>
Cr-Commit-Position: refs/heads/master@{#579396}
[modify] https://crrev.com/043669abdd797467ab3aa37d6e9b4e28dd987372/chrome/browser/ui/views/frame/browser_view.cc

The top container in immersive is still obscured behind the infobar container, if present. I would have thought BrowserView could fix this via GetChildrenInZOrder(), but that doesn't appear to work unless BrowserView itself has a layer, and even when BrowserView does have a layer, that for some reason puts the top container behind the web contents' layer.

Adding milestone and RB label, given that this affects the main full-screen mode accessible from the ChromeOS top-row of shortcut buttons.

estade@ is there a reliable way to get an infobar in immersive?

(and yes, GetChildrenInZOrder is useless when anything involved has a layer, as I discovered earlier in this process)

Re: getting an infobar: Run --no-sandbox ?

In addition to --no-sandbox, I get the missing google api keys infobar every time I launch a local build. If you're asking about showing an infobar after you're already in immersive, you can probably do that by installing a theme from the CWS while in immersive (there will be an undo infobar).

OK, got a repro with --no-sandbox, thanks! This isn't related to my change since it's the whole topchrome and not just the bookmark bar, so if someone with more CrOS context wants to take this that would be ideal. Unassigning for triage.

I would still expect it's a result of the material redesign. To pkasting@ for assignment

ping

Albert, can you find an owner?

I locally built a 70, 69, 68 and 67 chrome build and found: 
1) It was an issue at least since 67, see screenshot 1 (with "show bookmark bar" option turned off, fullscreen a browser window and then reveal the top of the window). The top view of the window is revealed, but the bookmark bar can't be seen. The behavior is the same with what we have in 68 and 70 (Tot)
2) In 69, the behavior was different, see screenshot 2 (same steps as above). The bookmark bar shows above the top chrome window, thus it's impossible to switch tabs). I think it's the CL in comment#16 that fixed it in 70.

So as a temporary fix, I will just merge the fix in comment#16 to M69. And for a long term fix (stack the detached bookmark bar right under the top chrome), I think someone from the Omnibox or bookmark team may need to take a look at it. 

Deleted: 67.png 48.2 KB

	67.png 48.2 KB View Download

Deleted: 69.png 35.7 KB

	69.png 35.7 KB View Download

Request merge CL in comment#16

This bug requires manual review: We are only 12 days from stable.
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

FYI - The screenshot from 67 is expected behavior. The new-tab-page has special behavior for bookmarks. They are part of the page content. The top-views are supposed to slide down on top of them. It's been that way since immersive fullscreen was first built.

The behavior in tip-of-trunk is still incorrect when an infobar is showing. (Despite trunk containing the CL in comment 16.) Run chrome --no-sandbox and enter fullscreen. Move the mouse to the top of the screen. The top views slide down *behind* the infobar and you can't switch tabs.

Backporting the CL in 16 sounds good to me, I just wanted to remind folks this won't fix everything.

Thanks James! 
- For the screenshot from 67, the screenshot shows a new-tab-page, but when top-view of the new-tab-page is revealed, the top-view slides down but the bookmark bar doesn't slide down (I assume it should also slide down to a position right below the top-view?). The bookmark bar is actually covered by the slided-down top-view. Is this expected behavior?

- For the info bar, I managed to repro using --no-sandbox as you described. The info bar covers the top-view, as what the bookmark bar did in screenshot 69. So I was wondering if it can also be fixed in the same way as the bookmark bar? lgrey@, is this something you are familiar with? (and btw: we probably should file a separate bug for this as it doesn't match this bug description)

xdai@ I think the difference here would be that the infobar has a "legitimate" reason to be ordered on top (since it needs to draw its shadow on the detached bar). 

View order is doing triple duty here (paint order in non-immersive, "depth"/layering in immersive mode, keyboard traversal) in ways that conflict with each other, so IMO something needs to be rethought.

The bookmark bar going under the top-views is consistent and correct IMHO, the bookmark bar in this mode is conceptually part of the new tab page, and not actually attached to the top views.

@xdai yes, the bookmark bar is not expected to slide down on the new tab page. That matched the behavior of macOS chrome at the time. However, macOS chrome has changed its behavior -- the top views don't slide off the screen at all on the new tab page. I think it would be OK to make the bookmarks bar slide down on the new tab page. I also think it would be OK to always keep the top-views visible on the new tab page, for consistency with Mac. It's more a UX question.

I have a potential fix here: https://chromium-review.googlesource.com/c/chromium/src/+/1192140

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

commit 7cf7a5216507181e4e05f72d6b57f6448f20161d
Author: Evan Stade <estade@chromium.org>
Date: Wed Aug 29 18:12:23 2018

Chrome OS: add top container to widget's overlay view during immersive

When the immersive reveal starts, move |top_container_| to the widget's
overlay view so it's on top of everything. The layout logic doesn't
change because it's handled by BrowserViewLayout and is agnostic to the
actual parent view, and because the overlay view has the same bounds as
the client view.

Some responsibility for immersive reveal init and shutdown is moved
from ImmersiveModeControllerAsh to the BrowserView.

Bug:  857546 
Change-Id: I6192810e411240b588cb1e82363b19cc30eb1edf
Reviewed-on: https://chromium-review.googlesource.com/1192140
Commit-Queue: Evan Stade <estade@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#587195}
[modify] https://crrev.com/7cf7a5216507181e4e05f72d6b57f6448f20161d/chrome/browser/ui/views/frame/browser_frame.cc
[modify] https://crrev.com/7cf7a5216507181e4e05f72d6b57f6448f20161d/chrome/browser/ui/views/frame/browser_frame.h
[modify] https://crrev.com/7cf7a5216507181e4e05f72d6b57f6448f20161d/chrome/browser/ui/views/frame/browser_non_client_frame_view.cc
[modify] https://crrev.com/7cf7a5216507181e4e05f72d6b57f6448f20161d/chrome/browser/ui/views/frame/browser_view.cc
[modify] https://crrev.com/7cf7a5216507181e4e05f72d6b57f6448f20161d/chrome/browser/ui/views/frame/browser_view.h
[modify] https://crrev.com/7cf7a5216507181e4e05f72d6b57f6448f20161d/chrome/browser/ui/views/frame/immersive_mode_controller_ash.cc

should be fixed, although that CL could potentially have bugs and get reverted. Punting from M69 because it's too risky of a change to merge this late in the game.

Thanks estade@! 
Are we still going to merge the CL in #16 to M69? It's the CL to fix the bookmark bar covering the tabstrip issue. 

Good point, sorry for co-opting the bug. I'll let lgrey@ and TPMs decide if 043669abdd797467ab3aa37d6e9 should be merged.

This bug requires manual review: We are only 5 days from stable.
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

govind@ WDYT re c#39? I think the change is safe, but agree with estade@ about merging so late

+ cindyb@(Chrome OS M69 release TPM) for M69 mere review.

Merge approved for CL in #16, M69.

This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

cindyb@, which branch head?

refs/branch-heads/3497

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

commit 3bbfacfdeafe89929fe5fbd3c6d50738b0823aec
Author: Leonard Grey <lgrey@chromium.org>
Date: Tue Sep 04 16:27:15 2018

Restore ordering detached bookmark bar to the back for ChromeOS

Background:
1. The bookmark bar has two states: "attached", where it is part of a single
block with the rest of the topchrome and "detached" (only shown on the
stock NTP) where it appears to be part of the web content.
2. In views toolkit, the logic order of a view's child views determines
both its focus traversal order and its z-order.
3. Previously, the bookmark bar was being forcibly ordered before the rest
of the topchrome to allow other topchrome views to draw elements (for example,
ink drops) on top of it.
4. The combination of 2 and 3 caused tab traversal to be out of order.
5. In Material Refresh, the issue with ink drops is no longer relevant, so
the reordering was removed.
6. Except that ChromeOS has an "immersive mode" which hides the top chrome by
default but allows it to slide down over the web content on mouse over.
7. Since the detached bookmark bar is supposed to look like web content,
it remains visible in immersive mode.
8. ...and since it's no longer z-ordered behind the rest of the topchrome,
the tabstrip and toolbar remain out of sight, since they're sliding
down behind the bookmark bar.

This change reenables reordering in the detached case for ChromeOS because
IMO there isn't a good solution to this situation in a world where traversal
order and z-order can't be separated.

Bug:  857546 
Change-Id: I4454ddb12bfcd9ab9b382c3455efb1b5ee5939ca
Reviewed-on: https://chromium-review.googlesource.com/1155656
Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Commit-Queue: Leonard Grey <lgrey@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#579396}(cherry picked from commit 043669abdd797467ab3aa37d6e9b4e28dd987372)
Reviewed-on: https://chromium-review.googlesource.com/1203551
Reviewed-by: Leonard Grey <lgrey@chromium.org>
Cr-Commit-Position: refs/branch-heads/3497@{#871}
Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753}
[modify] https://crrev.com/3bbfacfdeafe89929fe5fbd3c6d50738b0823aec/chrome/browser/ui/views/frame/browser_view.cc

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

commit 3bbfacfdeafe89929fe5fbd3c6d50738b0823aec
Author: Leonard Grey <lgrey@chromium.org>
Date: Tue Sep 04 16:27:15 2018

Restore ordering detached bookmark bar to the back for ChromeOS

Background:
1. The bookmark bar has two states: "attached", where it is part of a single
block with the rest of the topchrome and "detached" (only shown on the
stock NTP) where it appears to be part of the web content.
2. In views toolkit, the logic order of a view's child views determines
both its focus traversal order and its z-order.
3. Previously, the bookmark bar was being forcibly ordered before the rest
of the topchrome to allow other topchrome views to draw elements (for example,
ink drops) on top of it.
4. The combination of 2 and 3 caused tab traversal to be out of order.
5. In Material Refresh, the issue with ink drops is no longer relevant, so
the reordering was removed.
6. Except that ChromeOS has an "immersive mode" which hides the top chrome by
default but allows it to slide down over the web content on mouse over.
7. Since the detached bookmark bar is supposed to look like web content,
it remains visible in immersive mode.
8. ...and since it's no longer z-ordered behind the rest of the topchrome,
the tabstrip and toolbar remain out of sight, since they're sliding
down behind the bookmark bar.

This change reenables reordering in the detached case for ChromeOS because
IMO there isn't a good solution to this situation in a world where traversal
order and z-order can't be separated.

Bug:  857546 
Change-Id: I4454ddb12bfcd9ab9b382c3455efb1b5ee5939ca
Reviewed-on: https://chromium-review.googlesource.com/1155656
Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Commit-Queue: Leonard Grey <lgrey@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#579396}(cherry picked from commit 043669abdd797467ab3aa37d6e9b4e28dd987372)
Reviewed-on: https://chromium-review.googlesource.com/1203551
Reviewed-by: Leonard Grey <lgrey@chromium.org>
Cr-Commit-Position: refs/branch-heads/3497@{#871}
Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753}
[modify] https://crrev.com/3bbfacfdeafe89929fe5fbd3c6d50738b0823aec/chrome/browser/ui/views/frame/browser_view.cc