Closed Bug 965217 Opened 7 years ago Closed 7 years ago

Tab lacks a default title

Categories

(Thunderbird :: Toolbars and Tabs, defect)

Product:
Thunderbird
Component:
Toolbars and Tabs
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 30.0

People

(Reporter: aleth, Assigned: sshagarwal)

Details

Attachments

(4 files, 4 obsolete files)

Screenshot
120.74 KB, image/tiff
Details
Screenshot with Chat tab selected
45.58 KB, image/tiff
Details
Missing tab title in titlebar.
13.32 KB, image/png
Details
Patch for check-in
4.54 KB, patch
sshagarwal
: review+
sshagarwal
: ui-review+
sshagarwal
: feedback+
Details | Diff | Splinter Review
Attached image Screenshot — 
STR
Run Daily witb a fresh profile. After deciding not to create any accounts in the firstrun wizard, the screen is as on the screenshot.
Note the open tab lacks a fallback title.
This persists even after adding a Chat account and restarting. It looks particularly broken when there is a second tab open (see screenshot).
Severity: minor → normal
Summary: Firstrun: Tab lacks a default title → Tab lacks a default title
Attached patch Patch v1 (obsolete) — — Splinter Review 
Possible fix.
Thanks to Paenglab and bwinton for their input suggestions.
Attachment #8370174 - Flags: ui-review?(richard.marti)
Attachment #8370174 - Flags: review?(richard.marti)
Comment on attachment 8370174 [details] [diff] [review]
Patch v1

The code looks good, but the label should be localized (please add a note when this is used). And in titlebar where is only a "- Daily" where it should be "Home - Daily".
Attachment #8370174 - Flags: ui-review?(richard.marti)
Attachment #8370174 - Flags: ui-review-
Attachment #8370174 - Flags: review?(richard.marti)
Attachment #8370174 - Flags: review-
Comment on attachment 8370174 [details] [diff] [review]
Patch v1

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

::: mail/base/content/tabmail.xml
@@ +1193,1 @@
>                tabNode.setAttribute("label", tab.title);

also, indent
I think the titlebar should be only "Thunderbird" - in the normal case it's not "Home", but you're in the account wiz setting up your account.
With a chat account set up it's no more asking for a account. Then the "Home - Thunderbird" would make sense to differentiate from "Chat - Thunderbird".
Comment on attachment 8370174 [details] [diff] [review]
Patch v1

> +              if (aTabNodeOrInfo == null)
> +                tabNode.setAttribute("label", "Home");
> +              else
>                tabNode.setAttribute("label", tab.title);

I think this can be shortened to:

tabNode.setAttribute("label", tab.title? tab.title : "HomeSweetHome");
I didn't receive any mail because I wasn't cc'ed :(
And, I didn't see "Daily" or "Thunderbird" on Windows. Is it hidden by default on Windows systems?

Thanks.
On Windows mail.tabs.drawInTitlebar is on true. To see the title in titlebar you need it to set to false. I've set it to false for a easier screenshot. But you see it also in Task-bar's icon title when you hover over it.
Attached patch Patch v2 (obsolete) — — Splinter Review 
This tries to address most of the comments.
Attachment #8370174 - Attachment is obsolete: true
Attachment #8372264 - Flags: ui-review?(richard.marti)
Attachment #8372264 - Flags: review?(richard.marti)
Attachment #8372264 - Flags: review?(philip.chee)
Attachment #8372264 - Flags: review?(mkmelin+mozilla)
Comment on attachment 8372264 [details] [diff] [review]
Patch v2

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

Possible reasons for the changes in the patch.

And, from all the comments, I couldn't decide as to what was to be done with the titlebar. So, please let me know if what this patch does isn't desired.
Thanks.

::: mail/base/content/tabmail.xml
@@ +1461,5 @@
>          <body>
>            <![CDATA[
>              let docTitle = aTab.title;
> +            // If the document title is blank, add the default title.
> +            if (!docTitle.trim())

Doing this instead of (!docTitle) because the value of docTitle at this point was " " (a single space), not sure why.
Comment on attachment 8372264 [details] [diff] [review]
Patch v2

Looks good! ui-r=me

I'll let make the review through better suited people with more experience.
Attachment #8372264 - Flags: ui-review?(richard.marti)
Attachment #8372264 - Flags: ui-review+
Attachment #8372264 - Flags: review?(richard.marti)
Assignee: nobody → syshagarwal
Status: NEW → ASSIGNED
Comment on attachment 8372264 [details] [diff] [review]
Patch v2

> +              tabNode.setAttribute("label", aTabNodeOrInfo ?
> +                                   tab.title :
> +                                   document.documentElement.getAttribute("defaultTabTitle"));

The following is the preferred warping style for SeaMonkey:

> tabNode.setAttribute("label", aTabNodeOrInfo ?
>                               tab.title :
>                               document.documentElement.getAttribute("defaultTabTitle"));

An Alternate suggestion:

> var tabtitle = aTabNodeOrInfo ? tab.title :
>                                 document.documentElement.getAttribute("defaultTabTitle"));
> tabNode.setAttribute("label", tabtitle);

==========================================================================

>              let docTitle = aTab.title;

Move the trim to here:
let docTitle = aTab.title.trim();

> +            // If the document title is blank, add the default title.
> +            if (!docTitle.trim())
> +              docTitle += document.documentElement.getAttribute("defaultTabTitle");

If the docTitle is empty it's not necessary to append (+=).

if (!docTitle)
  docTitle = document.documentElement.getAttribute("defaultTabTitle");

> +            if (!Application.platformIsMac)
Even though it is not necessary, brace the if block to make it clear where the block begins and ends.

if (!Application.platformIsMac) { ... }

You use document.documentElement several times in this method. I suggest using a helper:

var dElement = document.documentElement;

"dElement" is just an example. Choose a name for the var that conforms to TB house style.
Attachment #8372264 - Flags: review?(philip.chee) → feedback+
> if (!docTitle)
>   docTitle = document.documentElement.getAttribute("defaultTabTitle");
OR
docTitle = docTitle || document.documentElement.getAttribute("defaultTabTitle");
Attached patch Patch v3 (obsolete) — — Splinter Review 
Made the changes. Thanks.
Attachment #8372264 - Attachment is obsolete: true
Attachment #8372264 - Flags: review?(mkmelin+mozilla)
Attachment #8374784 - Flags: ui-review+
Attachment #8374784 - Flags: review?(mkmelin+mozilla)
Attachment #8374784 - Flags: feedback+
Comment on attachment 8374784 [details] [diff] [review]
Patch v3

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

Looks good, thx! r=mkmelin
Attachment #8374784 - Flags: review?(mkmelin+mozilla) → review+
Keywords: checkin-needed
Thanks for fixing this!
https://hg.mozilla.org/comm-central/rev/a11f110bd8bb
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 30.0
Backed out for mozmill failures.
https://hg.mozilla.org/comm-central/rev/67bd7170bc1e

https://tbpl.mozilla.org/php/getParsedLog.php?id=34623078&tree=Thunderbird-Trunk

TypeError: aTab.title is undefined
Target Milestone: Thunderbird 30.0 → ---
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
aTab.title was being used before my patch too. Not sure what made it fail :/ 
Okay, uploading another patch. This time with safety measurements for aTab.
Attached patch Patch v4 (obsolete) — — Splinter Review 
Okay, so the tests pass now. Sorry for the trouble.
Attachment #8374784 - Attachment is obsolete: true
Comment on attachment 8375667 [details] [diff] [review]
Patch v4

This patch got the review, but I made a mistake and the tests failed. Can you please confirm that it works and the tests pass?

Thanks.
Attachment #8375667 - Flags: review?(mkmelin+mozilla)
Attachment #8375667 - Flags: feedback?(acelists)
Comment on attachment 8375667 [details] [diff] [review]
Patch v4

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

(Haven't run the tests )

::: mail/base/content/tabmail.xml
@@ +1461,5 @@
>          <body>
>            <![CDATA[
> +            let docTitle;
> +            if (aTab && aTab.title)
> +              docTitle = aTab.title.trim();

you don't have to check aTab (that wasn't checked earlier either)

let docTitle = aTab.title ? aTab.title.trim() : "";
Attachment #8375667 - Flags: review?(mkmelin+mozilla) → review+
Comment on attachment 8375667 [details] [diff] [review]
Patch v4

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

I run mozmill locally and I do not see the previous failures with this patch.
Attachment #8375667 - Flags: feedback?(acelists) → feedback+
Thanks.
Attachment #8375667 - Attachment is obsolete: true
Attachment #8375758 - Flags: ui-review+
Attachment #8375758 - Flags: review+
Attachment #8375758 - Flags: feedback+
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/5b15742aa87f
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 30.0
You need to log in before you can comment on or make changes to this bug.