1589299 - window.open with scrollbars=0 sets BarProp {visible: false} on window.scrollbars

Status: RESOLVED DUPLICATE of bug 1507375

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

Description

[Karl Dubost💡 :karlcow]

Here a testcase
https://codepen.io/webcompat/pen/wvvWLbB

<a
 href="http://www.mozilla.org/"
 target="Moz_WindowName"
 onclick="openRequestedPopup(); return false;" 
>Test me!</a></p>

<script>
var windowObjectReference;
var strWindowFeatures = "height=500,width=500,scrollbars=0";

function openRequestedPopup() {
  windowObjectReference = window.open("https://mozilla.org", "Moz_WindowName", strWindowFeatures);
}
</script>

On the new window, window.scrollbars returns

• On Firefox BarProp {visible: false}
• On Chrome, Safari BarProp {visible: true}

Basically Chrome, Safari ignores it.

https://html.spec.whatwg.org/multipage/window-object.html#dom-window-scrollbars

I don't see anything how scrollbars=0 should be translated.

Comment 1

[Karl Dubost💡 :karlcow]

Ah here.
https://html.spec.whatwg.org/multipage/window-object.html#concept-window-open-features-parse-boolean

To parse a boolean feature given a string value:

    If value is the empty string, then return true.

    If value is a case-sensitive match for yes, then return true.

    Let parsed be the result of parsing value as an integer.

    If parsed is an error, then set it to 0.

    Return false if parsed is 0, and true otherwise.

So I guess chrome and safari do not follow the spec here. Or maybe it's not clear

if value is  `0`, then return false

Should I open a bug on the whatwg spec.

Comment 2

[Karl Dubost💡 :karlcow]

I see https://bugs.chromium.org/p/chromium/issues/detail?id=310691
Issue 310691: Remove BarProp properties from Window object

Oh Bug 1200936

Comment 3

[Karl Dubost💡 :karlcow]

Anne,
Idea about the next step?

Comment 4

[Anne (:annevk)]

As far as I can tell Firefox is the only browser that lets the site dictate the scrollbar. Other browsers ignore scrollbars=0 and have a scrollbar anyway (which frankly seems a lot better for security). (No difference if the popup is same origin by the way.) I don't think bug 1200936 will help with this however as this bug is mostly whether window.open() features are respected, and of those scrollbars in particular.

As those window.open() features are still not properly defined some care probably has to be taken in removing support for one of them. I suspect bz would be best suited to at least explain what someone tackling this would need to do.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Basically modify https://searchfox.org/mozilla-central/rev/ebe492edacc75bb122a2b380e4cafcca3470864c/toolkit/components/windowwatcher/nsWindowWatcher.cpp#1718-1723 so that we always add the nsIWebBrowserChrome::CHROME_SCROLLBARS flag in the cases when !aDialog && !aHasChromeParent && !aChromeURL.

Comment 6

[Anne (:annevk)]

Per https://github.com/whatwg/html/issues/2464 there might also be other features we'd want to disallow web content to set (I haven't tested if these work to begin with, mind), potentially as a follow-up:

• resizable
• menubar
• status

Comment 7

[Boris Zbarsky [:bzbarsky]]

"resizable" and "status" are already disallowed by default. See https://searchfox.org/mozilla-central/rev/ebe492edacc75bb122a2b380e4cafcca3470864c/modules/libpref/init/all.js#949,951 and bug 177838 and bug 283103 (or maybe bug 22183) respectively.

"menubar" is allowed at the moment, as far as I can tell.

Comment 8

[Karl Dubost💡 :karlcow]

• scrollbars=0 → BarProp {visible: false} in Firefox
• scrollbars= → BarProp {visible: false} in Firefox
• scrollbars → BarProp {visible: true} in Firefox
• scrollbars=yes → BarProp {visible: true} in Firefox
• scrollbars=no → BarProp {visible: false} in Firefox

Safari, Chrome always BarProp {visible: true}

Comment 9

[Alphan Chen [:alchen]]

Attachment: Bug 1589299 - Force scrollbars flag to be always enabled for new windows. r?smaug

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4dfd98d37804
Force scrollbars flag to be always enabled for new windows. r=smaug

Comment 11

[Cristina Coroiu [:ccoroiu]]

Backed out changeset 4dfd98d37804 (bug 1589299) for Wr failures at css/css-writing-modes/wm-propagation-svg-root-scrollbar.svg

Backout: https://hg.mozilla.org/integration/autoland/rev/c230c87ea4414402b1e803ee965422310815f69b

Failure push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=4dfd98d37804f1f34f846bef0c31e2a5523146f5

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=290586781&repo=autoland&lineNumber=25178

Comment 12

[Alphan Chen [:alchen]]

I will check the failure.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3da713367dfd
Force scrollbars flag to be always enabled for new windows. r=smaug

Comment 14

[Stefan Hindli [:stefan_hindli]]

Backed out changeset 3da713367dfd (Bug 1589299) for causing mochitest permafailures in layout/base/tests/test_transformed_scrolling_repaints_3.html CLOSED TREE

https://treeherder.mozilla.org/#/jobs?repo=autoland&fromchange=458075843d46cd0f6900ba82973718d2d61d6496&searchStr=windows%2C7%2Cdebug%2Cmochitests%2Ctest-windows7-32%2Fdebug-mochitest-plain-e10s-3%2Cm%283%29&selectedJob=291472060

https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=291472060&repo=autoland&lineNumber=16113

Comment 15

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bc42bf95ce74
Backed out changeset 3da713367dfd for causing mochitest permafailures in layout/base/tests/test_transformed_scrolling_repaints_3.html CLOSED TREE

Comment 16

[Razvan Maries]

Also caused https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=291478478&repo=autoland&lineNumber=2382.

Comment 17

[Alphan Chen [:alchen]]

Since this bug is trying to "force scrollbars flag to be always enabled for new window", I remove all places of "set scroballbars flag" from our tests in the latest revision.

Only two of them will affect the result:
dom/html/test/test_bug369370.html: Fix the failure by refining the test.
layout/base/tests/test_transformed_scrolling_repaints_3.html: The scrollbar will affect the result on windows. After investigation, we can enlarge the new window to prevent the scrollbar and it won't affect the test purpose and result.

However, if we want to keep the ability to disable scrollbars, maybe we should introduce a preference for testing. What do you think?

Comment 18

[Alphan Chen [:alchen]]

Try result with removing all places of "set scroballbars flag" from our tests
https://treeherder.mozilla.org/#/jobs?repo=try&revision=440d6e02762322bab4bef29fe8100859c9c2876c

dom/html/test/test_bug369370.html | Checking scrollLeft - got 415, expected 400
OS X 10.14 debug, OS X 10.14 Shippable opt

layout/base/tests/test_transformed_scrolling_repaints_3.html | Fully-visible scrolled element should not have been painted - got true, expected false
Windows 7 opt, debug, Shippable opt
Windows 10 x64 opt, asan, debug, Shippable opt

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

I'm not sure whether we need a pref, but no need to modify the tests.
I mean, if the test passes with or without the scrollbar=*

Comment 20

[Anne (:annevk)]

FWIW, I don't think we should keep the capability to open a popup that does not have scrollbars. The popup can make the scrollbars disappear itself, but letting someone else do it is problematic.

Comment 21

[Tooru Fujisawa [:arai]]

bug 1507375 is going to change the features parameter (UI related items) just a condition for whether to open a popup or not,
just like Google Chrome and Safari.

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

Perhaps we should just land bug 1507375 and not this one at all.

Comment 23

[Alphan Chen [:alchen]]

(In reply to Olli Pettay [:smaug] from comment #22)

Perhaps we should just land bug 1507375 and not this one at all.
Agree.