Closed Bug 1394188 Opened 2 years ago Closed Last year

Tab titles should not display hostname on reload

Categories

(Firefox :: Tabbed Browser, defect, P3)

defect

Tracking

()

RESOLVED FIXED
Firefox 66
Tracking Status
firefox66 --- fixed

People

(Reporter: caspy77, Assigned: dao)

References

(Blocks 3 open bugs)

Details

(Whiteboard: [fxperf:p3])

Attachments

(3 files, 1 obsolete file)

Attached image 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.
Depends on: 1366870
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.
Attached patch tab-title-on-reload-fix (obsolete) — Splinter Review
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.
Attachment #8904153 - Flags: review?(wmccloskey)
Attachment #8904153 - Attachment is obsolete: true
Attachment #8904153 - Flags: review?(wmccloskey)
Attachment #8904154 - Flags: review?(wmccloskey)
Attachment #8904154 - Flags: review?(wmccloskey)
Attachment #8904154 - Flags: review?(mconley)
Attachment #8904154 - Flags: review?(dao+bmo)
Warning: this patch and the patch in bug 1362774 will bitrot each other.
Assignee: nobody → reinaldo13+bugzilla
It seems to me like you should have this code in OnLocationChange, not in setTabTitle?
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).
Attachment #8904154 - Flags: review?(mconley) → review-
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.
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.
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?
Flags: needinfo?(reinaldo13+bugzilla)
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.
Flags: needinfo?(reinaldo13+bugzilla)
Wait, so you're saying that when you click on the "Reload" button, the WebNavigation:Reload message isn't sent or handled?
Flags: needinfo?(reinaldo13+bugzilla)
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.
Flags: needinfo?(reinaldo13+bugzilla) → needinfo?(mconley)
Priority: -- → P3
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?
Flags: needinfo?(mconley) → needinfo?(dao+bmo)
Whiteboard: [fxperf]
Whiteboard: [fxperf] → [fxperf:p2]
See Also: → 1401091
Attachment #8904154 - Flags: review?(dao+bmo)
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
Flags: needinfo?(dao+bmo) → needinfo?(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.
Flags: needinfo?(bzbarsky)
(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
Rei, if you're still around, would you like to pick this up again?
Flags: needinfo?(reinaldo13+bugzilla)
See Also: → 1506072
See Also: 1506072
Duplicate of this bug: 1506072
Whiteboard: [fxperf:p2] → [fxperf:p3]
Assignee: reinaldo13+bugzilla → nobody
Flags: needinfo?(reinaldo13+bugzilla)
(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.
Assignee: nobody → dao+bmo
Status: NEW → ASSIGNED
oh damn, this looks elegant and exactly what we need! The flag will come useful in other places around tabbar too!
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8dd7fed52818
Make tab labels not change during reload. r=bzbarsky,florian
https://hg.mozilla.org/mozilla-central/rev/8dd7fed52818
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 66
Blocks: 1515000
Blocks: 1515003
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
QA Whiteboard: [bugday-20181219]
You need to log in before you can comment on or make changes to this bug.