Closed Bug 1257887 Opened 8 years ago Closed 8 years ago

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

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: ehsan.akhgari, Assigned: ben.tian)

References

Details

(Keywords: addon-compat, dev-doc-complete, site-compat, Whiteboard: [tw-dom] btpp-close)

Attachments

(1 file, 1 obsolete file)

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)
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
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?)
(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.
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).
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
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
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 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.
> (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.
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
(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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4adb52152ba4
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Whiteboard: [tw-dom] btpp-active → [tw-dom] btpp-close
See Also: → 1429900
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: