Last Comment Bug 635402 - Get rid of :not([inFullscreen]) workaround selectors for sizemode transitions
: Get rid of :not([inFullscreen]) workaround selectors for sizemode transitions
Product: Firefox
Classification: Client Software
Component: Theme (show other bugs)
: Trunk
: x86 Windows 7
: -- normal (vote)
: Firefox 6
Assigned To: :Felipe Gomes (needinfo me!)
Depends on:
  Show dependency treegraph
Reported: 2011-02-18 15:19 PST by :Felipe Gomes (needinfo me!)
Modified: 2011-05-09 13:36 PDT (History)
3 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Patch (6.92 KB, patch)
2011-02-18 15:19 PST, :Felipe Gomes (needinfo me!)
jmathies: review+
Details | Diff | Review
With tests (10.96 KB, patch)
2011-02-22 14:55 PST, :Felipe Gomes (needinfo me!)
no flags Details | Diff | Review
WIP (13.56 KB, patch)
2011-02-23 22:03 PST, :Felipe Gomes (needinfo me!)
no flags Details | Diff | Review
Patch (11.85 KB, patch)
2011-04-11 17:10 PDT, :Felipe Gomes (needinfo me!)
dao+bmo: review+
Details | Diff | Review

Description :Felipe Gomes (needinfo me!) 2011-02-18 15:19:23 PST
Created attachment 513614 [details] [diff] [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.
Comment 1 :Felipe Gomes (needinfo me!) 2011-02-21 04:16:55 PST
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
Comment 2 Dão Gottwald [:dao] 2011-02-21 04:34:35 PST
This should land before branching or after the release is wrapped up, to avoid merging problems.
Comment 3 Jim Mathies [:jimm] 2011-02-21 07:38:43 PST
Comment on attachment 513614 [details] [diff] [review]

r+ from me on the smallish nsWindow change.
Comment 4 :Felipe Gomes (needinfo me!) 2011-02-22 14:55:45 PST
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.
Comment 5 :Felipe Gomes (needinfo me!) 2011-02-22 18:45:24 PST
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 me!) 2011-02-23 22:03:58 PST
Created attachment 514731 [details] [diff] [review]

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 me!) 2011-04-11 17:10:36 PDT
Created attachment 525223 [details] [diff] [review]

Patch unbitrotted and adding the tests for Windows as discussed in the all hands.
Comment 8 Dão Gottwald [:dao] 2011-04-12 04:39:53 PDT
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.
Comment 9 :Felipe Gomes (needinfo me!) 2011-05-09 11:56:22 PDT

Note You need to log in before you can comment on or make changes to this bug.