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

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[:Felipe Gomes (needinfo for replies!)]

Attachment: 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.

Comment 1

[:Felipe Gomes (needinfo for replies!)]

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

Comment 2

[Dão Gottwald [:dao]]

This should land before branching or after the release is wrapped up, to avoid merging problems.

Comment 3

[Jim Mathies [:jimm]]

Comment on attachment 513614 [details] [diff] [review]
Patch

r+ from me on the smallish nsWindow change.

Comment 4

[:Felipe Gomes (needinfo for replies!)]

Attachment: 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.

Comment 5

[:Felipe Gomes (needinfo for replies!)]

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.

Comment 6

[:Felipe Gomes (needinfo for replies!)]

Attachment: WIP

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

Comment 7

[:Felipe Gomes (needinfo for replies!)]

Attachment: Patch

Patch unbitrotted and adding the tests for Windows as discussed in the all hands.

Comment 8

[Dão Gottwald [: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.

Comment 9

[:Felipe Gomes (needinfo for replies!)]

http://hg.mozilla.org/mozilla-central/rev/299c005302cd