1427186 - In Private Browsing tabs the title "New Tab" appears, before the title "Private Browsing" appears.

Status: VERIFIED FIXED

(Firefox :: General, defect, P3)

Description

[Mehmet]

Attachment: Private_newTab.mov

macOS 10.12.6 (but probably OS=All)
FF 59.0a1 (2017-12-27) (64-Bit)

1.) Open a Private Window and some Private New Tabs in it
2.) Take a look at the title of the Tab during the creation

Expected: Just only show the title "Private Browsing" while creating the tab.

Actual: The title "New Tab" appears, before the title "Private Browsing" appears.

Please find attached a screencast.

Thanks.

Comment 1

[Josh Matthews [:jdm]]

Presumably this occurs because https://dxr.mozilla.org/mozilla-central/rev/c2daa4c032b7f23a708a89237940ac8c49bba6cf/browser/components/privatebrowsing/content/aboutPrivateBrowsing.js#57 occurs in reaction to the DOMContentLoaded event.

Comment 2

[Josh Matthews [:jdm]]

It's not clear to me why that is preferable to adding a default <title> to https://dxr.mozilla.org/mozilla-central/rev/c2daa4c032b7f23a708a89237940ac8c49bba6cf/browser/components/privatebrowsing/content/aboutPrivateBrowsing.xhtml#21-26.

Comment 3

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

Sorry about that, I missed that :jdm had moved this out of Private Browsing.

Comment 4

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

Actually, this feels like a product matter. Jeff, does product have an opinion on how a new tab's title should behave in private browsing?

Comment 5

[Johann Hofmann [:johannh]]

I don't think there's something to decide here, it's just flicker we need to remove :)

I also think "New Tab Page" is the wrong component. It's either General or Private Browsing, really.

Comment 6

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

I'll look at fixing this once bug 1430751 lands.

Comment 7

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

Attachment: Bug 1427186 - stop ever showing 'new tab' as a title for about:privatebrowsing in private windows,

There are 3 issues here:
- the runtime changing of the title causes issues where the content title is
  not set. This is fixed by setting it to the private title by default;
  there is very little (if any) UI that allows users to even open
  about:privatebrowsing in 'normal' windows so we care a lot less about
  flicker there. To be able to include this title in the markup, we switched
  to a dtd.
- the 'empty tab' title we set on the tab initially is set when the tab is
  created. This has been updated to check for private windows, and default to
  'Private Browsing' instead. This will obviously also affect other new tabs
  in private browsing, but that seems desirable/consistent.
- Likewise, we use this title when updating a tab's title that we don't yet
  have a content title for (which can still happen while about:privatebrowsing
  is loading because e10s), and so that is updated, too.

Review commit: https://reviewboard.mozilla.org/r/225890/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/225890/

Comment 8

[Dão Gottwald [:dao]]

Comment on attachment 8956946 [details]
Bug 1427186 - stop ever showing 'new tab' as a title for about:privatebrowsing in private windows,

https://reviewboard.mozilla.org/r/225890/#review231984

::: browser/base/content/tabbrowser.js:256
(Diff revision 1)
>        newValue => Math.max(newValue, this._tabMinWidthLimit),
>      );
>  
>      this.tabMinWidth = this.tabMinWidthPref;
>  
> +    XPCOMUtils.defineLazyGetter(this, "emptyTabTitle", function() {

nit: _emptyTabTitle and use an arrow function

Comment 9

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

Comment on attachment 8956946 [details]
Bug 1427186 - stop ever showing 'new tab' as a title for about:privatebrowsing in private windows,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/225890/diff/1-2/

Comment 10

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

Comment on attachment 8956946 [details]
Bug 1427186 - stop ever showing 'new tab' as a title for about:privatebrowsing in private windows,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/225890/diff/2-3/

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4e5253710464
stop ever showing 'new tab' as a title for about:privatebrowsing in private windows, r=dao

Comment 12

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2b44ea19f287
Backed out changeset 4e5253710464 for failing ss tests on /mozscreenshots/primaryUI/browser_primaryUI.js and for failing bc tests on /privatebrowsing/test/browser/browser_privatebrowsing_windowtitle.js on a CLOSED TREE

Comment 13

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

Attachment: Bug 1427186 - update titlebar even if labels match, iff the tab is selected and this title came from content,

Review commit: https://reviewboard.mozilla.org/r/226450/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/226450/

Comment 14

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

Comment on attachment 8956946 [details]
Bug 1427186 - stop ever showing 'new tab' as a title for about:privatebrowsing in private windows,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/225890/diff/3-4/

Comment 15

[Dão Gottwald [:dao]]

Comment on attachment 8957570 [details]
Bug 1427186 - update titlebar even if labels match, iff the tab is selected and this title came from content,

https://reviewboard.mozilla.org/r/226450/#review232578

::: browser/base/content/tabbrowser.js:1366
(Diff revision 1)
> +    let differentContentTitleState = !!aOptions.isContentTitle != !!aTab._labelIsContentTitle;
>      aTab._labelIsContentTitle = aOptions.isContentTitle;
>  
>      if (aTab.getAttribute("label") == aLabel) {
> +      if (differentContentTitleState && aTab.selected) {
> +        this.updateTitlebar();

I'm a bit confused as to why this is needed. Is this transitioning from "is not a content title" to "is content title" or vice versa? Was updateTitlebar not called in the previous state or did just not do the right thing?

Comment 16

[Dão Gottwald [:dao]]

Does this need to an update too? https://searchfox.org/mozilla-central/rev/44fa24847e4e73ce8932de4c8cf47559302e4ba2/browser/base/content/tabbrowser.xml#119

Comment 17

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

(In reply to Dão Gottwald [::dao] from comment #15)
> Comment on attachment 8957570 [details]
> Bug 1427186 - update titlebar even if labels match, iff the tab is selected
> and this title came from content,
> 
> https://reviewboard.mozilla.org/r/226450/#review232578
> 
> ::: browser/base/content/tabbrowser.js:1366
> (Diff revision 1)
> > +    let differentContentTitleState = !!aOptions.isContentTitle != !!aTab._labelIsContentTitle;
> >      aTab._labelIsContentTitle = aOptions.isContentTitle;
> >  
> >      if (aTab.getAttribute("label") == aLabel) {
> > +      if (differentContentTitleState && aTab.selected) {
> > +        this.updateTitlebar();
> 
> I'm a bit confused as to why this is needed. Is this transitioning from "is
> not a content title" to "is content title" or vice versa?

From "not a content title" to "is a content title", but the title stays the same (ie "Private Browsing" in the private browsing case).

> Was updateTitlebar
> not called in the previous state or did just not do the right thing?

Not 100% sure what you're asking. `updateTitlebar` will have been called for the opening a new tab / new window, but when the title is not a content title (so, previously when it was "New Tab" and now, post-patch, when it is "Private Browsing") we don't include it, we show the app name instead (so just "Nightly" on a normal window and "Nightly - (Private Browsing)" on a private window).

Does that help? :-)

(In reply to Dão Gottwald [::dao] from comment #16)
> Does this need to an update too?
> https://searchfox.org/mozilla-central/rev/
> 44fa24847e4e73ce8932de4c8cf47559302e4ba2/browser/base/content/tabbrowser.
> xml#119

Oh, good catch, yes. Should I just use the getter I defined on the tabbrowser, or should I move the getter to the tabstrip instead (and update the tabbrowser uses to use it from this.tabContainer)?

Comment 18

[Dão Gottwald [:dao]]

(In reply to :Gijs (under the weather; responses will be slow) from comment #17)
> > I'm a bit confused as to why this is needed. Is this transitioning from "is
> > not a content title" to "is content title" or vice versa?
> 
> From "not a content title" to "is a content title", but the title stays the
> same (ie "Private Browsing" in the private browsing case).
> 
> > Was updateTitlebar
> > not called in the previous state or did just not do the right thing?
> 
> Not 100% sure what you're asking. `updateTitlebar` will have been called for
> the opening a new tab / new window, but when the title is not a content
> title (so, previously when it was "New Tab" and now, post-patch, when it is
> "Private Browsing") we don't include it, we show the app name instead (so
> just "Nightly" on a normal window and "Nightly - (Private Browsing)" on a
> private window).

Ideally we'd want the window title to be the same in both states, right? Be it "New Tab - Firefox Nightly" or just "Firefox Nightly" -- probably the latter.

Comment 19

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

(In reply to Dão Gottwald [::dao] from comment #18)
> Ideally we'd want the window title to be the same in both states, right? Be
> it "New Tab - Firefox Nightly" or just "Firefox Nightly" -- probably the
> latter.

OK, in that case I will update the test expectation(s) and do a trypush with that. FWIW, I tried asking you about this on IRC yesterday, but I suspect that got lost somehow ( https://mozilla.logbot.info/fx-team/20180309#c14431575 ) .

Still curious about this:

(In reply to :Gijs (under the weather; responses will be slow) from comment #17)
> (In reply to Dão Gottwald [::dao] from comment #15)
> (In reply to Dão Gottwald [::dao] from comment #16)
> > Does this need to an update too?
> > https://searchfox.org/mozilla-central/rev/
> > 44fa24847e4e73ce8932de4c8cf47559302e4ba2/browser/base/content/tabbrowser.
> > xml#119
> 
> Oh, good catch, yes. Should I just use the getter I defined on the
> tabbrowser, or should I move the getter to the tabstrip instead (and update
> the tabbrowser uses to use it from this.tabContainer)?

I would prefer not to make the tabbrowser/tabstrip init story worse here...

Comment 20

[Dão Gottwald [:dao]]

(In reply to :Gijs (under the weather; responses will be slow) from comment #19)
> (In reply to Dão Gottwald [::dao] from comment #18)
> > Ideally we'd want the window title to be the same in both states, right? Be
> > it "New Tab - Firefox Nightly" or just "Firefox Nightly" -- probably the
> > latter.
> 
> OK, in that case I will update the test expectation(s) and do a trypush with
> that. FWIW, I tried asking you about this on IRC yesterday, but I suspect
> that got lost somehow (
> https://mozilla.logbot.info/fx-team/20180309#c14431575 ) .

Yeah, didn't see it.

> (In reply to :Gijs (under the weather; responses will be slow) from comment
> #17)
> > (In reply to Dão Gottwald [::dao] from comment #15)
> > (In reply to Dão Gottwald [::dao] from comment #16)
> > > Does this need to an update too?
> > > https://searchfox.org/mozilla-central/rev/
> > > 44fa24847e4e73ce8932de4c8cf47559302e4ba2/browser/base/content/tabbrowser.
> > > xml#119
> > 
> > Oh, good catch, yes. Should I just use the getter I defined on the
> > tabbrowser, or should I move the getter to the tabstrip instead (and update
> > the tabbrowser uses to use it from this.tabContainer)?
> 
> I would prefer not to make the tabbrowser/tabstrip init story worse here...

This can stay in gBrowser, I think, without the underscore. The tabbrowser-tabs constructor already needs gBrowser.

Comment 21

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

Attachment: Bug 1427186 - treat empty tab title as non-content tab title (use default title) and update test,

Review commit: https://reviewboard.mozilla.org/r/226880/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/226880/

Comment 22

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

Comment on attachment 8956946 [details]
Bug 1427186 - stop ever showing 'new tab' as a title for about:privatebrowsing in private windows,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/225890/diff/4-5/

Comment 23

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

Comment on attachment 8957964 [details]
Bug 1427186 - treat empty tab title as non-content tab title (use default title) and update test,

https://reviewboard.mozilla.org/r/226880/#review232628

::: browser/base/content/tabbrowser.js:873
(Diff revision 1)
> -    if (!docTitle)
> +    if (!docTitle || docTitle == this.emptyTabTitle)
>        docTitle = docElement.getAttribute("titledefault");

I needed to do this to ensure that in the case where we move a tab into its own window, and then run `updateTitlebar` against the data for the tab at that point, we really still take the 'this is an empty tab, so use the app/default title' path. This is verified in the test in this commit...

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_windowtitle.js:57
(Diff revision 1)
>      let win = aWindow.gBrowser.replaceTabWithWindow(tab);
>      await BrowserTestUtils.waitForEvent(win, "load", false);
>  
>      await BrowserTestUtils.waitForCondition(() => {
>        return win.document.title === expected_title;
> -    }, `Window title should be ${expected_title}, got ${aWindow.document.title}`);
> +    }, `Window title should be ${expected_title}, got ${win.document.title}`);

... where I noticed that this logging was broken.

Comment 24

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [::dao] from comment #20)
> > (In reply to :Gijs (under the weather; responses will be slow) from comment
> > #17)
> > > (In reply to Dão Gottwald [::dao] from comment #15)
> > > (In reply to Dão Gottwald [::dao] from comment #16)
> > > > Does this need to an update too?
> > > > https://searchfox.org/mozilla-central/rev/
> > > > 44fa24847e4e73ce8932de4c8cf47559302e4ba2/browser/base/content/tabbrowser.
> > > > xml#119
> > > 
> > > Oh, good catch, yes. Should I just use the getter I defined on the
> > > tabbrowser, or should I move the getter to the tabstrip instead (and update
> > > the tabbrowser uses to use it from this.tabContainer)?
> > 
> > I would prefer not to make the tabbrowser/tabstrip init story worse here...
> 
> This can stay in gBrowser, I think, without the underscore. The
> tabbrowser-tabs constructor already needs gBrowser.

Actually, let's put this on the tab container. That currently initializes gBrowser before gBrowserInit and I think we can avoid that. I'll post a patch in bug 1443849 in a bit.

Comment 25

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

Comment on attachment 8956946 [details]
Bug 1427186 - stop ever showing 'new tab' as a title for about:privatebrowsing in private windows,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/225890/diff/5-6/

Comment 26

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

Comment on attachment 8957964 [details]
Bug 1427186 - treat empty tab title as non-content tab title (use default title) and update test,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/226880/diff/1-2/

Comment 27

[Dão Gottwald [:dao]]

Comment on attachment 8957964 [details]
Bug 1427186 - treat empty tab title as non-content tab title (use default title) and update test,

https://reviewboard.mozilla.org/r/226880/#review233252

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8edb52693ba5
stop ever showing 'new tab' as a title for about:privatebrowsing in private windows, r=dao
https://hg.mozilla.org/integration/autoland/rev/6000d2089392
treat empty tab title as non-content tab title (use default title) and update test, r=dao

Comment 29

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/8edb52693ba5
https://hg.mozilla.org/mozilla-central/rev/6000d2089392

Comment 30

[Mehmet]

Great 

Comment 31

[Mehmet]

Great! Thank you!

Comment 32

[Sajedul Islam]

I have reproduced this bug with Nightly 59.0a1 (2017-12-27) on Windows 10 , 64 Bit ! 

This bug's fix is Verified with latest Nightly !

Build   ID    20180317100337
User Agent    Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:61.0) Gecko/20100101 Firefox/61.0

[bugday-20180314]

Comment 33

[Mohammad Maruf Rahman]

I have reproduced this bug with Nightly 59.0a1 (2017-12-27) on Ubuntu 16.04 LTS!

This bug's fix is Verified with latest Nightly!

Build   ID    20180317111042
User Agent    Mozilla/5.0 (X11; Linux x86_64; rv:61.0) Gecko/20100101 Firefox/61.0

Comment 34

[Sajedul Islam]

As par comment 32 & comment 33 i am marking this bug as verified fixed.