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

RESOLVED FIXED in Firefox 48

Status

()

Core
DOM
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mccr8, Assigned: Away for a while)

Tracking

(Blocks: 1 bug)

Trunk
mozilla48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s+, firefox45 affected, firefox48 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
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.

Updated

2 years ago
Blocks: 984139
tracking-e10s: --- → +
(Reporter)

Comment 1

2 years ago
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.
(Reporter)

Updated

2 years ago
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
(Reporter)

Comment 2

2 years ago
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.
(Reporter)

Comment 3

2 years ago
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.
(Assignee)

Comment 6

2 years ago
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.
(Assignee)

Comment 8

2 years ago
I have a fix.
Assignee: nobody → ehsan
(Assignee)

Comment 9

2 years ago
Created attachment 8729718 [details] [diff] [review]
Update the scrollbar visibility prefs when initializing a TabChild

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+

Comment 11

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/66fdeb1acce5
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)
(Assignee)

Comment 13

2 years ago
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)
(Assignee)

Comment 14

2 years ago
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)

Comment 15

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc66a53a75fd
(Assignee)

Updated

2 years ago
See Also: → bug 1257887
(Reporter)

Updated

2 years ago
Blocks: 1257923

Comment 16

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/dc66a53a75fd
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.