1177008 - Loading about:preferences#[category] urls in tabs causes these tabs to have no icon ( if [category] is not "general" )

Status: RESOLVED DUPLICATE of bug 519028

(Firefox :: Settings UI, defect)

Description

[arni2033]

Attachment: Refreshing about-preferences causes tab to lose icon.webm

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

Steps to reproduce:

1. Open page about:preferences
2. Click to a category on the left (but not "general")
3. Press Ctrl+R to reload tab


Actual results:

The icon on tab (in the tabs toolbar) disappeared.

Comment 1

[Loic]

Confimed.

Comment 2

[arni2033]

Just want to be clear. This bug happens if you:

a) open about:preferences, click category and then refresh tab (just like in original Steps To Reproduce)
b) click Australis Menu -> Sync (this opens about:preferences#sync in a new tab)
c) click searchbar -> Change Search Settings (this opens about:preferences#search in a new tab)
d) left about:preferences#[category] opened from last session
e) send link about:preferences#[category] to somebody

Comment 3

[arni2033]

It started to happen right after bug 754304
> https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=bdac18bd6c74&tochange=776f6d341b3f

Comment 4

[Matthew N. [:MattN]]

Sounds like a dupe of bug 408415?

Comment 6

[arni2033]

How is this a duplicate, if there's clearly a way NOT to re-set hash when loading preference URLs?
That is a way better option than waiting another 8 years until bug 408415 is fixed.

Comment 7

[:Gijs (out for now; he/him)]

(In reply to arni2033 from comment #6)
> How is this a duplicate, if there's clearly a way NOT to re-set hash when
> loading preference URLs?

Is there? It's certainly not "clearly" the case. The JS code seems to already avoid setting location.hash in cases where it would be the same. Without more specific information, this is not a helpful comment, especially when adding

> That is a way better option than waiting another 8 years until bug 408415 is
> fixed.

that kind of passive-aggressive commentary.

Comment 8

[arni2033]

(In reply to :Gijs Kruitbosch from comment #7)
> The JS code seems to already avoid setting location.hash in cases where it would be the same.

preferences.js, function gotoPref. Here's some code in the beginning:
  if (gLastHash == category)
    return;
  let item = categories.querySelector(".category[value=" + category + "]");
  if (!item) {
    category = kDefaultCategoryInternalName;
    item = categories.querySelector(".category[value=" + category + "]");
  }

And here's some code in the middle:
  let newHash = internalPrefCategoryNameToFriendlyName(category);
  if (gLastHash || category != kDefaultCategoryInternalName) {
    document.location.hash = newHash;
  }
  // Need to set the gLastHash before setting categories.selectedItem since
  // the categories 'select' event will re-enter the gotoPref codepath.
  gLastHash = category;

I looked precisely at location.hash change in the last block of code.
I concluded that the only purpose of that is to display actual "friendly name" of category in urlbar
However, it will run gotoPref() again [no big deal, because when it will happen, "gLastHash" will already be equal to "category"]. But also, we will run into bug 408415 [bad].
So it's better to only change hash if it's really needed. I.e. add 3 lines
>   let newHash = internalPrefCategoryNameToFriendlyName(category);
>   if (gLastHash || category != kDefaultCategoryInternalName) {
> +   if (newHash != hash.substr(1)) {
> +   // Avoid unnecessary changes to location.hash
>       document.location.hash = newHash;
> +   }
>   }
I would attach a patch, but that'd require 6-7 hours of installing everything, building, and so on.

Comment 9

[arni2033]

I forgot to mention that if you follow STR in comment 0, then gotoPref() is executed normally until you reach that part where location.hash changes (i.e. there's no "return" to leave the function).
gLastHash == "", and category != kDefaultCategoryInternalName.
So STR in comment 0 lead to unnecessary hash change.

Suggested 3 lines of code would protect from this scenario w/o breaking anything in gotoPref() and onHashChange(), because none of them rely on launching gotoPref() 2 times.

Comment 10

[:Gijs (out for now; he/him)]

I put up a patch in bug 408415, so changing about:preferences is now not necessary anymore.

(In reply to arni2033 from comment #8)
> So it's better to only change hash if it's really needed. I.e. add 3 lines
> >   let newHash = internalPrefCategoryNameToFriendlyName(category);
> >   if (gLastHash || category != kDefaultCategoryInternalName) {
> > +   if (newHash != hash.substr(1)) {
> > +   // Avoid unnecessary changes to location.hash
> >       document.location.hash = newHash;
> > +   }
> >   }

This seems like it would work, though it seems to me that a more correct solution might be to update gLastHash with hash.substr(1) before we hit this block, and to check gLasthash != newHash (I think we could get rid of the kDefaultCategoryInternalName check in that case, but I've not verified that. It's possible that would still have other benefits, even with 408415 fixed.

> I would attach a patch, but that'd require 6-7 hours of installing
> everything, building, and so on.

It's true that installing mozillabuild and getting an initial copy of mozilla-central is time-consuming if you're not on a very-high-bandwidth connection. Hard to put a number on that without knowing more about what connection you're on.

However, building takes only about 5 minutes the first time, if all you're doing is frontend changes, and likely <20 seconds after the initial build. See https://developer.mozilla.org/en-US/docs/Artifact_builds . So... I'm biased, but I would say it'd probably be worth doing in the long run, as you've clearly filed enough bugs where you could potentially write the patch when filing it.