Realized my Canary was not up-to-date, but this still reproduces after updating to 59.0.3043.0.

A coworker checked on Windows and it doesn't reproduce there, so I guess this is Mac-only.

This appears to only occur when Chrome is in fullscreen mode when there are multiple tabs. It appears possibly the full screen mode on Mac is slightly different because I have multiple tabs and it seems to be preventing these fields from working.

Seems like a pretty good regression to me that we should allow closing a tab.

Ah, yes, I should've mentioned that this was in a fullscreen window. This issue reproduces for me in fullscreen/canary, but not windowed/canary, fullscreen/stable, or windowed/stable.

This is an expected behavior, which is called Browser Keyboard Lock. The intent to implement is at https://goo.gl/4tJ32G.
Simply speaking, this feature gives all browser reserved keyboard events to the web page if it's in full screen mode. And it does not relate to preventDefault().

Missing the URL of the change, https://codereview.chromium.org/2698013004/
There are several changes before or after, but this one shows the key change I have made to impact the browser behavior in full screen mode.

The problem is it is broken when you maximize the window on Mac. I believe Mac calls this full screen mode but it isn't the same as full screen mode as for other tabs.

The maximized/fullscreen mode on Mac has a tab strip.

I'd expect this to only work this way when there isn't a tab strip.

Thank you for the feedback. We have received an exactly same feedback at  http://crbug.com/693613 . I believe both of you have the same feeling that once the toolbar is visible, Chrome should not treat the web page as fullscreen. But unfortunately now we do not have a third states in Chrome for MacOS.
Would you mind to have a quick look at the attached bug above? If it's the same, we can merge them into one.

Adding the blink owners that approved this. I'd expect a maximized window on mac not to have this behavior but perhaps that is me.

I agree with the conclusion of Comment 7. We should not allow the behavior when
the tab strip is visible.

As for whether a maximized window on mac counts as
fullscreen - maybe it does as long as there is no browser chrome?

Yes, I think this is the same as 693613. Maximizing a window on OS X is different from fullscreen; it doesn't handle Esc (nor should it).

It's also possible to open a window with just a location bar and no tab strip [1], and it's even more difficult to close the window in that case. I think the best condition for OS X windows would be "if you can't press Esc to leave it, it's not 'fullscreen'". Fullscreen windows get a warning about how to leave; maximized windows do not.

[1] window.open("https://sfiera.net/~sfiera/prevent-default.html", "", "toolbar=no");

Attaching comments from  http://crbug.com/693613  for backlogging.

When the toolbar is visible in fullscreen mode, websites should not be able to override the browser's keyboard shortcuts. See  Issue 692077  for back story.

Assigning to dominickn@ for now (please reassign as appropriate).

 Issue 693867  has been merged into this issue.

 Issue 693938  has been merged into this issue.

 Issue 694667  has been merged into this issue.

 Issue 694920  has been merged into this issue.

Any reason this bug is restrict-view-google?

 Issue 693613  has been merged into this issue.

Nope, I just didn't know how severe it would be considered.

When the toolbar is visible in fullscreen on the Mac, keyboard shortcuts should be active.

It also seems that this example prevents me from using the fullscreen keyboard shortcut to enter fullscreen - I will file a separate bug about that.

I'm fairly sure we decided in  crbug.com/693613  that this exact situation should result in not allowing browser shortcuts to be captured. Implementation-wise, we need to look at how to distinguish HTML5-type "no-toolbar" fullscreen from macOS's "app fullscreen" where the browser chrome is still visible.

I had a brief look and the key property is FullscreenToolbarStyle::TOOLBAR_NONE on the Mac BrowserWindowController. We need to expose this through BrowserWindowCocoa so that the BrowserCommandController can know on Mac that we're in a toolbar-less fullscreen mode. Doing this will probably need a new method on BrowserWindow (IsFullscreenWithNoToolbar()) that returns IsFullscreen() on all platforms but Mac, where BrowserWindowCocoa checks [BrowserWindowController hasToolbar] as well). We'll need a new method since BrowserCommandController only has access to a BrowserWindow abstract type.

shrike@: WDYT?

shrike@/zijiehe@: I am uncomfortable implementing all of the above for M58 given that we've already branched. I think we need to revert browser keyboard lock for M58, add the new methods to the various classes, and reland for M59. WDYT?

zijiehe@: I think we need a feature flag for browser keyboard lock so that we can remotely test and control it. Part of doing the above refactoring should also introduce this feature flag and gate browser keyboard lock behind it.

Re #9: my understanding on the intent was that this applied only when no browser chrome was visible.  If we want to change the behavior in Mac Fullscreen mode when the tabstrip is visible, IMHO that should have a separate intent discussion.

Re: c#17, that all sounds good.

I agree on c#18, currently the intent discusses the behavior when browser is full-screen. And the implementation strictly follows the intent.
If we are talking about the different full-screen modes on Mac OS, or whether the current full-screen mode on Mac OS is really full-screen, we need a different intent. It looks more like a Mac OS specific issue instead of the unclearness of this intent. In either case, adding a flag for Mac OS only rather than this feature seems like the right thing to do.

c#18, c#20: agreed, the intent explicitly said that browser keyboard lock would apply when no browser chrome was visible, and we definitely don't want to apply it when we still see the tab strip.

Strictly speaking, the implementation does not currently follow the intent because Mac app-fullscreen is a "maximise" operation, not fullscreen (there is still browser chrome), but the implementation of BrowserWindowCocoa::IsFullscreen() does not capture this distinction.

zijiehe@: are you able to follow up on the plan outlined in c#17? We'd need to:

1. land a CL to fully disable browser keyboard lock, and merge that to M58
2. land a feature flag controlling it
3. land the new IsFullscreenWithNoToolbar() method on BrowserWindow and its implementations on BrowserView and BrowserWindowCocoa
4. reland browser keyboard lock behind the flag and checking IsFullscreenWithNoToolbar() for M59

It looks like on Mac OS, there is no full-screen mode as other platforms. I suggest we simply revert the change on Mac OS only instead of impacting other platforms.

You can still invoke HTML5 fullscreen on Mac with no browser chrome. When in HTML5 fullscreen on Mac, it is completely within the intent to allow these shortcuts to be captured. The problem is that BrowserWindow::IsFullscreen() does not distinguish between no-toolbar and with-toolbar fullscreen on Mac, and that is what needs to be fixed.

Additionally, I don't really feel that it makes much sense to only have this feature available on desktop platforms except Mac in M58 - delaying to M59 does not really have that much impact. We should implement it right on all platforms so that it is consistent, and that means reverting everywhere for M58.

Re: c#21 and c#22, app fullscreen is not the same as maximizing a window. When you place a window in app fullscreen, there are a couple things that happen:

1. The window is placed into its own "Space" (essentially a desktop separate from other desktops). You can swipe between desktops.

2. The window's "toolbar" (in Chrome this is the top chrome) is hidden, as is the menu bar. Placing the mouse at the top of the screen animates both into view, and moving the mouse away animates them out. In Chrome Mac (and other Mac apps) you can force the top chrome to always remain visible in fullscreen.

So really the only wrinkle with fullscreen on the Mac is Chrome cannot assume that being in fullscreen mode means that the top chrome is hidden. You have to always check, because it may have been forced to always be visible, or it may be temporarily visible based on the mouse's location.

To avoid any confusing, let me summarize all the discussions before,

- On Mac OS, there are two full-screen modes, 1. tab stripe + menu bar is accessible, 2. only menu bar is accessible. (Let's forget about the implementation details, but only the user experience.)

- Both two full-screen modes do not show up on other platforms (Linux (X11) / Windows / ChromeOS), i.e. on other platforms, there is a third full-screen mode, both menu bar and tap stripe are not accessible.

- Chris thinks if the tab stripe is accessible, the browser shortcuts should take effect.

- Jayson thinks if the menu bar is visible, the browser shortcuts should take effect.

Please correct me if anything misunderstood.

Expanding my example before a bit, to trap and log all keys it is able to, and enter [HTML5] fullscreen with "f":

data:text/html,<script>window.addEventListener("keydown",function(e)%7Bdocument.body.innerHTML+=e.key+"<br>";if(e.key=="f")document.body.webkitRequestFullscreen();e.preventDefault()%7D);</script>

Here's what I would expect, starting from a regular window, and assuming "Always Show Toolbar in Full Screen" is not initially enabled:

1. Esc -> logged (Esc)
2. ⌘W -> closes window

1. F -> logged (f), enters [HTML5] full-screen
2. ⌘W -> logged (Meta, w)
3. Esc -> leaves fullscreen, returns to windowed mode
4. ⌘W -> closes window

1. ⌃⌘F -> maximizes window (called "Full Screen" on macOS)
2. Esc -> logged (Escape)
3. ⌘W -> closes window

1. ⌃⌘F -> maximizes window
2. F -> logged (f), enters [HTML5] full-screen
3. ⌘W -> logged (Meta, w)
4. Esc -> leaves full-screen, returns to maximized mode
5. Esc -> logged (Escape)
6. ⌘W -> closes window

Here's what I'm not sure about:

1. ⌃⌘F -> maximizes window
2. ⇧⌘F -> hides toolbar
3. ⌘W -> logged (Meta, w)
4. Esc -> logged (Escape)
5. ⇧⌘F -> logged (Shift, Meta, w)
6. In "View" menu, deselect "Always Show Toolbar in Full Screen"
7. ⌘W -> closes window

I think this is the proposed resolution of this bug, but it feels weird to me that there's no symmetry of ⇧⌘F to enter/exit blocked mode. The suggestion I gave earlier was to treat fullscreen initiated by the browser specially, e.g.:

1. ⌃⌘F -> maximizes window
2. ⇧⌘F -> hides toolbar
3. ⌘W -> closes window

1. F -> logged (f), enters [HTML5] full-screen
2. ⇧⌘F -> logged (Shift, Meta, w)
3. Esc -> leaves full-screen
4. ⌘W -> closes window

But it also feels weird to me that the site can enter blocked mode and I can't. I'm not sure it's any better.

One rationale for allowing the site to enter a mode where it gets all keys, but not to allow the user to do so for an arbitrary site, is that most sites don't need these keys. Those that do can use the requestFullscreen() API.

At that point we're starting to impinge upon the requestSystemKeyboardLock() API that we're proposing separately, but if you're looking for a rationale for your final proposal, it might help make it more palatable.

So if my understanding is correct, your preference is similar as Jayson. i.e. If the menu bar (or toolbar in your comment) is visible, the browser shortcuts should take effect.
And you also think this is still not that reasonable, because this full-screen mode is only triggerable by web page instead of users.

I believe this is the reason I do not think this feature works on Mac OS for now: there is no comparable full-screen mode on Mac OS. A user initiated full-screen (⌃⌘F) shows both tab stripe and may show menu bar, a page initiated full-screen (HTML5) may show menu bar.

Meanwhile I have not found an easy way to test the visibility of menu. (http://stackoverflow.com/questions/27584963/get-window-values-under-mouse/27589911#27589911 may be a choice, but it involves accessibility APIs, and cannot detect the configuration Jayson mentioned.)
So Dominick, what you suggested may not be workable. Reverting this feature on other platforms cannot help to make the thing work better on Mac OS.

We may enable this feature back on Mac OS once we have a reliable way to detect the visibility of menu bar. i.e. On Mac OS, there are three full-screen states, nothing else visible, menu bar visible, tab stripe accessible. But this seems out of the scope of this intent to implement, and also impacts only the design of Mac OS.

What do you think?

> I believe this is the reason I do not think this feature works on Mac OS for now: there is no comparable full-screen mode on Mac OS. A user initiated full-screen (⌃⌘F) shows both tab stripe and may show menu bar, a page initiated full-screen (HTML5) may show menu bar.

Still not quite right.

After a user-initiated fullscreen mode (⌃⌘F), the tabstrip/menu bar may or may not be visible. They are visible if the user has checked "Always Show Toolbar in Fullscreen". They are also visible if the user moves the mouse to the top of the screen, regardless of the "Always Show Toolbar in Fullscreen" setting.

The toolbar and menu bar are always visible together - it is never the case that the menu bar is visible without the toolbar (and vice versa). So you only need to check for toolbar visibility to known when to enable keyboard shortcuts.

---

In content-initiated fullscreen, the toolbar is always hidden. The menu bar is hidden unless the user moves the mouse to the top of the screen, in which case the menu bar animates into view.

Sorry, it was a typo, yes, a user initiated full-screen (⌃⌘F) may show both tab stripe and menu bar.

After discussion with Jamie (JamieWalch@), we may consider following three solutions for browser keyboard lock.
1. Enabling this feature only in web page initiated full-screen mode. When the full-screen mode is triggered by web page, it's more likely the page would prefer to receive more keys. Otherwise, the web page should expect some of the key combinations may be consumed by browser.
2. Same as #c38. There is no fully comparable full-screen mode on Mac OS, thus disabling this feature on Mac OS would be a simple approach.
3. Revert the changes entirely and wait for System Keyboard Lock.

Since System Keyboard Lock (http://go/chrome-system-keyboard-lock) feature is on-going. It seems not worthy to implement a too complexity logic on Mac OS to guess both whether the web page expects the key combinations (by detecting whether the full-screen is initiated by the web page) and the users' expectations (by detecting whether the menu-bar is visible).

I'm not quite sure where this discussion is going. :)

shrike@ and I seem to be in agreement regarding the plan of action laid out in c#18. It's entirely driven by what the user sees (toolbar or no toolbar), not by how the user ended up there. And it means that browser keyboard lock is fine as is on other desktop platforms, with an extra wrinkle on Mac that only enables it if in fullscreen with no toolbar visible. A big motivation for this feature was that being in fullscreen with no browser UI visible is a signal to / from the user that they are not in Chrome and that their shortcuts are permitted to be overridden. c#18 is consistent with this motivation, and that's what I think we should do.

Restricting the feature to only web-page initiated fullscreen makes the use asymmetrical and undermines the motivation (why is user-initiated fullscreen different?). System keyboard lock is a much heavier API, and it would be sad to only have the browser keyboard lock available there.

I think the problem is that there are a lot of references to different bugs so chasing down the discussion is difficult. If I'm reading it correctly, the relevant comment is https://bugs.chromium.org/p/chromium/issues/detail?id=692077#c10:

> Basically, if the top chrome is showing my keyboard shortcuts should work.

I think this translates to disabling browser shortcuts on Mac always for HTML full-screen, but for ⌃⌘F to do so only if the "View > Always Show Toolbar in Full Screen" option is disabled. Does everyone agree with that?

> I think this translates to disabling browser shortcuts on Mac always for HTML full-screen, but for ⌃⌘F to do so only if the "View > Always Show Toolbar in Full Screen" option is disabled. Does everyone agree with that?

No - for ⌃⌘F, disable browser shortcuts if the toolbar is visible. So basically disable browser shortcuts when the toolbar hides, and enable them when it appears.

Then as I have mentioned above, I do not think we have an easy and reliable way to detect the visibility of the menu-bar, which is a showstopper of browser keyboard lock feature.

I'm confused - why do you keep mentioning the menu bar? The menu bar has nothing to do with solving this problem.

If the call is SetBrowserShortcutsEnabled(), say, right now you are calling SetBrowserShortcutsEnabled(false) when you enter fullscreen? Instead on the Mac you would just call SetBrowserShortcutsEnabled(false) when the toolbar animates out of view, and SetBrowserShortcutsEnabled(true) when it animates back in. Is it more complicated than that?

Re #c34: Are you suggesting that if the toolbar is set to auto-hide, then browser shortcuts would be disabled unless the user moves the mouse to the top of the screen in order to show it, at which point they would work? That seems like strange UX--the user has to take their hand off the keyboard in order to move the mouse to show the tool-bar, only to move back to the keyboard to use a shortcut. Why wouldn't they simply use the mouse to do whatever they were trying to do at that point?

I'm not a Mac user or a UX expert, so if someone has reason to believe that this is a common workflow, or if I've missed something, then I have no objection to implementing it this way, but it seems counter-intuitive. It's certainly not obvious to me that it's better than tying it to the "always show" state as suggested in #c33.

Re #c36, I believe this bug originally talks about the HTML initiated full-screen, which does not have tab strip (or you may call it toolbar), but only menu bar. (Please refer to the attachments for anything that may be confusing.)

Deleted: chrome-tab-strip.png 10.0 KB

	chrome-tab-strip.png 10.0 KB View Download

Deleted: menu-bar-macos-sierra.jpg 11.8 KB

	menu-bar-macos-sierra.jpg 11.8 KB View Download

Hi, has the revert for this landed yet? I'm still seeing the broken behavior on 59.0.3050.0 Canary.

(I'm not sure why the menu bar is relevant either. It's not visible by default in any full-screen mode, either maximized or HTML-initiated)

Re: c#37 - making the toolbar/tabstrip visible does not imply I intend to invoke a menu command. For example, the only way to make the omnibox visible in fullscreen is to move the mouse to the top of the screen. Once it's visible, I might Cmd-L to select the URL, start typing a URL, then Cmd-Option-Left Arrow to visit the previous tab, Cmd-W to close that tab, and so on.

Re: c#38 - no, it's not about HTML fullscreen. The user went to the NYT crossword page and ⌃⌘F-fullscreened the window. Please see c#4, as well as my  Issue 693613  which was merged into this issue.

It seems like there's still confusion about what needs to happen, and when - shall we GVC to all get on the same page?

No need for GVC, I don't think--your explanation makes sense. We'll revert the original change for all platforms and then work on the refinement for Mac.

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

commit e84b99c44420c5214aaf0d6b095130f5f6535a4e
Author: zijiehe <zijiehe@chromium.org>
Date: Sat Mar 25 04:46:35 2017

Revert change http://crrev.com/3c7af99a93f4b4837b2fbee5cb66697f66ccf241/

According to the discussion in  http://crbug.com/702251 , we decide to revert this
change on all platforms, and propose a more comprehensive change on Mac OS
during M59.

BUG= 702251 ,  680809 

Review-Url: https://codereview.chromium.org/2778523002
Cr-Commit-Position: refs/heads/master@{#459639}

[modify] https://crrev.com/e84b99c44420c5214aaf0d6b095130f5f6535a4e/chrome/browser/ui/browser_command_controller.cc
[modify] https://crrev.com/e84b99c44420c5214aaf0d6b095130f5f6535a4e/chrome/browser/ui/browser_command_controller_unittest.cc

Change https://codereview.chromium.org/2778523002/ (in #c42) needs to be merged to M58.

This bug requires manual review: Reverts referenced in bugdroid comments after merge request.
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop)

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

Working again in the latest Canary. Thanks!

:) I am merging it into M58.

Before approving this for M58 Beta merge, can you please confirm if this has been well tested in Canary, stability looks good, and if there are automated test cases already created?

This change reverts another, and there are test cases covered. Meanwhile Chris (sfiera@) verified it worked as expected on Canary.

Thanks - approving it for M58 Beta. Thanks for confirming. 

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

commit 32d6e978b9cba1260ceace176d3f6e9eae199ef7
Author: Jamie Walch <jamiewalch@chromium.org>
Date: Tue Mar 28 19:15:00 2017

Revert change http://crrev.com/3c7af99a93f4b4837b2fbee5cb66697f66ccf241/

According to the discussion in  http://crbug.com/702251 , we decide to revert this
change on all platforms, and propose a more comprehensive change on Mac OS
during M59.

BUG= 702251 ,  680809 

Review-Url: https://codereview.chromium.org/2778523002
Cr-Commit-Position: refs/heads/master@{#459639}
(cherry picked from commit e84b99c44420c5214aaf0d6b095130f5f6535a4e)

Review-Url: https://codereview.chromium.org/2778063004 .
Cr-Commit-Position: refs/branch-heads/3029@{#457}
Cr-Branched-From: 939b32ee5ba05c396eef3fd992822fcca9a2e262-refs/heads/master@{#454471}

[modify] https://crrev.com/32d6e978b9cba1260ceace176d3f6e9eae199ef7/chrome/browser/ui/browser_command_controller.cc
[modify] https://crrev.com/32d6e978b9cba1260ceace176d3f6e9eae199ef7/chrome/browser/ui/browser_command_controller_unittest.cc

Verified this issue on Mac OS 10.12.3 using chrome latest Beta M58-58.0.3029.41 by following steps mentioned in the original comment. After following the step-1 comment observed the tab gets closed by pressing command+w as expected when the browser switched to full screen. Hence adding TE-Verified label.

Thanks!

Deleted: 702251.mp4 825 KB

	702251.mp4 825 KB View Download

Thank you Bibin.
If no other comments, I will close this issue. And for the following implementation on Mac OS, please use  issue 680809  to track.

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

commit 68cd3dc22ddbfddfdcf252f42b129a12e2d39a8b
Author: zijiehe <zijiehe@chromium.org>
Date: Fri Apr 07 18:50:29 2017

Allow keyboard shortcuts can be captured by webpage when toolbar is not visible

After discussing in  http://crbug.com/702251 , we decide to enable browser
shortcuts when the tab-strip / toolbar is visible on Mac OS. There is no
comparable behavior on other platforms.

This change adds a BrowserWindow::IsToolbarShowing() function to indicate
whether the toolbar is showing up on the screen. BrowserWindowCocoa needs to
override this function, which has not been done yet in this change.

BUG= 680809 ,  702251 

Review-Url: https://codereview.chromium.org/2781633003
Cr-Commit-Position: refs/heads/master@{#462942}

[modify] https://crrev.com/68cd3dc22ddbfddfdcf252f42b129a12e2d39a8b/chrome/browser/ui/browser_command_controller.cc
[modify] https://crrev.com/68cd3dc22ddbfddfdcf252f42b129a12e2d39a8b/chrome/browser/ui/browser_command_controller_unittest.cc
[modify] https://crrev.com/68cd3dc22ddbfddfdcf252f42b129a12e2d39a8b/chrome/browser/ui/browser_window.h
[modify] https://crrev.com/68cd3dc22ddbfddfdcf252f42b129a12e2d39a8b/chrome/browser/ui/cocoa/browser_window_cocoa.h
[modify] https://crrev.com/68cd3dc22ddbfddfdcf252f42b129a12e2d39a8b/chrome/browser/ui/cocoa/browser_window_cocoa.mm
[modify] https://crrev.com/68cd3dc22ddbfddfdcf252f42b129a12e2d39a8b/chrome/browser/ui/views/frame/browser_view.cc
[modify] https://crrev.com/68cd3dc22ddbfddfdcf252f42b129a12e2d39a8b/chrome/browser/ui/views/frame/browser_view.h
[modify] https://crrev.com/68cd3dc22ddbfddfdcf252f42b129a12e2d39a8b/chrome/test/base/test_browser_window.cc
[modify] https://crrev.com/68cd3dc22ddbfddfdcf252f42b129a12e2d39a8b/chrome/test/base/test_browser_window.h