736476 - Title page for New tab is about:newtab after restart

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Virgil Dicu [:virgil] [QA]]

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

Comment 1

[Bellindira Castillo [:bellindira]]

Attachment: Patch

Applied the same logic used for about:blank to set title page.

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 3

[Bellindira Castillo [:bellindira]]

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.

Comment 4

[Bellindira Castillo [:bellindira]]

Attachment: Changed the default namespace to "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"

The title is set using the default namespace.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

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

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 7

[Dão Gottwald [:dao]]

(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

[Tim Taubert [:ttaubert] (inactive)]

(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

[Tim Taubert [:ttaubert] (inactive)]

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

[Raymond Lee [:raymondlee]]

Attachment: v3

Removed the xul: for attributes

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 616497 [details] [diff] [review]
v3

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

Thanks!

Comment 12

[Raymond Lee [:raymondlee]]

Attachment: Patch for checkin

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

(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

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/fe5bf2fca92e

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/fe5bf2fca92e

Comment 17

[Alex Keybl [:akeybl]]

Comment on attachment 616500 [details] [diff] [review]
Patch for checkin

[Triage Comment]
Approved for Aurora 13.

Comment 18

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/757b166b71fc

Comment 19

[Virgil Dicu [:virgil] [QA]]

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.