Closed
Bug 736476
Opened 12 years ago
Closed 12 years ago
Title page for New tab is about:newtab after restart
Categories
(Firefox :: General, defect)
Tracking
()
VERIFIED
FIXED
Firefox 14
People
(Reporter: virgil.dicu, Assigned: bellindira)
References
Details
Attachments
(1 file, 3 obsolete files)
1.07 KB,
patch
|
akeybl
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
Mozilla/5.0 (Windows NT 5.1; rv:13.0) Gecko/20120315 Firefox/13.0a2 1. Start firefox. 2. Select "show windows from last time" and "don't load tabs until selected". 3. Open two new tabs. 4. Restart. Actual results: "about:newtab" in the title page instead of "New tab" for the two tabs opened at step 3. Expected: "new tab" in the title page. remember the title name before shutdown, as any regular tab or as about:blank
Updated•12 years ago
|
Assignee: nobody → bellindira
Assignee | ||
Updated•12 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•12 years ago
|
||
Applied the same logic used for about:blank to set title page.
Attachment #614649 -
Flags: review?(ttaubert)
Comment 2•12 years ago
|
||
Comment on attachment 614649 [details] [diff] [review] Patch Review of attachment 614649 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for tackling this, Bellindira. I had a look at it and actually we shouldn't need to hard-code this in the session store service. The tab has a title and should be saved no matter what the actual URL behind this is: http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/nsSessionStore.js#1989 Interestingly, applying this little change fixes the problem (but yields other side-effects and shouldn't be needed): > diff --git a/browser/base/content/newtab/newTab.xul b/browser/base/content/newtab/newTab.xul > --- a/browser/base/content/newtab/newTab.xul > +++ b/browser/base/content/newtab/newTab.xul > > <xul:window id="newtab-window" xmlns="http://www.w3.org/1999/xhtml" > xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" > - xul:disablefastfind="true" xul:title="&newtab.pageTitle;"> > + xul:disablefastfind="true" title="&newtab.pageTitle;"> I'm not exactly sure why using html:title="" is any different from using xul:title="" but hopefully this helps as a little hint.
Attachment #614649 -
Flags: review?(ttaubert)
Assignee | ||
Comment 3•12 years ago
|
||
Thanks! I'll check what's the difference because my first approach was trying to save all titles however in the aEntry parameter in _serializeHistoryEntry, title was always empty on xul pages cases.
Assignee | ||
Comment 4•12 years ago
|
||
The title is set using the default namespace.
Attachment #614649 -
Attachment is obsolete: true
Attachment #614999 -
Flags: review?(ttaubert)
Comment 5•12 years ago
|
||
Ok, so I took a look at this again and the problem lies here: http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.cpp#5205 This reads only "title" attributes without a namespace from XUL elements. So it doesn't read "xul:title". According to http://www.w3.org/TR/REC-xml-names/#defaulting "A default namespace declaration applies to all unprefixed element names within its scope. Default namespace declarations do not apply directly to attribute names; the interpretation of unprefixed attributes is determined by the element on which they appear." That means we don't really need the "xul:" prefix for attributes on the #newtab-window element.
Comment 6•12 years ago
|
||
Comment on attachment 614999 [details] [diff] [review] Changed the default namespace to "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" Review of attachment 614999 [details] [diff] [review]: ----------------------------------------------------------------- After investigating this a bit further I think we should remove the "xul:" prefix from attributes of the #newtab-window element. Additionally, we need to add |title=" "| to the #newtab-scrollbox element or else we'd introduce a tooltip that is shown all the time no matter where the cursor is on the newtab page.
Attachment #614999 -
Flags: review?(ttaubert)
Comment 7•12 years ago
|
||
(In reply to Tim Taubert [:ttaubert] from comment #6) > After investigating this a bit further I think we should remove the "xul:" > prefix from attributes of the #newtab-window element. Additionally, we need > to add |title=" "| to the #newtab-scrollbox element or else we'd introduce a > tooltip that is shown all the time no matter where the cursor is on the > newtab page. What you suggest is obviously a hack that shouldn't be needed, because the root element shouldn't get a tooltip in the first place.
Comment 8•12 years ago
|
||
(In reply to Dão Gottwald [:dao] from comment #7) > What you suggest is obviously a hack that shouldn't be needed, because the > root element shouldn't get a tooltip in the first place. Indeed. Let's fix this in another bug blocking this one. (In reply to Tim Taubert [:ttaubert] from comment #6) > Additionally, we need to add |title=" "| to the #newtab-scrollbox element So, Bellindira, please don't do this. I'll file a bug.
Comment 9•12 years ago
|
||
Hey, Bellindira, can you please attach a patch that removes the "xul:" prefixes from the <xul:window>'s attributes? That's an easy fix we should include in Fx 14. Thanks!
Comment 10•12 years ago
|
||
Removed the xul: for attributes
Attachment #614999 -
Attachment is obsolete: true
Attachment #616497 -
Flags: review?(ttaubert)
Comment 11•12 years ago
|
||
Comment on attachment 616497 [details] [diff] [review] v3 Review of attachment 616497 [details] [diff] [review]: ----------------------------------------------------------------- Thanks!
Attachment #616497 -
Flags: review?(ttaubert) → review+
Comment 12•12 years ago
|
||
Updated•12 years ago
|
Attachment #616497 -
Attachment is obsolete: true
Comment 13•12 years ago
|
||
Comment on attachment 616500 [details] [diff] [review] Patch for checkin [Approval Request Comment] User impact if declined: having newtab loaded and restarting the browser causes the title of those tabs to show as 'about:newtab'. it's a small glitch but also a very small fix. Risk to taking this patch (and alternatives if risky): very small risk and patch String changes made by this patch: none We need bug 745712 approved as well, as a prerequisite for this patch.
Attachment #616500 -
Flags: approval-mozilla-aurora?
Comment 14•12 years ago
|
||
(In reply to Tim Taubert [:ttaubert] from comment #13) > User impact if declined: having newtab loaded and restarting the browser > causes the title of those tabs to show as 'about:newtab'. I wanted to say: if you open some new tabs and restart the browser those will be restored. Instead of having the correct title 'New Tab' they'll have their url shown (about:newtab). Additionally, session restore is now enabled by default which is why I think we should backport this to Aurora.
Comment 15•12 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/fe5bf2fca92e
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 14
Comment 16•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/fe5bf2fca92e
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Comment 17•12 years ago
|
||
Comment on attachment 616500 [details] [diff] [review] Patch for checkin [Triage Comment] Approved for Aurora 13.
Attachment #616500 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 18•12 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/757b166b71fc
status-firefox13:
--- → fixed
Reporter | ||
Comment 19•12 years ago
|
||
Mozilla/5.0 (X11; Linux i686; rv:13.0) Gecko/20120424 Firefox/13.0a2 Mozilla/5.0 (X11; Linux i686; rv:14.0) Gecko/20120424 Firefox/14.0a1 Verified on mac OS 10.6, Ubuntu 11.10 and Windows 7 with the steps in comment 0 on Firefox 13 Aurora and Nightly 14.
You need to log in
before you can comment on or make changes to this bug.
Description
•