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.

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 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.

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

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).

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?

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.

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

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.

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?

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

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.

(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?

(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.

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

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