1477525 - The new bar while stay in place in full screen if you press CTRL + T

Status: RESOLVED FIXED

(Firefox :: General, defect, P1)

Description

[michaelziliang]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:62.0) Gecko/20100101 Firefox/62.0
Build ID: 20180719140244

Steps to reproduce:

1. Enter full screen
2. Press CTRL + T to open a new tab


Actual results:

The new bar while stay in place in full screen even after restarting Firefox


Expected results:

The new tab bar should have disapeared after a few seconds/moving your mouse of the new tab bar.

Comment 1

[Alice0775 White]

this relates to fullscreen, ni :xidorn

Comment 2

[Xidorn Quan [:xidorn] UTC+11]

So this is because when new tab is created, the focus is on the url bar, in which case we would not collapse the toolbar.

But we should still collapse it as far as the input loses the focus. I think I have some idea about how to fix it.

Comment 3

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Bug 1477525 part 1 - Merge the logic of _safeToCollapse into hideNavToolbox.

Review commit: https://reviewboard.mozilla.org/r/258664/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/258664/

Comment 4

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Bug 1477525 part 2 - Try hiding toolbox again when input in chrome loses the focus.

Review commit: https://reviewboard.mozilla.org/r/258662/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/258662/

Comment 5

[Sam Foster [:sfoster] (he/him)]

[triage] :xidorn, to clarify this is f11 / browser full screen, and the "new bar" is the main tab bar? 
Assigning p1 as this has a patch up for review.

Comment 6

[Xidorn Quan [:xidorn] UTC+11]

I think the "new bar" means the whole toolbox. I'm not completely sure but that feels like the only possible thing given the description.

Comment 7

[Dão Gottwald [::dao]]

Comment on attachment 8994077 [details]
Bug 1477525 part 2 - Try hiding toolbox again when input in chrome loses the focus.

https://reviewboard.mozilla.org/r/258662/#review269218

::: browser/base/content/browser-fullScreenAndPointerLock.js:606
(Diff revision 1)
> -        document.commandDispatcher.focusedElement.localName == "input") {
> +        focused.localName == "input") {
> +      // But try collapse the chrome again when it loses the focus.
> +      focused.addEventListener("blur", () => {
> +        // If we don't set any timeout here, the content may move before
> +        // user's click takes effect.
> +        setTimeout(() => this.hideNavToolbox(aAnimate), 300);

This is rather fragile as it assumes that a click will be done after 300ms. Technically I don't think there's any limit for how long a click can take. Is there a more robust way to detect this?

Comment 8

[Dão Gottwald [::dao]]

Comment on attachment 8994076 [details]
Bug 1477525 part 1 - Merge the logic of _safeToCollapse into hideNavToolbox.

https://reviewboard.mozilla.org/r/258664/#review269220

Comment 9

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8994077 [details]
Bug 1477525 part 2 - Try hiding toolbox again when input in chrome loses the focus.

https://reviewboard.mozilla.org/r/258662/#review269218

> This is rather fragile as it assumes that a click will be done after 300ms. Technically I don't think there's any limit for how long a click can take. Is there a more robust way to detect this?

I admit that it is rather fragile, but I don't think it's a problem in general.

The consideration is that the UI shouldn't change in a way that user cannot realize what's happening. However, if the user deliberately presses the button for long, it is probably not a big problem if the UI has changed when the button's up.

There are several alternatives I guess:
1. always animate the hiding in this case, so that the UI wouldn't change dramatically in a short time
2. track whether a click is on the way as some global state
3. listen on specific events (e.g. keypress, click) for this check rather than using blur

(2) feels fragile as well, and (3) is harder to be reliable. (1) may work although we may still want to wait for a random time before start animating.

Any other suggestion?

Comment 10

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Bug 1477525 part 1 - Merge the logic of _safeToCollapse into hideNavToolbox. r=dao

MozReview-Commit-ID: 1SEUVGprmv3

Comment 11

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Bug 1477525 part 2 - Try hiding toolbox again when input in chrome loses the focus. r=dao

MozReview-Commit-ID: LnwVXN4oS2Z

Depends on D6750

Comment 12

[Xidorn Quan [:xidorn] UTC+11]

I'm losing my development environment to test this patches in several days, so it would be really appreciated if you can review the patches soonish, otherwise I may not be able to fix and land it if there is any issue.

Comment 13

[Xidorn Quan [:xidorn] UTC+11]

It doesn't matter now. I've wiped the machine.

Comment 14

[Dão Gottwald [::dao]]

Comment on attachment 9011671 [details]
Bug 1477525 part 1 - Merge the logic of _safeToCollapse into hideNavToolbox. r=dao

Dão Gottwald [::dao] has approved the revision.

Comment 15

[Dão Gottwald [::dao]]

Comment on attachment 9011672 [details]
Bug 1477525 part 2 - Try hiding toolbox again when input in chrome loses the focus. r=dao

Dão Gottwald [::dao] has approved the revision.

Comment 16

[Xidorn Quan [:xidorn] UTC+11]

Thanks.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1ecda91f00bf
part 1 - Merge the logic of _safeToCollapse into hideNavToolbox. r=dao
https://hg.mozilla.org/integration/autoland/rev/cdedc6a6b40a
part 2 - Try hiding toolbox again when input in chrome loses the focus. r=dao

Comment 18

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/1ecda91f00bf
https://hg.mozilla.org/mozilla-central/rev/cdedc6a6b40a

Comment 19

[Mohammad Maruf Rahman]

I have reproduced this bug with Nightly 63.0a1 (2018-07-21) on Windows 7, 64 Bit!
This bug's fix is verified with latest Beta!

Build ID 	20181122182000
User Agent 	Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:64.0) Gecko/20100101 Firefox/64.0