Get rid of :not([inFullscreen]) workaround selectors for sizemode transitions

RESOLVED FIXED in Firefox 6

Status

()

Firefox
Theme
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Felipe, Assigned: Felipe)

Tracking

Trunk
Firefox 6
x86
Windows 7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

(Assignee)

Description

6 years ago
Created attachment 513614 [details] [diff] [review]
Patch

The code that deals with the sizemode attribute is kinda messy. It was only used to persist the sizemode and restore it on startup, so it's set off of a timer in nsXULWindow.
When we started using it for style the transition to fullscreen problem appeared. It was not a problem in maximized or normal mode because an NS_ACTIVATE event dispatched forces an instant update of the attribute. I believe there's no reason to not do that for fullscreen too and it allows us to clean-up the CSS quite a bit.
(Assignee)

Updated

6 years ago
Assignee: nobody → felipc
(Assignee)

Comment 1

6 years ago
Comment on attachment 513614 [details] [diff] [review]
Patch

Asking review from Jim for the nsWindow.cpp change. Jim, see comment 0. We send this event on changes between normal<->maximized mode, and it helps trigger an instant update of the sizemode attribute. We don't do the same for fullscreen though, but I believe it should have no problems in doing so, and it makes it possible to remove a CSS workaround that is being used at the moment.

I don't plan on taking this patch before branching
Attachment #513614 - Flags: review?(jmathies)
This should land before branching or after the release is wrapped up, to avoid merging problems.

Comment 3

6 years ago
Comment on attachment 513614 [details] [diff] [review]
Patch

r+ from me on the smallish nsWindow change.
Attachment #513614 - Flags: review?(jmathies) → review+
(Assignee)

Comment 4

6 years ago
Created attachment 514327 [details] [diff] [review]
With tests

Wrote a test to check that the sizemode attribute is updated immediately so that we can remove this CSS. Working well on Windows, sent to tryserver to see if all platforms behaves the same.

Should we get rid of the other instances of inFullscreen=true altogether and use sizemode=fullscreen instead? I see that inFullscreen is also set on gNavToolbox, but I don't see where it's being used for. Don't know if there are add-ons out there who expect this attribute.

(In reply to comment #2)
> This should land before branching or after the release is wrapped up, to avoid
> merging problems.

Ok, I'll leave this for after the release.
(Assignee)

Comment 5

6 years ago
So Linux and Mac suffer from the same problem, except that it happens for all sizemodes (not only fullscreen). I'm testing a nsXULWindow patch instead to make this work consistently on all platforms. sizemode is not currently used on other themes but it might be in the future, so I think it makes sense to get a general solution for this.
(Assignee)

Comment 6

6 years ago
Created attachment 514731 [details] [diff] [review]
WIP

WIP patch with changes needed to nsWebShellWindow. This should work on all platforms, except that it doesn't.. trying to figure out why
(Assignee)

Comment 7

6 years ago
Created attachment 525223 [details] [diff] [review]
Patch

Patch unbitrotted and adding the tests for Windows as discussed in the all hands.
Attachment #525223 - Flags: review?(dao)
Comment on attachment 525223 [details] [diff] [review]
Patch

>+function checkAndContinue(sizemode, fullScreen) {
>+  setTimeout(function() {
>+    is(window.document.documentElement.getAttribute("sizemode"), sizemode, "incorrect window sizemode attribute");
>+    is(window.fullScreen, fullScreen, "incorrect window.fullScreen property");
>+    nextStep();
>+  }, 0);
>+}

* You can get rid of the fullScreen argument and use sizemode == "fullscreen" instead.
* The test descriptions seem backwards, should be along the lines of "sizemode attribute should match actual window state" and "window.fullScreen should match actual window state".
* It might make sense to test window.windowState as well.
Attachment #525223 - Flags: review?(dao) → review+
(Assignee)

Comment 9

6 years ago
http://hg.mozilla.org/mozilla-central/rev/299c005302cd
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 6
You need to log in before you can comment on or make changes to this bug.