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

RESOLVED FIXED in Firefox 49

Status

()

defect
RESOLVED FIXED
3 years ago
2 months ago

People

(Reporter: Ehsan, Assigned: ben.tian)

Tracking

({addon-compat, dev-doc-complete, site-compat})

unspecified
mozilla49
Points:
---

Firefox Tracking Flags

(firefox49 fixed)

Details

(Whiteboard: [tw-dom] btpp-close)

Attachments

(1 attachment, 1 obsolete attachment)

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.)
Whiteboard: [tw-dom] btpp-backlog
This may not be super important. Ehsan?
Flags: needinfo?(ehsan)
What do you mean?
Flags: needinfo?(ehsan)
Mike, what do you think about changing this behaviour from a web compat standpoint?
Flags: needinfo?(miket)
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.)
Flags: needinfo?(miket)
Assignee

Comment 5

3 years ago
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.
Assignee: nobody → btian
Assignee

Updated

3 years ago
Whiteboard: [tw-dom] btpp-backlog → [tw-dom] btpp-active
(should we also get rid of dom.disable_window_open_feature.scrollbars if we change our default behavior to what it enabled?)
Assignee

Comment 7

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

Comment 8

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

Comment 10

3 years ago
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
Assignee

Comment 11

3 years ago
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
Attachment #8752135 - Attachment is obsolete: true
Assignee

Comment 12

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

Comment 13

3 years ago
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.
Attachment #8753297 - Flags: review?(bugs)
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.
Attachment #8753297 - Flags: review?(bugs) → review+
(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.
Assignee

Comment 16

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

Updated

3 years ago
Attachment #8753297 - Attachment description: Patch 1 (v2): Make windows opened through window.open() be scrollable by default → [final] Patch 1: Make windows opened through window.open() be scrollable by default, r=smaug
Assignee

Comment 17

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

Updated

3 years ago
Keywords: checkin-needed

Comment 20

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4adb52152ba4
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Assignee

Updated

3 years ago
Whiteboard: [tw-dom] btpp-active → [tw-dom] btpp-close
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.