1663500 - Links are opening new windows despite browser.link.open_newwindow being set to 1

Status: RESOLVED FIXED

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

Description

[202nine]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:80.0) Gecko/20100101 Firefox/80.0

Steps to reproduce:

Clicked on links from this search page:

https://www.startpage.com/en/

Actual results:

All links are opening in new windows, although it can also be configured to open in new tabs instead.

Expected results:

Previous to v82 the links opened in the same tab.

Comment 1

[202nine]

This is a regression from this bug:

https://bugzilla.mozilla.org/show_bug.cgi?id=1661643

Prior to the removal of “browser.link.open_newwindow" there was never a problem opening links in the same tab, including the links from bug 1661643. Please reconsider removing this pref and fixing the regression as it has been an option for many years in Firefox and no doubt has considerable usage. There are currently no addons available to restore the lost feature.

Comment 2

[Tooru Fujisawa [:arai]]

browser.link.open_newwindow pref haven't been removed.
The bug dropped the support for browser.link.open_newwindow=1 value, because the option haven't been supported in about:preferences and the behavior have been inconsistent (in some case it still opened new window).

about:preferences contains "Open links in tabs instead of new windows" checkbox, where unchecked means browser.link.open_newwindow=2 and checked means browser.link.open_newwindow=3, so browser.link.open_newwindow=1 couldn't be represented/kept there.

So, if we restore the pref value, we should look into the preference page for consistency.
Gijs, can I have your opinion?

Comment 3

[:Gijs (he/him)]

(In reply to 202nine from comment #1)

no doubt has considerable usage.

And your source for this confident assertion about a pref that has never been exposed to users directly is...?

There are currently no addons available to restore the lost feature.

It would be trivial to write one, it would just need to remove target=_blank from link tags from pointerdown events, and keydown events for the enter key.

(In reply to Tooru Fujisawa [:arai] from comment #2)

browser.link.open_newwindow pref haven't been removed.
The bug dropped the support for browser.link.open_newwindow=1 value, because the option haven't been supported in about:preferences and the behavior have been inconsistent (in some case it still opened new window).

Was there also an engineering reason? That is, did removing it accomplish anything else / make other work easier?

about:preferences contains "Open links in tabs instead of new windows" checkbox, where unchecked means browser.link.open_newwindow=2 and checked means browser.link.open_newwindow=3, so browser.link.open_newwindow=1 couldn't be represented/kept there.

So I'm a little confused here; when I originally reviewed the patch, my impression from your commit message and comments was that if you opened about:preferences with the pref set to 1 then the pref would be altered immediately, so its value was 2 or 3. Is that correct? It doesn't appear to be the case from a quick test... if not, when would the preference be changed - only when the user interacts with the checkbox? That doesn't seem so bad...

Comment 4

[Tooru Fujisawa [:arai]]

(In reply to Tooru Fujisawa [:arai] from comment #2)

browser.link.open_newwindow pref haven't been removed.
The bug dropped the support for browser.link.open_newwindow=1 value, because the option haven't been supported in about:preferences and the behavior have been inconsistent (in some case it still opened new window).

Was there also an engineering reason? That is, did removing it accomplish anything else / make other work easier?

It was to simplify the condition around window.open target.
We had 3 cases about where the newly opened window goes (if it's not a popup), depending on the pref value:

1. current tab
2. new window
3. new tab

and "current tab" case was troublesome to standardize the behavior (in https://github.com/whatwg/html/issues/5872 ).

Without "current tab" case, it reuses existing browsing context only when windowName parameter matches to existing one.

But "current tab" case overrides whether to create a new browsing context, and I didn't want to introduce that option into the spec.

about:preferences contains "Open links in tabs instead of new windows" checkbox, where unchecked means browser.link.open_newwindow=2 and checked means browser.link.open_newwindow=3, so browser.link.open_newwindow=1 couldn't be represented/kept there.

So I'm a little confused here; when I originally reviewed the patch, my impression from your commit message and comments was that if you opened about:preferences with the pref set to 1 then the pref would be altered immediately, so its value was 2 or 3. Is that correct? It doesn't appear to be the case from a quick test... if not, when would the preference be changed - only when the user interacts with the checkbox? That doesn't seem so bad...

Yes, sorry, I misunderstood the behavior about the current pref pane.
The value seems to be altered only when the checkbox is touched.

Comment 5

[:Gijs (he/him)]

(In reply to Tooru Fujisawa [:arai] from comment #4)

(In reply to Tooru Fujisawa [:arai] from comment #2)

browser.link.open_newwindow pref haven't been removed.
The bug dropped the support for browser.link.open_newwindow=1 value, because the option haven't been supported in about:preferences and the behavior have been inconsistent (in some case it still opened new window).

Was there also an engineering reason? That is, did removing it accomplish anything else / make other work easier?

It was to simplify the condition around window.open target.
We had 3 cases about where the newly opened window goes (if it's not a popup), depending on the pref value:

1. current tab
2. new window
3. new tab

and "current tab" case was troublesome to standardize the behavior (in https://github.com/whatwg/html/issues/5872 ).

I'm sorry, but from a skim-read and looking for "current" in the github issue, I don't see any discussion of this particular issue (ie forcing _blank to open in the current context). What am I missing? Or was it not discussed yet?

Without "current tab" case, it reuses existing browsing context only when windowName parameter matches to existing one.

But "current tab" case overrides whether to create a new browsing context, and I didn't want to introduce that option into the spec.

Can you be more specific about how the removal of the option to reuse the browsing context presents obstacles for the spec / behaviour? I would have thought that language along the lines of "If the windowName parameter is "_blank", User Agents may offer the user the option to override it and open the link in the current browsing context" would be sufficient? Does this open cans of worms I'm not seeing?

Basically, I think we shouldn't gratuitously remove things if there is no need. Equally, unlike the reporter, I am very unconvinced that people in general want/need this option to the point that it couldn't just be an add-on, and I definitely don't think we should advertise the option in the main preferences, as it is more likely to cause unexpected issues than do what people want. What I'm not sure about is how to evaluate the "need" side of the removal here.

Comment 6

[202nine]

(In reply to :Gijs (he/him) from comment #3)

(In reply to 202nine from comment #1)

no doubt has considerable usage.

And your source for this confident assertion about a pref that has never been exposed to users directly is...?

There are currently no addons available to restore the lost feature.

It would be trivial to write one, it would just need to remove target=_blank from link tags from pointerdown events, and keydown events for the enter key.

Well I don't have telemetry figures obviously to give you, it's strictly anecdotal based on those in forums etc who prefer it. I've been using the option for years.

Good to know about the addon if you can't/won't restore it.

Comment 7

[Tooru Fujisawa [:arai]]

(In reply to :Gijs (he/him) from comment #5)

I'm sorry, but from a skim-read and looking for "current" in the github issue, I don't see any discussion of this particular issue (ie forcing _blank to open in the current context). What am I missing? Or was it not discussed yet?

Sorry, I was unclear.

I'm trying to standardize the beahvior of window.open in that issue,
but I haven't talked about "current tab" case there.

As a preparation for the issue, I was cleaning up the current situation summarized in https://arai-a.github.io/window-open-features/ ,
and thought that "current tab" case complicates how/where to modify the spec.
(I removed the case in https://github.com/arai-a/window-open-features/commit/a56d87e445baae39a4038e42cc6977c5c1008598 , I don't think the steps defined there was correct )

Can you be more specific about how the removal of the option to reuse the browsing context presents obstacles for the spec / behaviour? I would have thought that language along the lines of "If the windowName parameter is "_blank", User Agents may offer the user the option to override it and open the link in the current browsing context" would be sufficient? Does this open cans of worms I'm not seeing?

That might work.
Actually, I'm not much get used to HTML spec (like, how to provide impl-dependent option), and not sure how bad providing that option is.
Just that I wanted to simplify/minimize the change before starting the discussion.

I'm open to any suggestion and also now I'm fine reverting the patch.

Basically, I think we shouldn't gratuitously remove things if there is no need. Equally, unlike the reporter, I am very unconvinced that people in general want/need this option to the point that it couldn't just be an add-on, and I definitely don't think we should advertise the option in the main preferences, as it is more likely to cause unexpected issues than do what people want. What I'm not sure about is how to evaluate the "need" side of the removal here.

Okay, I agree.
At least we should keep the previous behavior until the spec change really hits actual trouble.

Comment 8

[Tooru Fujisawa [:arai]]

Attachment: Bug 1663500 - Revert bug 1661643 change and resurrect browser.link.open_newwindow=1. r?Gijs!

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/65595b2ad617
Revert bug 1661643 change and resurrect browser.link.open_newwindow=1. r=Gijs,preferences-reviewers

Comment 10

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1661643

Comment 11

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/65595b2ad617