Simplify fullscreen code

VERIFIED FIXED in Firefox 40

Status

()

VERIFIED FIXED
3 years ago
2 years ago

People

(Reporter: dao, Assigned: dao)

Tracking

Trunk
Firefox 41
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 fixed, firefox41 verified)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
Despite having been partially refactored recently, the browser fullscreen code is still a mess and hard to follow, which has lead to regressions such as bug 1174431. I think there are some simplification opportunities:

- hide more stuff with CSS instead of JS
- rename showXULChrome to something more sensible, remove the aTag parameter
- replace browser.fullscreen.animateUp with a boolean pref
- get rid of the _shouldAnimate flag and forceHide parameters
(Assignee)

Comment 1

3 years ago
Created attachment 8624712 [details] [diff] [review]
patch
Attachment #8624712 - Flags: review?(quanxunzhen)

Comment 2

3 years ago
Is this going to be safe for branch uplift right before we go to beta? bug 1174431 also affects 40.
(Assignee)

Comment 3

3 years ago
Reasonably safe. Since it simplifies the code rather than adding layers of complication on top of it, it would hopefully make followup fixes easier in case more issues with the fullscreen code surface. There's a risk that the simplification is fundamentally flawed, but I think it's straightforward enough that this is very unlikely.
Comment on attachment 8624712 [details] [diff] [review]
patch

Review of attachment 8624712 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me. Thanks.

::: browser/base/content/browser-fullScreen.js
@@ +508,5 @@
>      MousePosTracker.removeListener(this);
>    },
>  
> +  _updateToolbars: function (aEnterFS) {
> +    for (let el of document.querySelectorAll("toolbar[fullscreentoolbar=true]")) {

I suppose the selector "toolbar" has the same meaning with the original getElementsByTagNameNS() call even considering namespace. But I'm not that sure.
Attachment #8624712 - Flags: review?(quanxunzhen) → review+
(Assignee)

Comment 6

3 years ago
This has been merged without the bug being updated properly.

https://hg.mozilla.org/mozilla-central/rev/1f7db5d87e80
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Probably we want to uplift this patch for bug 1174431?
Flags: needinfo?(dao)
(Assignee)

Comment 8

3 years ago
I'll request approval once this has baked in Nightly for a few more days.
(Assignee)

Comment 9

3 years ago
Comment on attachment 8624712 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]: bug 1168397
[User impact if declined]: bug 1174431
[Describe test coverage new/current, TreeHerder]: n/a
[Risks and why]: see comment 3
[String/UUID change made/needed]: none
Flags: needinfo?(dao)
Attachment #8624712 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

3 years ago
status-firefox41: affected → fixed
Dão, please n-i me when you think it is ready. Thanks
status-firefox40: --- → affected

Updated

3 years ago
Depends on: 1177185
(Assignee)

Updated

3 years ago
Attachment #8624712 - Flags: approval-mozilla-aurora? → approval-mozilla-beta?
(Assignee)

Comment 11

3 years ago
(In reply to Sylvestre Ledru [:sylvestre] PTO => July 10th from comment #10)
> Dão, please n-i me when you think it is ready. Thanks

Missed that comment. It's been ready ever since I requested approval ;)
Flags: needinfo?(sledru)
Florin - Given that this is not just a fix but also a larger change to simplify the code, can your team verify the fix on 41 or 42 before we uplift to Beta? If the fix can be verified by Wed, we can take the fix in beta3.
Flags: needinfo?(sledru) → needinfo?(florin.mezei)
(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #12)
> Florin - Given that this is not just a fix but also a larger change to
> simplify the code, can your team verify the fix on 41 or 42 before we uplift
> to Beta? If the fix can be verified by Wed, we can take the fix in beta3.

We'll try to verify this by Wednesday or Thursday at the latest, depending on what other tasks we have in our backlog. Right now it seems we should be able to do it by tomorrow. 

Also, just to be clear, here we are only talking about the browser fullscreen mode (e.g. using F11 on Windows), and no connection to things like Video fullscreen.
(Assignee)

Comment 14

3 years ago
This is actually relevant for DOM fullscreen (e.g. video) too, since both fullscreen features share some frontend code. That's even part of the reason why the code got messy. For instance, this patch fixed bug 1174431.
Thanks Dao! Petruta started testing this today on Windows 7 x64 and Mac OS X 10.9.5, and so far things are going well. We intend to continue tomorrow with Windows 10 and Ubuntu and have the final results for this.
We performed exploratory testing around fullscreen using Aurora 41.0a2 2015-07-08 under Windows 7 x64, Windows 10 Pro x64 (Insider Preview Build 10162, Microsoft Surface Pro 2), Windows 10 Pro x64, Ubuntu 14.04 x32 and Mac OS X 10.9.5.

No new issues were logged, the issues found were either reproducible before the fix entered, so unrelated to fullscreen refactoring, either 3rd-party issues.
We were also able to successfully verify bug 1177185, bug 1174431, bug 1168397 and bug 1162752.

More details of our testing can be found at: https://etherpad.mozilla.org/FullscreenRefactoring-bug1176233

We think that this patch can be uplifted to beta.
Status: RESOLVED → VERIFIED
status-firefox41: fixed → verified
Flags: needinfo?(florin.mezei)
Comment on attachment 8624712 [details] [diff] [review]
patch

With this fix verified, let's get it into beta3. Beta+
Attachment #8624712 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Needs rebasing for beta uplift.
Flags: needinfo?(dao)
Keywords: branch-patch-needed
(Assignee)

Comment 19

3 years ago
https://hg.mozilla.org/releases/mozilla-beta/rev/9d1a822f6d8e
status-firefox40: affected → fixed
Flags: needinfo?(dao)
Keywords: branch-patch-needed

Updated

3 years ago
Depends on: 1191966
Encountered an issue on Firefox 38.5.0 ESR (under Ubuntu 14.04 32-bit) which is not reproducible on Firefox 45.0a1. It seems to be fixed by this bug. 


The “Edit this bookmark” doorhanger is incorrectly displayed in fullscreen mode while using 2 screens.

STR
1. Launch Firefox.
2. Open http://cp.literature.agilent.com/litweb/pdf/5989-8139EN.pdf
3. Enter in fullscreen mode and click “Allow” button.
4. Right click and select “Bookmark this page”.

“Bookmarks this page” doorhanger is displayed on the second screen.
See screenshot: http://i.imgur.com/O3y66lY.jpg


Regression window with --find-fix flag  

- First good revision: 1f7db5d87e8059e7e2947c4e1ece06e610107faf
- Last bad revision: 391241169becf6ed866341b96c7a88d4325ed83b
- Pushlog: https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=391241169becf6ed866341b96c7a88d4325ed83b&tochange=1f7db5d87e8059e7e2947c4e1ece06e610107faf



Should I file a new bug for this issue, in order to be treated separately?
Flags: needinfo?(dao)
This sounds more like a bug fixed by bug 1137009.
(Assignee)

Comment 22

3 years ago
Either way, no need to report separately if it's fixed on Nightly. It's not the kind of bug we would fix on ESR. ESR gets stability and security fixes only.
Flags: needinfo?(dao)

Updated

3 years ago
Depends on: 1235592

Updated

2 years ago
Depends on: 1274834
You need to log in before you can comment on or make changes to this bug.