Title page for New tab is about:newtab after restart

VERIFIED FIXED in Firefox 13

Status

()

Firefox
General
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: virgil, Assigned: bellindira)

Tracking

13 Branch
Firefox 14
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox13 verified, firefox14 verified)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

5 years ago
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
Assignee: nobody → bellindira
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 1

5 years ago
Created attachment 614649 [details] [diff] [review]
Patch

Applied the same logic used for about:blank to set title page.
Attachment #614649 - Flags: review?(ttaubert)
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

5 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

5 years ago
Created attachment 614999 [details] [diff] [review]
Changed the default namespace to "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"

The title is set using the default namespace.
Attachment #614649 - Attachment is obsolete: true
Attachment #614999 - Flags: review?(ttaubert)
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 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)
(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.
(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.
Depends on: 745712
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!
Created attachment 616497 [details] [diff] [review]
v3

Removed the xul: for attributes
Attachment #614999 - Attachment is obsolete: true
Attachment #616497 - Flags: review?(ttaubert)
Comment on attachment 616497 [details] [diff] [review]
v3

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

Thanks!
Attachment #616497 - Flags: review?(ttaubert) → review+
Created attachment 616500 [details] [diff] [review]
Patch for checkin
Attachment #616497 - Attachment is obsolete: true
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?
(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.
https://hg.mozilla.org/integration/fx-team/rev/fe5bf2fca92e
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 14
https://hg.mozilla.org/mozilla-central/rev/fe5bf2fca92e
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
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+
https://hg.mozilla.org/releases/mozilla-aurora/rev/757b166b71fc
status-firefox13: --- → fixed
(Reporter)

Comment 19

5 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.
Status: RESOLVED → VERIFIED
status-firefox13: fixed → verified
status-firefox14: --- → verified
You need to log in before you can comment on or make changes to this bug.