Closed
Bug 1427186
Opened 6 years ago
Closed 6 years ago
In Private Browsing tabs the title "New Tab" appears, before the title "Private Browsing" appears.
Categories
(Firefox :: General, defect, P3)
Firefox
General
Tracking
()
VERIFIED
FIXED
Firefox 61
People
(Reporter: mehmet.sahin, Assigned: Gijs)
References
(Blocks 1 open bug)
Details
(Whiteboard: [fxperf:p1])
Attachments
(3 files, 1 obsolete file)
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•6 years ago
|
||
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•6 years ago
|
||
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.
Updated•6 years ago
|
Component: Private Browsing → General
Component: General → Private Browsing
Component: Private Browsing → New Tab Page
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?
Comment 5•6 years ago
|
||
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.
Updated•6 years ago
|
Whiteboard: [fxperf]
Assignee | ||
Comment 6•6 years ago
|
||
I'll look at fixing this once bug 1430751 lands.
Assignee | ||
Updated•6 years ago
|
Whiteboard: [fxperf:p2] → [fxperf:p1]
Comment hidden (mozreview-request) |
Comment 8•6 years ago
|
||
mozreview-review |
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+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Comment 11•6 years ago
|
||
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
Comment 12•6 years ago
|
||
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
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(gijskruitbosch+bugs)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(gijskruitbosch+bugs)
Comment 15•6 years ago
|
||
mozreview-review |
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)
Comment 16•6 years ago
|
||
Does this need to an update too? https://searchfox.org/mozilla-central/rev/44fa24847e4e73ce8932de4c8cf47559302e4ba2/browser/base/content/tabbrowser.xml#119
Assignee | ||
Comment 17•6 years ago
|
||
(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)
Comment 18•6 years ago
|
||
(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)
Assignee | ||
Comment 19•6 years ago
|
||
(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)
Comment 20•6 years ago
|
||
(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)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8957570 -
Attachment is obsolete: true
Assignee | ||
Comment 23•6 years ago
|
||
mozreview-review |
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•6 years ago
|
||
(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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 27•6 years ago
|
||
mozreview-review |
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+
Comment 28•6 years ago
|
||
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
Comment 29•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8edb52693ba5 https://hg.mozilla.org/mozilla-central/rev/6000d2089392
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Reporter | ||
Comment 30•6 years ago
|
||
Great
Reporter | ||
Comment 31•6 years ago
|
||
Great! Thank you!
Comment 32•6 years ago
|
||
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•6 years ago
|
||
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]
Comment 34•6 years ago
|
||
As par comment 32 & comment 33 i am marking this bug as verified fixed.
Status: RESOLVED → VERIFIED
Updated•6 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•