1394188 - Tab titles should not display hostname on reload

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect, P3)

Description

[Caspy7]

Attachment: Jumpy Tab Title

I saw someone post a gif of the new burst effect for tab loading on 57...

https://i.redd.it/eu6m5oox0whz.gif (which I've also attached just in case)

and noticed that even though they were hitting reload the title needlessly changed momentarily to the domain. We shouldn't do this. If you're on a page and hit reload 9 times out of 10 it's going to be the same title. Even if it's not, you're on the same page.

If a user hits reload on a site, they know what domain they're on, there's no need to flash it quickly.

Comment 1

[Rei Colina]

I started looking at this one today. It looks like the tab's 'title' variable gets cleared on reload. Looking into it further in order to come up with a patch.

Comment 2

[Rei Colina]

Attachment: tab-title-on-reload-fix

Attached is a patch/fix for this issue. Please review when you get a chance. The tab will now remember the last URI that the user loaded so the tab will show the URL only if it is different from the previous one.

Comment 3

[Rei Colina]

Attachment: tab-title-on-reload-fix-1

Comment 4

[Florian Quèze [:florian]]

Warning: this patch and the patch in bug 1362774 will bitrot each other.

Comment 5

[Zibi Braniecki [:zbraniecki][:gandalf]]

It seems to me like you should have this code in OnLocationChange, not in setTabTitle?

Comment 6

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

Comment on attachment 8904154 [details] [diff] [review]
tab-title-on-reload-fix-1

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

Hey Rei - thanks for the patch and for diving into tabbrowser! I have a mild concern about stashing the URI on the tab node - but I have an alternative idea for you to try. See below.

::: browser/base/content/tabbrowser.xml
@@ +1531,5 @@
> +                // don't replace the tab's label.
> +                if (tabCurrentURI.displaySpec === aTab._currentURI) {
> +                  return false;
> +                }
> +                aTab._currentURI = tabCurrentURI.displaySpec;

Hm, stashing the URI on the tab worries me a little, especially since it looks like there are plenty of cases where we won't hit this branch, so the URI and the actual current URI of the browser could go out of sync with one another.

It almost sounds like what we really want to do is to _ignore_ the DOMTitleChange that occurs with just the domain when we're reloading.

I know that nsIWebProgressListener2 offers onRefreshAttempted with a sameURI argument passed to it that we could use here... so, how about this:

In http://searchfox.org/mozilla-central/rev/67f38de2443e6b613d874fcf4d2cd1f2fc3d5e97/toolkit/content/browser-child.js#226-228 , before returning true, stash a property on the WebProgressListener global... something like _titleBeforeRefresh, and set it to content.document.title.

Then, in onLocationChange at http://searchfox.org/mozilla-central/rev/67f38de2443e6b613d874fcf4d2cd1f2fc3d5e97/toolkit/content/browser-child.js#183 , check to see if _titleBeforeRefresh exists. If so, pass that as the title instead of document.content.title, and then delete that property.

It still means stashing some state on some global, but at least here, I suspect there are fewer cases where we might go out of sync. Does my solution give the right effect?

(Note that my solution will only with with e10s, but going forward with 57, that's going to be the primarily supported configuration, I believe).

Comment 7

[Rei Colina]

Thanks for the detailed and helpful review/feedback :mconley. I will take a look and make the necessary changes based on your proposed solution within the next couple of days and get back with a new patch that we can work off. Cheers.

Comment 8

[Rei Colina]

I took a swing at the solution proposed by :mconley, however, nsIWebProgressListener2 doesn't seem to be triggering on user initiated reloads, but only on the presence of a <meta http-equiv="refresh"> or an HTTP Refresh: header. I dug in a bit deeper but couldn't find an event that notifies of a reload, at least at the browser-child.js level; which took me back to my original solution in the patch (in tabbrowser.xml). I will keep poking around to try find a different approach, but I'm open to discussions in case there are any suggestions or thoughts.

Comment 9

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

Ah, I'm sorry - you're right. onRefreshAttempted _is_ only called in those cases.

For user-initiated cases, we do send a message to browser-child.js. We send the WebNavigation:Reload message, which is received in browser-child.js here: http://searchfox.org/mozilla-central/rev/00fa5dacedb925022f53d025121f1a919508e7ce/toolkit/content/browser-child.js#296

and it's eventually handled here: http://searchfox.org/mozilla-central/rev/00fa5dacedb925022f53d025121f1a919508e7ce/toolkit/content/browser-child.js#366

Can my solution by modified using this new information?

Comment 10

[Rei Colina]

Thanks :mconley. This is getting more interesting :-) I noticed that out of all the Navigation Actions, 'WebNavigation:Reload' is the only one that doesn't get fired. All the other ones do: goBack, goForward, loadURI. Perhaps this is the smoking gun for an underlying problem? I'm trying to find out why that specific event is not being triggered at a lower level. Solving that will allow us to apply :mconley 's solution.

Comment 11

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

Wait, so you're saying that when you click on the "Reload" button, the WebNavigation:Reload message isn't sent or handled?

Comment 12

[Rei Colina]

Correct. I put logging messages inside the switch statement here: http://searchfox.org/mozilla-central/rev/00fa5dacedb925022f53d025121f1a919508e7ce/toolkit/content/browser-child.js#272 and you can see that out of all the navigation control events (goBack, goForward, stop and reload) the one for 'reload' is the only one that doesn't get received. It is almost like the Reload button is wired through a different route.

Comment 13

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

Took me a few days to get back to this, but you're right - there _is_ a different route, strangely. Here's where it happens:

http://searchfox.org/mozilla-central/rev/f6dc0e40b51a37c34e1683865395e72e7fca592c/browser/base/content/tab-content.js#68-90

So unfortunately, things are kind of spread around all over the place, which makes my plan from comment 6 a lot trickier to pull off. :( Sorry for leading you down the garden path, there.

Any ideas on better approaches here, dao?

Comment 14

[Dão Gottwald [:dao]]

It seems that it would simplify things if onLocationChange would get a location change flag for identifying reloads. bz, does this sound reasonable / feasible?

If anyone else wants to look into this, I believe the relevant onLocationChange calls originate here: https://searchfox.org/mozilla-central/rev/6e0e603f4852b8e571e5b8ae133e772b18b6016e/docshell/base/nsDocShell.cpp#1277

Comment 15

[Boris Zbarsky [:bzbarsky]]

Seems OK, and should be pretty feasible.  The relevant callsite where the location change flags are computed for this case would be https://searchfox.org/mozilla-central/source/docshell/base/nsDocShell.cpp#11418-11423 and the new flag could be added if (aLoadType & LOAD_CMD_RELOAD) tests true.

Comment 16

[Dão Gottwald [:dao]]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #15)
> Seems OK, and should be pretty feasible.  The relevant callsite where the
> location change flags are computed for this case would be
> https://searchfox.org/mozilla-central/source/docshell/base/nsDocShell.
> cpp#11418-11423 and the new flag could be added if (aLoadType &
> LOAD_CMD_RELOAD) tests true.

Permalink: https://searchfox.org/mozilla-central/rev/6e0e603f4852b8e571e5b8ae133e772b18b6016e/docshell/base/nsDocShell.cpp#11418-11423

Comment 17

[Dão Gottwald [:dao]]

Rei, if you're still around, would you like to pick this up again?

Comment 19

[Dão Gottwald [:dao]]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #15)
> Seems OK, and should be pretty feasible.  The relevant callsite where the
> location change flags are computed for this case would be
> https://searchfox.org/mozilla-central/source/docshell/base/nsDocShell.
> cpp#11418-11423 and the new flag could be added if (aLoadType &
> LOAD_CMD_RELOAD) tests true.

This doesn't seem to do anything because aFireOnLocationChange == false when this is called from a reload.

Comment 20

[Dão Gottwald [:dao]]

Attachment: Bug 1394188 - Make tab labels not change during reload. r=bzbarsky,florian

Comment 21

[Dão Gottwald [:dao]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e36fc05afada4b5ea4fe9e2a8587a3ff0f250dc3

Comment 22

[Zibi Braniecki [:zbraniecki][:gandalf]]

oh damn, this looks elegant and exactly what we need! The flag will come useful in other places around tabbar too!

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8dd7fed52818
Make tab labels not change during reload. r=bzbarsky,florian

Comment 24

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/8dd7fed52818

Comment 25

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/8dd7fed52818

Comment 26

[Md. Majedul islam]

I have reproduced this bug with Nightly 57.0a1(2017-08-26) on Windows 10, 64 bit!

The Bug's fix is now verified on Latest Nightly 66.0a1.

Build ID 	20181218095120
User Agent 	Mozilla/5.0 (Windows NT 10.0; WOW64; rv:66.0) Gecko/20100101 Firefox/66.0