1017156 - "about:preferences" briefly shows as the tab title when opening the in-content preferences

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect)

Comment 1

[Marco Castelluccio [:marco]]

IIRC, this also happens with about:addons.

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Blair, any idea what we can do here?

Comment 3

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Ah, so, this happens when we end up using gBrowser.addTab() - which happens when:
* Using some UI to open prefs (ie, not typing in the urlbar)
* Current tab isn't blank
* Prefs isn't already open

addTab() will set the initial tab label to the URL. This is the expected and wanted behaviour generally.

So, how to make this better for internal pages like prefs? I can think of two solutions, both involve whitelisting internal pages:
(1) whitelist, leave initial tab title blank for such pages
(2) whitelist, get initial tab title from a pre-defined URL->title map for such pages


Option (1) is easiest. And option (2) has the downside of needing to ensure locale strings are in sync between pages and the map, which could be a bit fragile. 

I don't think there will be a generally perceivable difference between the two options, because of the short time it takes to get the actual page title. The current glitch is perceivable because it's different text flashing visible before changing to something else - with option (1) it's merely adding a short delay, which is far less perceivable than a flash of different text. So I think option (1) would be the best approach.

Comment 4

[Dão Gottwald [:dao]]

(In reply to Blair McBride [:Unfocused] from comment #3)
> (1) whitelist, leave initial tab title blank for such pages

We have related code here that does without a whitelist:
http://hg.mozilla.org/mozilla-central/annotate/e6f113c83095/browser/base/content/tabbrowser.xml#l548

Comment 5

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Dão Gottwald [:dao] from comment #4)
> We have related code here that does without a whitelist:
> http://hg.mozilla.org/mozilla-central/annotate/e6f113c83095/browser/base/
> content/tabbrowser.xml#l548

Oh! Even better! Thanks, didn't know about that.

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Tim, do you think you could take this?

Comment 7

[Brian R. Bondy [:bbondy]]

(In reply to Dão Gottwald [:dao] from comment #4)
> (In reply to Blair McBride [:Unfocused] from comment #3)
> > (1) whitelist, leave initial tab title blank for such pages
> 
> We have related code here that does without a whitelist:
> http://hg.mozilla.org/mozilla-central/annotate/e6f113c83095/browser/base/
> content/tabbrowser.xml#l548

The code here by the way decides if it should replace the initial text with "Connecting..." or not. 

That is, we already return false for _shouldShowProgress for about:preferences and so it leaves the initial tab title. Setting it to true would show Connecting... which I think would be confusing.

http://hg.mozilla.org/mozilla-central/annotate/e6f113c83095/browser/base/content/tabbrowser.xml#l622

Comment 8

[Brian R. Bondy [:bbondy]]

what I'm doing by the way is using that call in combination with Blair's #1.  Maybe that's what Dao meant all along.

Comment 9

[Brian R. Bondy [:bbondy]]

Attachment: Patch rev1.

I'll fix about:addons and check the other about protocol links in a followup if this method is accepted. 

So I tried several things, but I didn't like the look of any of them except for the approach in this patch.

- Tried returning true from _shouldShowProgress so it would say Connecting..., but that didn't seem right for a local resource like Preferences.
- Tried returning true from _shouldShowProgress but setting setTabTitleLoading to use a blank string for this case, but having nothing in the tab name seemed wrong too.  And I think there was still a very very brief about:preferences.
- Tried returning a blank string from addTab for about: URLs but sometimes we want to preserve the URI name itself.
- Tried hard coding blank string from addTab for about:preferences, but again having nothing in the tab name for a while when loading seemed strange

Note that I couldn't use _shouldShowProgress directly in addTab because it uses the request channel and only the URI name is available in addTab.  With the approach used though there's no desire to do that.

I'd be glad to implement a different solution if you can think of anything.  The things I thought of mentioned above looked just as bad as the original problem.  Note that there is also a gear icon that shows up when it loads but isn't there when it is loading, but this patch is an improvement in the situation of text changing at least.

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8443230 [details] [diff] [review]
Patch rev1.

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

This works, and I feel that it is probably the most straight-forward KISS approach that we'll be able to get.

::: browser/locales/en-US/chrome/browser/tabbrowser.properties
@@ +31,5 @@
>  tabs.closeWarningPromptMe=Warn me when I attempt to close multiple tabs
> +#LOCALIZTION NOTE: Some tab names are preloaded to reduce visual inconsistencies
> +# See also browser/preferences/preferences.dtd prefWindow.titleWin and prefWindow.titleGNOME
> +# These should match the values there.  %S is for brandShortName.
> +tabs.preloadTitle.prefWindow.titleGNOME=%S Preferences

Let's file a follow-up to remove the brandShortName from the preferences tab, since we don't include it for the Add-ons Manager. We include it for about:home (&brandShortName; Start Page) and about:customizing (Customize &brandShortName;), but I think those two cases are special cases.

Since the tab lives within the browser window now, it is even less important to call out what application these are preferences for.

But we can do this in a follow-up in case there is some debate about this change.

Comment 11

[Dão Gottwald [:dao]]

(In reply to Brian R. Bondy [:bbondy] from comment #9)
> Created attachment 8443230 [details] [diff] [review]
> Patch rev1.
> 
> I'll fix about:addons and check the other about protocol links in a followup
> if this method is accepted. 
> 
> So I tried several things, but I didn't like the look of any of them except
> for the approach in this patch.
> 
> - Tried returning true from _shouldShowProgress so it would say
> Connecting..., but that didn't seem right for a local resource like
> Preferences.
> - Tried returning true from _shouldShowProgress but setting
> setTabTitleLoading to use a blank string for this case, but having nothing
> in the tab name seemed wrong too.  And I think there was still a very very
> brief about:preferences.

I'm not sure what you were up to here. We do not want to show progress indicators for about:preferences, so _shouldShowProgress needs to continue to return false.

> - Tried returning a blank string from addTab for about: URLs but sometimes
> we want to preserve the URI name itself.

Can you be more specific? When is "sometimes"?

> - Tried hard coding blank string from addTab for about:preferences, but
> again having nothing in the tab name for a while when loading seemed strange

Probably still better than briefly showing about:preferences, though? Since most of the time we would animate the opening tab (which means that initially, the tab label cannot be read anyway) and the proper title would be displayed before the animation is done, this doesn't sound too bad.

Comment 12

[Dão Gottwald [:dao]]

Comment on attachment 8443230 [details] [diff] [review]
Patch rev1.

I'd like to avoid embedding arbitrary knowledge about random pages in tabbrowser.xml. Granted, this patch alone doesn't make tabbrowser.xml unmaintainable, but the perspective of more and more stuff like this creeping in is concerning. Duplicating the strings seems wrong too. Since this already affects numerous other about: pages and this list is only going to grow, I don't think this approach scales well enough.

Maybe there's a clean and lightweight way to share a single list of titles to avoid the duplication. Otherwise I think we should go for making the tab label blank initially.

Comment 13

[Brian R. Bondy [:bbondy]]

Thanks for the reviews Dao and Jared.

> I'm not sure what you were up to here. We do not want to show progress indicators for about:preferences
>, so _shouldShowProgress needs to continue to return false.

What I was up to was testing out different things to see how they looked. I wanted to preserve the initially set tab name in case it is needed, but show nothing while it was loading...


> > - Tried returning a blank string from addTab for about: URLs but sometimes
> > we want to preserve the URI name itself.
> Can you be more specific? When is "sometimes"?

I was thinking of cases like: about:, about:buildconfig, about:config, ...


> Probably still better than briefly showing about:preferences, though? Since most of the time we would 
> animate the opening tab (which means that initially, the tab label cannot be read anyway) and the 
> proper title would be displayed before the animation is done, this doesn't sound too bad.

It looks ugly to me but I'm using a debug build so it's slower. 

> Maybe there's a clean and lightweight way to share a single list of titles to avoid
> the duplication. Otherwise I think we should go for making the tab label blank initially.

I can create a  new .properties file that's used by both places. I'd need to remove this XUL title though because it references something in the .dtd file which I don't think is usable within Javascript. And instead I'd programmatically set the title.

Is this the desired approach? Or should I go for a blank tab while it's loading.  The blank-ness doesn't look good to me by trying it out but I'm open to doing whatever you want here.

Comment 14

[Dão Gottwald [:dao]]

(In reply to Brian R. Bondy [:bbondy] from comment #13)
> > > - Tried returning a blank string from addTab for about: URLs but sometimes
> > > we want to preserve the URI name itself.
> > Can you be more specific? When is "sometimes"?
> 
> I was thinking of cases like: about:, about:buildconfig, about:config, ...

Technically these pages aren't different. They too have their titles specified in the markup, it just happens that they're the same as the URI. These pages aren't primary or secondary UI (there aren't even addTab calls for them), so we shouldn't optimize for them.

> > Maybe there's a clean and lightweight way to share a single list of titles to avoid
> > the duplication. Otherwise I think we should go for making the tab label blank initially.
> 
> I can create a  new .properties file that's used by both places. I'd need to
> remove this XUL title though because it references something in the .dtd
> file which I don't think is usable within Javascript. And instead I'd
> programmatically set the title.

Right. It would be two .properties files, though, since these pages are scattered across toolkit and browser...

> Is this the desired approach? Or should I go for a blank tab while it's
> loading.  The blank-ness doesn't look good to me by trying it out but I'm
> open to doing whatever you want here.

Assuming the blank label would be an incremental improvement over what we have now, I would vote for doing that here and exploring the more involved shared-titles solution in a followup.

Comment 15

[Brian R. Bondy [:bbondy]]

Ya I think it's an incremental improvement but not as nice visually (nicer code wise) as the first solution.  I'll go for that route.  So just to avoid an extra round of patches that aren't needed. Are you OK with checking if the URI starts with about: inside addTab and, if so having a blank label?  Thanks for your help.

Comment 16

[Dão Gottwald [:dao]]

(In reply to Brian R. Bondy [:bbondy] from comment #15)
> So just to avoid
> an extra round of patches that aren't needed. Are you OK with checking if
> the URI starts with about: inside addTab and, if so having a blank label?

It's not perfect since about: URIs can point to remote resources (e.g. about:credits), but probably good enough for now.

Comment 17

[Brian R. Bondy [:bbondy]]

In those cases I think the blank tab will almost immediately be replaced with Connecting... because _shouldShowProgress only returns false for URIs pointing to local resources.

Comment 18

[Dão Gottwald [:dao]]

Good point, although this raises the question why addTab should ever set the label to the URI...

Comment 19

[Brian R. Bondy [:bbondy]]

I was thinking because of cases where the document doesn't set a title, but I think you mentioned that doesn't happen.

Comment 20

[Dão Gottwald [:dao]]

(In reply to Brian R. Bondy [:bbondy] from comment #19)
> I was thinking because of cases where the document doesn't set a title, but
> I think you mentioned that doesn't happen.

It shouldn't happen for our about: pages. It may happen for random other pages we don't control. However, setTabTitle will set the tab label to the URI in that case; this isn't addTab's business.

Comment 21

[Brian R. Bondy [:bbondy]]

So I guess it has the advantage of showing the URI and not something blank when the URI is loading and it ends up being a URI. But it seems like that is rare if it even happens.  I'll just change it to blank and we can special case it to about: URIs if needed.

Comment 22

[Dão Gottwald [:dao]]

(In reply to Brian R. Bondy [:bbondy] from comment #21)
> So I guess it has the advantage of showing the URI and not something blank
> when the URI is loading and it ends up being a URI. But it seems like that
> is rare if it even happens.

The URI wouldn't stick around until the page is loaded anyway. Like you said, shortly after addTab sets it, it would be replaced by "Connecting...".

Comment 23

[Brian R. Bondy [:bbondy]]

Attachment: Patch rev2. Don't set URI as tab title in addTab

Comment 24

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1 - test fix

Comment 25

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8444461 [details] [diff] [review]
Patch v1 - test fix

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

::: browser/base/content/test/plugins/browser_pluginnotification.js
@@ +377,5 @@
>      if (event.type == "TabOpen") {
>        gBrowser.tabContainer.removeEventListener("TabOpen", this, false);
>        this.tab = event.originalTarget;
> +      // TabOpen is fired within addTab, and tab name should be blank at this point.
> +      is(event.target.label, "", "Test 18a, Update link should open up the plugin check page");

This doesn't verify that the correct link was opened anymore. Maybe have the TabOpen listener add a 'load' event listener which then checks the currentURI.spec with this.url?

Comment 26

[Dão Gottwald [:dao]]

Comment on attachment 8444461 [details] [diff] [review]
Patch v1 - test fix

This test doesn't really care about the tab label, it just used that as a means to check that a tab with the expected URL was opened. Checking that the label is empty removes the actually interesting part of that test. Assuming that the tab is selected, maybe you can check gURLBar.value instead?

Comment 27

[Brian R. Bondy [:bbondy]]

Will do one of those, thanks for the suggestions.

Comment 28

[Brian R. Bondy [:bbondy]]

Attachment: Patch v2 - test fix

The pageshow event already does a url check. So I just used the default handleEvent of TabOpenListener.

Comment 29

[Brian R. Bondy [:bbondy]]

https://hg.mozilla.org/integration/fx-team/rev/c8e3aff6a401
https://hg.mozilla.org/integration/fx-team/rev/753f14cf4b91

Comment 30

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

https://hg.mozilla.org/mozilla-central/rev/c8e3aff6a401
https://hg.mozilla.org/mozilla-central/rev/753f14cf4b91

Comment 31

[Marco Mucci [:MarcoM]]

Hey Brian, should this bug be marked as [qa+] or [qa-] for QA verification.

Comment 32

[Marco Mucci [:MarcoM]]

Hi Florin, can a contact be assigned to this bug for QA verification.

Comment 33

[Camelia Badau [:cbadau], Desktop QA]

Verified fixed on Windows 7 64bit, Ubuntu 13.10 32bit and Mac OSX 10.7.5 using latest Nightly 33.0a1 (buildID: 20140625030206).