Closed Bug 1427186 Opened 5 years ago Closed 5 years ago

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

Categories

(Firefox :: General, defect, P3)

Product:
Firefox
Component:
General
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 61
Tracking Flags:
Tracking Status
firefox59 --- wontfix
firefox60 --- wontfix
firefox61 --- verified

People

(Reporter: mehmet.sahin, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

(Whiteboard: [fxperf:p1])

Attachments

(3 files, 1 obsolete file)

Private_newTab.mov
2.18 MB, video/quicktime
Details
Bug 1427186 - stop ever showing 'new tab' as a title for about:privatebrowsing in private windows,
59 bytes, text/x-review-board-request
dao
: review+
Details
Bug 1427186 - treat empty tab title as non-content tab title (use default title) and update test,
59 bytes, text/x-review-board-request
dao
: review+
Details
Attached video 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.
Component: Private Browsing → General
Sorry about that, I missed that :jdm had moved this out of Private Browsing.
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?
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.
Blocks: photon-perf-windows
Component: New Tab Page → General
Priority: -- → P3
Whiteboard: [fxperf]
I'll look at fixing this once bug 1430751 lands.
Assignee: nobody → gijskruitbosch+bugs
Depends on: 1430751
Whiteboard: [fxperf] → [fxperf:p2]
Whiteboard: [fxperf:p2] → [fxperf:p1]
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
Attachment #8956946 - Flags: review?(dao+bmo) → review+
Status: NEW → ASSIGNED
status-firefox59: affected → wontfix
status-firefox60: --- → affected
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/4e5253710464
stop ever showing 'new tab' as a title for about:privatebrowsing in private windows, r=dao
Backout by rgurzau@mozilla.com:
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
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(gijskruitbosch+bugs)
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?
Attachment #8957570 - Flags: review?(dao+bmo)
(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)?
Flags: needinfo?(dao+bmo)
(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.
Flags: needinfo?(dao+bmo)
(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...
Flags: needinfo?(dao+bmo)
(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.
Flags: needinfo?(dao+bmo)
Attachment #8957570 - Attachment is obsolete: true
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.
(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 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
Attachment #8957964 - Flags: review?(dao+bmo) → review+
Pushed by gijskruitbosch@gmail.com:
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
https://hg.mozilla.org/mozilla-central/rev/8edb52693ba5
https://hg.mozilla.org/mozilla-central/rev/6000d2089392
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Great 
Great! Thank you!
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]
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
QA Whiteboard: [bugday-20180314]
As par comment 32 & comment 33 i am marking this bug as verified fixed.
Status: RESOLVED → VERIFIED
status-firefox61: fixed → verified
Blocks: 1448482
Blocks: 1377071
You need to log in before you can comment on or make changes to this bug.