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




8 years ago
8 years ago


(Reporter: Felipe, Assigned: Felipe)


Firefox 6
Windows 7

Firefox Tracking Flags

(Not tracked)



(4 attachments)

Posted patch PatchSplinter Review
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: nobody → felipc
Comment on attachment 513614 [details] [diff] [review]

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 on attachment 513614 [details] [diff] [review]

r+ from me on the smallish nsWindow change.
Attachment #513614 - Flags: review?(jmathies) → review+
Posted patch With testsSplinter Review
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.
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.
Posted patch WIPSplinter Review
WIP patch with changes needed to nsWebShellWindow. This should work on all platforms, except that it doesn't.. trying to figure out why
Posted patch PatchSplinter Review
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]

>+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+
Last Resolved: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 6
You need to log in before you can comment on or make changes to this bug.