1303838 - Not switch to existing "target" tab when opening anchor link with a fragment

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Vincent MONIER]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:47.0) Gecko/20100101 Firefox/47.0
Build ID: 20160623154057

Steps to reproduce:

Let's say you have some links that open in a named tab, with a fragment:
<a href="mypage.html#section-1" target="my-tab">My page</a>
<a href="mypage.html#section-2" target="my-tab">My page</a>
<a href="mypage.html#section-3" target="my-tab">My page</a>




Actual results:

Then when you click the first link, the tab opens up and go front.
But when you click another link without having closed the "my-tab" tab, then the "my-tab" tab will *not* move to front: it stays back, and so, it feels like the link did not open.


Expected results:

The "my-tab" tab should be put to front (ie: it becomes the active tab, as Opera do).

Comment 1

[Vincent MONIER]

Attachment: Showcase

Show case: open in FF, click the 1st link, and the tab will be opened with the correct hash fragment (title becomes red).
Then click the 2nd link on that 1st tab, without closing the opened tab (with the red title): the 2nd tab will not be on front, but its hash fragment gets updated (text becomes red, title becomes black).

Comment 2

[Alice0775 White]

Edge and IE11 has same behavior of Firefox. But tab highlighted.
Chrome's behavior is as the expected.

Comment 3

[Andrew Overholt [:overholt]]

Initially I thought this was a dupe of bug 668213 but now I think it might be a dupe of bug 321493. Do you think it's the same as either of those bugs?

Comment 4

[Alice0775 White]

(In reply to Andrew Overholt [:overholt] from comment #3)
> Initially I thought this was a dupe of bug 668213 but now I think it might
> be a dupe of bug 321493. Do you think it's the same as either of those bugs?

No not a duplication.  They are different bug.

Comment 5

[Alice0775 White]

Attachment: testcase

Comment 6

[Andrew Overholt [:overholt]]

Oh, so you're saying the root problem here is that 'tab' (in your example) isn't focused upon subsequent link clicking? Sorry I was wrong on the dupes; I was thrown off by the anchors.

Comment 7

[Alice0775 White]

(In reply to Andrew Overholt [:overholt] from comment #6)
> Oh, so you're saying the root problem here is that 'tab' (in your example)
> isn't focused upon subsequent link clicking? Sorry I was wrong on the dupes;
> I was thrown off by the anchors.

yep.

Comment 8

[Vincent MONIER]

Yes, that's it: you're not lead to the other tab.

When the link points to another domain (like in attachment 8793635 [details]), then this issue is not very blocking: when you click a link for the 2nd time, you see that the tab is loading the new page, even if you're not lead to that new tab.

But when you have a fragment identifier in the link, then the tab won't show any loading sign and stays in the background: it very looks like the link is just "not working".

I believe that this is another consequence of the drastic fix for #310825

Comment 9

[Hsin-Yi Tsai (she/her) (REO Fx 129) [:hsinyi]]

Samael, I am guessing that you might have some thoughts about this issue. Could you please take a look? Thanks!

Comment 10

[Samael Wang [:freesamael] (away for now)]

If I didn't misunderstand it, the first link click basically goes to openURIInFrame() [1], which honors the preference "browser.tabs.loadDivertedInBackground" to decide if it should focus on the newly opened tab; While in subsequent link clicks docshell should be able to find the target docshell directly and use targetDocShell->InternalLoad() to load the URI. Looks to me that in the second case it doesn't try to gain focus at all.

window.open() looks to have the similar execution path and has the same behavior. I expect if we change the behavior for <a href>, window.open() will also be changed.

I'm not quite familiar with HTML specs but my interpretation for window.open() [3] is that although it says the target browsing context should navigate to the given resource, it didn't define if the user agent should focus on the target browsering context.

Chrome / Safari / Opera do focus on the target browsering context. I can try to do the same thing on gecko. Do we need any UI / UX input for that before actually making the change?

[1] https://dxr.mozilla.org/mozilla-central/rev/b1d60f2f68c7cccc96fcf9a2075bb430a500a0f2/browser/base/content/browser.js#5030
[2] https://dxr.mozilla.org/mozilla-central/rev/b1d60f2f68c7cccc96fcf9a2075bb430a500a0f2/docshell/base/nsDocShell.cpp#10002-10022
[3] https://html.spec.whatwg.org/multipage/browsers.html#dom-open

Comment 11

[Samael Wang [:freesamael] (away for now)]

Hi Mike,

I may start working on this recently, but I'm not familiar with mozilla's process on UX behavior change. Do you know if we need any kind of UX review?

And talking about the implementation, since the first link click (or window.open) would honor "browser.tabs.loadDivertedInBackground", I think we should make following link clicks or window.open follow the same preference. Does it sound reasonable?

Comment 12

[Mike Conley (:mconley) (:⚙️)]

I suspect we'll want to converge with the other browsers and switch browser tabs in this case, especially since it's a user initiated action as opposed to script.

Let's ask phlsa from UX just in case, though.

Comment 13

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Sounds good to me!

Comment 14

[Mike Conley (:mconley) (:⚙️)]

Cool, thanks phlsa.

Got everything you need, freesamael?

Comment 15

[Samael Wang [:freesamael] (away for now)]

Yes that's all I need. Thanks.

Comment 16

[Samael Wang [:freesamael] (away for now)]

Attachment: v0

Merge "DOMServiceWorkerFocusClient" & "DOMWebNotificationClicked"
to "DOMWindowFocus" event. Utilize the event to switch tab when
loading links to an existing target tab.

Comment 17

[Samael Wang [:freesamael] (away for now)]

Attachment: v1

Clean up

Comment 18

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8806266 [details] [diff] [review]
v1

Hi Mike,

Would you help to review this patch?

Comment 19

[Samael Wang [:freesamael] (away for now)]

Hi Catalin & Nikhil,

Just want to bring your attention that I'm merging "DOMServiceWorkerFocusClient" & "DOMWebNotificationClicked"
events since they have the same behavior currently. Kindly let me know if it doesn't sound correct.

Comment 20

[Cătălin Badea (:catalinb)]

Looks good to me. Is this event also going to be used on Fennec?

Comment 21

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8806266 [details] [diff] [review]
v1

Review of attachment 8806266 [details] [diff] [review]:
-----------------------------------------------------------------

This looks mostly good - just some concerns about the tests, mostly; you should take advantage of add_task and BrowserTestUtils for readability.

I'm also not 100% sure if that's the right place in nsDocShell to put that sort of thing. It looks okay to me, but you might want smaug to double-check.

::: browser/base/content/content.js
@@ +621,5 @@
>  
>  // TODO: Load this lazily so the JSM is run only if a relevant event/message fires.
>  var pluginContent = new PluginContent(global);
>  
> +addEventListener("DOMWindowFocus", function(event) {

Can you double-check that custom events with this name fired from web content won't be handled?

::: browser/base/content/test/general/browser_bug1303838.js
@@ +1,2 @@
> +const BASE_URL = "http://example.com/browser/browser/base/content/test/general/";
> +

Please add a header docstring describing exactly what this test is testing.

@@ +2,5 @@
> +
> +add_task(function* test() {
> +  let tab = gBrowser.loadOneTab(BASE_URL + 'file_bug1303838.html', {inBackground: false});
> +  let testTab;
> +  return promiseTabLoaded(tab)

Instead of using .then(), you can take advantage of add_task and yield Promises, like:

let tab = gBrowser.loadOneTab(BASE_URL + 'file_bug1303838.html', {inBackground: false});
let testTab;
yield promiseTabLoaded(tab);
is(gBrowser.tabs.length, 2, "Check tabs.length");
is(gBrowser.selectedTab, tab, "Check selectedTab");

yield pushPrefs(['browser.tabs.loadDivertedInBackground', false]);

...etc

Don't forget to look at BrowserTestUtils for some handy utility functions for tests.

@@ +50,5 @@
> +  .then(() => gBrowser.removeTab(testTab))
> +  .then(() => gBrowser.removeTab(tab));
> +});
> +
> +function clickLink(linkId, tab, testTab)

I believe there's a utility function in BrowserTestUtils that you can use instead.

::: docshell/base/nsDocShell.cpp
@@ +10092,5 @@
>          //      do nothing.
>        }
> +
> +      // Switch to target tab if we're focused window, and the target is top
> +      // level. Take loadDivertedInBackground into acount so the behavior

Nit: "acount" -> "account"

Comment 22

[Samael Wang [:freesamael] (away for now)]

Attachment: v2

Merge "DOMServiceWorkerFocusClient" & "DOMWebNotificationClicked"
to "DOMWindowFocus" event. Utilize the event to switch tab when
loading links to an existing target tab.

MozReview-Commit-ID: Hd1NkVkrJA1

Comment 23

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8813153 [details] [diff] [review]
v2

Hi Mike,

Could you help to check the updated test case?

(In reply to Mike Conley (:mconley) from comment #21)
> > +addEventListener("DOMWindowFocus", function(event) {
> 
> Can you double-check that custom events with this name fired from web
> content won't be handled?

I only did manual test on that. The events generated from web content are untrusted events so won't be delivered to this event handler.
 
> > +function clickLink(linkId, tab, testTab)
> 
> I believe there's a utility function in BrowserTestUtils that you can use
> instead.

Changed to BrowserTestUtils.synthesizeMouse, but I still need to wait for tab loaded so I'm keep this function.

Comment 24

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8813153 [details] [diff] [review]
v2

Hi Olli,

I'm trying to make not only the first click, but all consequent clicks to a link opening in an existing tab to honor the preference "browser.tabs.loadDivertedInBackground", and switch focus to the target tab if necessary. Does the docshell modification look correct to you?

(In reply to Mike Conley (:mconley) from comment #21)
> I'm also not 100% sure if that's the right place in nsDocShell to put that
> sort of thing. It looks okay to me, but you might want smaug to double-check.

Comment 25

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

Comment on attachment 8813153 [details] [diff] [review]
v2

>@@ -10107,16 +10107,27 @@ nsDocShell::InternalLoad(nsIURI* aURI,
>         // helper-app style protocol (ie. mailto://)
>         //
>         rv = NS_OK;
>       } else if (isNewWindow) {
>         // XXX: Once new windows are created hidden, the new
>         //      window will need to be made visible...  For now,
>         //      do nothing.
>       }
>+
>+      // Switch to target tab if we're focused window, and the target is top
>+      // level. Take loadDivertedInBackground into account so the behavior
>+      // would be the same as how the tab first opened.
>+      if (isTargetTopLevelDocShell && mIsActive &&
>+          !Preferences::GetBool("browser.tabs.loadDivertedInBackground", false)) {
>+        if (NS_FAILED(nsContentUtils::DispatchFocusChromeEvent(
>+            targetDocShell->GetWindow()))) {
>+          return NS_ERROR_FAILURE;
>+        }
>+      }
>     }
Why isTargetTopLevelDocShell only?  What do other browsers do there? And we clearly at least need a test for iframes.
Please add a test and test also what other browsers do and depending on that, change this code.
And please test also the case when we don't open new tabs but new windows (can't recall the pref name for that).

Comment 26

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8813153 [details] [diff] [review]
v2

As per comment 25, the code and test need be updated so cancel review request.

Comment 27

[Samael Wang [:freesamael] (away for now)]

Attachment: v3

update

Comment 28

[Samael Wang [:freesamael] (away for now)]

Attachment: v4

update

Comment 29

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8830190 [details] [diff] [review]
v4

> What do other browsers do there?

The test case is updated with different types of links:
1. Link to load a page into the background tab.
2. Link to an anchor of the background tab.
3. Link to load a page into an iframe of the background tab.

With all 3 types of links, Chrome, Safari & Opera switch to the background tabs immediately after link clicked, and Chrome on Android has the same behavior as desktop. Edge doesn't switch at all, but it has a very good indicator - the updated tab's background flashes with orange color until user manually switches to the tab.

Would you take a look on the updated patch?

Comment 30

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

Er, hmm, do we have tests when link clicking happens in iframe?

Comment 31

[Samael Wang [:freesamael] (away for now)]

(In reply to Olli Pettay [:smaug] from comment #30)
> Er, hmm, do we have tests when link clicking happens in iframe?

No. I can update the test. Do you want to make another review for that?

Comment 32

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

rs+ for such change to the test.

Comment 33

[Samael Wang [:freesamael] (away for now)]

Attachment: v5

Update the test case.

Comment 34

[Samael Wang [:freesamael] (away for now)]

Attachment: v6

Was encountering intermittent timeout. Thought somehow location change didn't occur but turns out it's the progress listener in the test script got free'd. There wasn't any strong ref to the listener when location change occured. Silly me.

Comment 35

[Samael Wang [:freesamael] (away for now)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4259e8bffe1411bc8990d26e49ed852bce49e030&group_state=expanded&selectedJob=84271111

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/20537ebcebdb
Switch to existing target tab when clicking links. r=smaug

Comment 37

[Carsten Book [:Tomcat]]

backed out for eslint failure like https://treeherder.mozilla.org/logviewer.html#?job_id=84596145&repo=mozilla-inbound

Comment 38

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0635931c726c
Backed out changeset 20537ebcebdb for eslint failure

Comment 39

[:Gijs (he/him; back Aug 12)]

Driveby because I looked at browser/base/content/test/general/browser.ini today: Please don't put new tests into the dumping ground that is browser/base/content/test/general/.

You probably want a more relevant directory, somewhere under docshell/ or dom/ .

Comment 40

[Samael Wang [:freesamael] (away for now)]

Attachment: Switch to existing target tab when clicking links. r=smaug

rebase; address comment 37 & comment 39

Comment 41

[Samael Wang [:freesamael] (away for now)]

(In reply to :Gijs from comment #39)
> Driveby because I looked at browser/base/content/test/general/browser.ini
> today: Please don't put new tests into the dumping ground that is
> browser/base/content/test/general/.
> 
> You probably want a more relevant directory, somewhere under docshell/ or
> dom/ .

Thanks for your reminding. I often get confused about where I should put test cases.

Comment 42

[Samael Wang [:freesamael] (away for now)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1f4b1adb3f7da068468fb123342d8e72f2f657f7&group_state=expanded

Comment 43

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d47b335bd9a5
Switch to existing target tab when clicking links. r=smaug

Comment 44

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d47b335bd9a5