Closed Bug 1229220 Opened 8 years ago Closed 8 years ago

[e10s] window.scrollbars.visible is not false when window is opened with scrollbars=no

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
e10s + ---
firefox45 --- affected
firefox48 --- fixed

People

(Reporter: mccr8, Assigned: ehsan.akhgari)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

This test opens two new windows, once with scrollbars=yes and once with scrollbars=no (along with some other settings). In the latter case, |w['scrollbars'].visible| is expected to be false, but it is true when run with e10s. I'm not sure what the matter is. (The test fails even when only one window is opened.) Maybe setting scrollbars=no does some kind of async round trip to the parent process, and the child process hasn't gotten the reply back yet by the time it queries the value.
Blocks: e10s-tests
tracking-e10s: --- → +
Bug 1194897 is related, but it involves the other barprops things, which are derived from the chrome flags. scrollbars seems to poke directly at the docshell to get the information.
Summary: [e10s] test_window_bar.html | scrollbars should follow window.open settings. - got true, expected false → [e10s] window.scrollbars.visible is not false when window is opened with scrollbars=no
dom/tests/browser/browser_test_toolbars_visibility.js could be updated to include this. Also I'm not sure test_window_bar tests anything different, so it might be worth removing it.
Do you have time to look into this, Mike? It involves docshell and barprops.
Flags: needinfo?(mconley)
I'm kinda swamped with e10s perf work right now, which is my highest priority. This has been tracking-e10s+'d, which (presumably) means that we'll revisit this when things calm down, e10s-wise.
Flags: needinfo?(mconley)
If, of course, you feel it needs reconsideration, please set tracking-e10s to ? and we'll discuss in triage next week.
I spent some time debugging test_window_bar.html.  It seems like in e10s mode, nsXULWindow::SetContentScrollbarVisibility() is called just like the non-e10s mode.  That function finds _some_ inner window in the parent process, and gets the ScrollbarsProp object off of it, and calls SetVisible() on it, but the child process has no idea any of this has ever happened, so as far as it's concerned, the scrollbars are still visible.

Presumably the fix would be to do the work similar to SetContentScrollbarVisibility() in the child somewhere appropriate.  I don't know where such a place would be.
(In reply to :Ehsan Akhgari from comment #6)
> Presumably the fix would be to do the work similar to
> SetContentScrollbarVisibility() in the child somewhere appropriate.  I don't
> know where such a place would be.

Perhaps in https://hg.mozilla.org/mozilla-central/file/af7c0cb0798f/dom/ipc/nsIContentParent.cpp#l118 ?

We already do the work there to propagate private browsing state from the parent - maybe we can do the same thing with scrollbar visibility.
I have a fix.
Assignee: nobody → ehsan
This will make sure that window.scrollbars correctly reflects the respective
chrome flags in e10s mode.

We also update nsXULWindow::SetContentScrollbarVisibility() to the new
nsContentUtils helper.  That code is responsible for doing this work in the
single process case.
Attachment #8729718 - Flags: review?(bugs)
Comment on attachment 8729718 [details] [diff] [review]
Update the scrollbar visibility prefs when initializing a TabChild

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

FWIW, this looks good to me.
Attachment #8729718 - Flags: feedback+
Attachment #8729718 - Flags: review?(bugs) → review+
Backed out fro M-e10s(2) failure in test_fullscreen-api.html

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/9ccaec771922

Push with failures (has leaks from a different bug before it in its log): https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=66fdeb1acce5
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=23711900&repo=mozilla-inbound

14:37:19     INFO -  1047 INFO TEST-PASS | dom/html/test/test_fullscreen-api.html | [scrollbar] Should only check the current fullscreen element
14:37:19     INFO -  1048 INFO TEST-UNEXPECTED-FAIL | dom/html/test/test_fullscreen-api.html | [scrollbar] Should have vertical scrollbar when [object HTMLHtmlElement] is in fullscreen - got 1600, expected 1585
14:37:19     INFO -      SimpleTest.is@SimpleTest/SimpleTest.js:267:5
14:37:19     INFO -      is@dom/html/test/file_fullscreen-scrollbar.html:30:25
14:37:19     INFO -      assertHasScrollbars@dom/html/test/file_fullscreen-scrollbar.html:54:3
14:37:19     INFO -      checkScrollbars@dom/html/test/file_fullscreen-scrollbar.html:73:3
14:37:19     INFO -      enteredFullscreenOnRoot@dom/html/test/file_fullscreen-scrollbar.html:100:3
14:37:19     INFO -      addFullscreenChangeContinuation/invokeCallback/</<@dom/html/test/file_fullscreen-utils.js:54:50
14:37:19     INFO -      setTimeout handler*SimpleTest_setTimeoutShim@SimpleTest/SimpleTest.js:621:12
14:37:19     INFO -      addFullscreenChangeContinuation/invokeCallback/<@dom/html/test/file_fullscreen-utils.js:54:33
14:37:19     INFO -      FrameRequestCallback*invokeCallback@dom/html/test/file_fullscreen-utils.js:54:5
14:37:19     INFO -      onFullscreenChange@dom/html/test/file_fullscreen-utils.js:59:7
14:37:19     INFO -      EventListener.handleEvent*addFullscreenChangeContinuation@dom/html/test/file_fullscreen-utils.js:70:3
14:37:19     INFO -      begin@dom/html/test/file_fullscreen-scrollbar.html:95:3
14:37:19     INFO -      SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@SimpleTest/SimpleTest.js:740:59
14:37:19     INFO -  Not taking screenshot here: see the one that was previously logged
14:37:19     INFO -  1049 INFO TEST-UNEXPECTED-FAIL | dom/html/test/test_fullscreen-api.html | [scrollbar] Should have horizontal scrollbar when [object HTMLHtmlElement] is in fullscreen - got 1200, expected 1185
Flags: needinfo?(ehsan)
Hmm, so the thing that this test does that is special is that it calls window.open() with a non-empty features argument without specifying scrollbars.  In this case, Firefox 45 and trunk non-e10s mode create a non-scrollable window.  But trunk e10s mode, Chrome and Safari all create a scrollable window.  My patch "fixes" the e10s mode to be non-scrollable as well (presumably because before nobody was setting the attribute on the docshell in the first place in e10s mode.)

I personally think making the window non-scrollable in this case is pretty broken, and we should only honor scrollbars=no in nsWindowWatcher::CalculateChromeFlags().  Olli, what do you think?
Flags: needinfo?(ehsan) → needinfo?(bugs)
We discussed this on IRC yesterday, and smaug asked me to keep the non-e10s behavior and file a follow-up for changing it.
Flags: needinfo?(bugs)
See Also: → 1257887
Blocks: 1257923
https://hg.mozilla.org/mozilla-central/rev/dc66a53a75fd
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
See Also: → 1429900
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.