1257887 - Consider changing the default for whether a window opened through window.open() to be scrollable

Status: RESOLVED FIXED

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

Description

[(no longer active)]

Currently if you pass an empty string as the third argument to window.open(), the resulting window will have scrollbars enabled, but if you do include features in that argument but leave out "scrollbars", the resulting window will have scrollbars disabled.

This is a bit hostile to the user IMO, and it assumes that the opener page is the best one to decide whether the opened window can be scrollable.  However, there are existing ways of achieving that (such as making the viewport overflow:hidden), and I think that in the majority of the cases the user will want to be able to scroll, even if the page content doesn't need scrolling in the "normal" case (think for example a page with enough content to fit within the dimensions of the window, but the user zooming in to be able to read the text, but suddenly won't be able to scroll to see all of the text.)

I suggest that we should enable scrollbars by default when scrollbars doesn't appear in the feature argument.  We can probably continue to support scrollbars=no.

Chrome and Safari don't provide a way to disable scrollbars.  I think IE does what we do.

(This is a follow-up to bug 1229220.)

Comment 1

[Andrew Overholt [:overholt]]

This may not be super important. Ehsan?

Comment 2

[(no longer active)]

What do you mean?

Comment 3

[Andrew Overholt [:overholt]]

Mike, what do you think about changing this behaviour from a web compat standpoint?

Comment 4

[Mike Taylor [:miketaylr]]

Given Chrome and Safari's behavior, and our own option `dom.disable_window_open_feature.scrollbars`, I think risk is probably very low.

AFAICT, Chrome and Safari don't honor scrollbars=no.

(Doing some research, a lot of accessibility docs recommend enabling scrollbars by default as well -- seems like a win there.)

Comment 5

[Ben Tian (inactive)]

Attachment: Patch 1 (v1): Make windows opened through window.open() be scrollable by default

The patch enables scrollbar by default as titlebar and closebox, if they are not mentioned in argument |features| of window.open().

I'm still confirming whether to enable scrollbar by default for popup windows.

Comment 6

[Mike Taylor [:miketaylr]]

(should we also get rid of dom.disable_window_open_feature.scrollbars if we change our default behavior to what it enabled?)

Comment 7

[Ben Tian (inactive)]

(In reply to Mike Taylor [:miketaylr] from comment #6)
> (should we also get rid of dom.disable_window_open_feature.scrollbars if we
> change our default behavior to what it enabled?)

I thought we want to keep it per comment 4? I'm not sure whether the option is still required.

Comment 8

[Ben Tian (inactive)]

The following test cases fail with comment 5 patch
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e9713809be80
1) dom/html/test/test_bug369370.html | Checking scrollLeft - got 413, expected 400
2) layout/base/tests/test_transformed_scrolling_repaints_3.html | Fully-visible scrolled element should not have been painted - got true, expected false
3) dom/tests/mochitest/bugs/test_window_bar.html | scrollbars should follow window.open settings. - got true, expected false

==
1) and 2) both hit priv check failure [1] even with explicit 'scrollbars=no' set. I'll revise code flow of comment 5 patch. For 3) I'll remove the check for scrollbars since it's enabled by default.

[1] https://dxr.mozilla.org/mozilla-central/source/embedding/components/windowwatcher/nsWindowWatcher.cpp#1753

Comment 9

[Mike Taylor [:miketaylr]]

Sorry my Comment #4 wasn't clear. AFAIU, dom.disable_window_open_feature.scrollbars exists to enable the default behavior you're implementing with your patch. So it seems redundant (but it could be removed in a follow-up bug).

Comment 10

[Ben Tian (inactive)]

Seems reasonable to me. Note there's a minor difference that dom.disable_window_open_feature.scrollbars overrides scrollbars=no. Removing the pref makes scrollbar flag depend on only string specified in |features| argument.

I'll remove dom.disable_window_open_feature.scrollbars in next patch. since [1] is the only test case using it.
[1] https://dxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/bugs/test_window_bar.html#46

Comment 11

[Ben Tian (inactive)]

Attachment: [final] Patch 1: Make windows opened through window.open() be scrollable by default, r=smaug

Change:
- default scrollbar to "on" unless explicitly turned off 
- remove dom.disable_window_open_feature.scrollbars pref
- turn off scrollbar explicitly in two layout test cases
- remove trailing spaces

Comment 12

[Ben Tian (inactive)]

Try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a2e58280a4f1

Comment 11 patch seems irrelevant to the failed test cases (mostly copy/paste related on Win7).

Comment 13

[Ben Tian (inactive)]

Comment on attachment 8753297 [details] [diff] [review]
[final] Patch 1: Make windows opened through window.open() be scrollable by default, r=smaug

Olli,

Can you review my patch that makes windows opened through window.open() be scrollable by default? Patch details are in comment 11 and try result in comment 12.

Comment 14

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

Comment on attachment 8753297 [details] [diff] [review]
[final] Patch 1: Make windows opened through window.open() be scrollable by default, r=smaug

Looks ok.

This may cause some regressions if pages are relying on our behavior.


Might be worth to send an email to dev.platform about the change.

Comment 15

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

(In reply to Olli Pettay [:smaug] from comment #14
> This may cause some regressions if pages are relying on our behavior.

So we need to just follow whatever incoming bugs there are and then decide what to do if there are several regressions.

Comment 16

[Ben Tian (inactive)]

> (In reply to Olli Pettay [:smaug] from comment #14
> > This may cause some regressions if pages are relying on our behavior.
> 
> So we need to just follow whatever incoming bugs there are and then decide
> what to do if there are several regressions.

Thanks for the review. I'll send a mail to dev.platform and follow possible regression bugs.

Comment 17

[Ben Tian (inactive)]

(In reply to Ben Tian [:btian] from comment #16)
> Thanks for the review. I'll send a mail to dev.platform and follow possible
> regression bugs.

The mail to dev.platform:
https://groups.google.com/forum/#!topic/mozilla.dev.platform/BAmbAhZiR7o

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4adb52152ba4

Comment 19

[Kohei Yoshino]

Posted the site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2016/pop-up-window-is-now-scrollable-by-default/

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4adb52152ba4

Comment 21

[Eric Shepherd [:sheppy]]

Updated https://developer.mozilla.org/en-US/docs/Web/API/Window/open#Window_functionality_features and added a note to Firefox 49 for developers.