Advanced Preferences doesn't fit in the window on Retina displays on OS X 10.9

RESOLVED FIXED in Thunderbird 41.0

Status

defect
--
minor
RESOLVED FIXED
4 years ago
Last year

People

(Reporter: floss, Assigned: Paenglab)

Tracking

({regression})

38 Branch
Thunderbird 41.0
x86
macOS
Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird38+ fixed, thunderbird39 fixed, thunderbird40 fixed)

Details

Attachments

(7 attachments, 2 obsolete attachments)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:37.0) Gecko/20100101 Firefox/37.0
Build ID: 20150402191859

Steps to reproduce:

Run Thunderbird, press Command-, to bring up the Preferences window, and click the Advanced button at the top of the preferences window.

This is on OS X 10.9.5. affects both 38.0 beta and 39.0a2 Earlybird. I'm on a Retina MacBook Pro, running at native ("Best for display") resolution.


Actual results:

The pane displaying the Advanced preferences appears not to fit in its containing widget. The radio button bar at the top ending in "Certificates" is truncated and you cannot see the right hand border of that panel. 

It appears that the addition of the "Data Choices" tabl has made the tab button bar (or whatever you call it) too wide for the window's defined size.


Expected results:

Full panel should be displayed inside the preferences window, as it is in 31.6.0.
prefs, or theme?
Severity: normal → minor
Component: Untriaged → Preferences
Many tabs with long headers. But on Win XP it is no problem. The whole window can't be resized but maybe it has a larger width on Windows. The style on the prefwindow element is set as "width: 48em; min-height: 38.5em;". The window is 656px wide. In the screenshot the image is much wider but also the font is immensely larger. The style of the window probably needs some tuning in the theme for OS X.
Possible cause is (fixed by me) bug 986078. I don't own a Retina Display, so I was unable to test on one of them.

I stopped knowing which Thunderbird released version matches every "development" version long ago.
Keywords: regression
This affects also the in-content prefs. OS X is using a wider font than Linux or Windows. The size is the same.

I propose either to shorten the tab titles. "Reading & Display" could be shortened to only "Reading" when we move the only display entry to the "Display" pane. "Network & Disk Space" could be shortened to only "Network" like FX is doing.

Or we could move the whole "Reading & Display" tab to the "Display" pane, as I think, it belongs more to "Display" like the name in tab says.

Josiah, what do you think?
Flags: needinfo?(josiah)
Is Changing font a serious option?  It does seem to me to be quite large.

Our documentation and support often explicit when referring to these titles, so I'd rather not see them changed unless it is accompanied by some general reorganization of the settings.
(In reply to Eckard Berberich from comment #7)
> Created attachment 8609822 [details]

It's even worse in the French version of v38 beta since some tabs have rather long names in French.
My screen shot has been taken on a iMac 20" display, running OS X 10.9.5.
The most worrying is the "Advanced" pref pane in which the "Data Choices" tab has been added in v38 (see attachment 8609822 [details]). 

IMHO the problem in the "classic" pref pane could be easily fixed by increasing its width:
I went to Thunderbird.app/contents/MacOS/omni.ja/chrome/fr/locale/fr/messenger/preferences/preferences.dtd which I edited.
In the line 
<!ENTITY  prefWindow.styleMac     "width: 55em;">
I replaced 55em with 64em to restore harmonious, well proportioned pref panes, especially the Advanced pane.

Until a patch will be written, a workaround could be to use the following css-code in Stylish or the userChrome.css file:

#MailPreferences {
width: 64em !important;
}
Blocks: TB38found
(In reply to janke from comment #10)
> Created attachment 8609895 [details]
> TB 38 beta 6 DSB Advanced prefs - after cursing through tabs.png

Looks like some other European languages are affected even harder. German is like French. Lower Sorbian (DSB) is lengthy enough that the "Certificates" tab is lost off the edge of the viewport entirely. You can still get to it if you know it's there by clicking one of the other tabs and then using the arrow keys to switch to it. But this causes the entire view to shift within the viewport, losing the left side of the pane off the edge of the visible area. That has important text and buttons, so the pane looks like it's unusable at that point.

Would there be a way to change the layout definition so that if the aggregate tab label length is too great, individual tab labels get truncated, leaving the tab bar and contained panel constrained to a fixed maximum size? It would be ugly, but that seems like it would be a more graceful decay than letting the contained panel expand past the viewport size and making some widgets inaccessible.
Posted patch Bug1155545.patch (obsolete) — Splinter Review
This patch moves the "Reading & Display" tab to "Display" > "Advanced" like discussed in IRC.
Assignee: nobody → richard.marti
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #8610172 - Flags: review?(bwinton)
Posted patch Patch for TB 38 (obsolete) — Splinter Review
The same patch but without string changes for TB 38.
Attachment #8610175 - Flags: review?(bwinton)
Clearing n-i as it was discussed on IRC with Blake.
Flags: needinfo?(josiah)
Comment on attachment 8610172 [details] [diff] [review]
Bug1155545.patch

Adding Kent to r? as Blake is unsure he can review fast enough.
Attachment #8610172 - Flags: review?(rkent)
Attachment #8610175 - Flags: review?(rkent)
For the record, ui-r=me for the idea.
Comment on attachment 8610172 [details] [diff] [review]
Bug1155545.patch

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

I like the change and the code works for me. Please fix the small nits.

::: mail/components/preferences/display.js
@@ +314,5 @@
> +    delayTextbox.disabled = !globalCheckbox.checked ||
> +                            !delayRadioOption.selected || intervalPref.locked;
> +    if (!delayTextbox.disabled && aFocusTextBox)
> +      delayTextbox.focus();
> +  },

Doesn't need the trailing comma.

::: mail/components/preferences/display.xul
@@ +41,5 @@
>        <!-- FONTS -->
>        <preference id="font.language.group" name="font.language.group"
>                    type="wstring" onchange="gDisplayPane._rebuildFonts();"/>
> +
> +      <!-- Advanced -->

Can we actually call the tab "Reading" to keep the original meaning. Who decided on the "Advanced" label?

@@ +59,5 @@
>      <tabbox id="displayPrefs" flex="1" onselect="gDisplayPane.tabSelectionChanged();">
>        <tabs id="displayPrefsTabs">
>          <tab id="formattingTab" label="&itemFormatting.label;"/>
>          <tab id="tagTab" label="&itemTags.label;"/>
> +        <tab id="displayTab" label="&itemDisplay.label;"/>

Make the id the same as the label, so maybe readingTab.

@@ +188,5 @@
>              </vbox>
>            </hbox>
>          </tabpanel>
> +
> +        <!-- Advanced -->

Reading?

::: mail/locales/en-US/chrome/messenger/preferences/display.dtd
@@ +3,5 @@
>     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
>  
>  <!ENTITY itemFormatting.label             "Formatting">
>  <!ENTITY itemTags.label                   "Tags">
> +<!ENTITY itemDisplay.label                "Advanced">

You are changing the string so better change the entity name too. Also coders will probably wonder why Display is called Advanced (or Reading). I suggest itemReading.label to align with the label.

@@ +45,5 @@
>  <!ENTITY fontOptions.label       "Advanced…">
>  <!ENTITY colorButton.label       "Colors…">
>  <!ENTITY colorButton.accesskey   "C">
> +
> +<!-- Display and Reading Settings -->

The section is called Advanced (or Reading) now.

@@ +49,5 @@
> +<!-- Display and Reading Settings -->
> +<!ENTITY reading.caption               "Reading">
> +<!ENTITY display.caption               "Display">
> +<!ENTITY showCondensedAddresses.label  "Show only display name for people in my address book">
> +<!ENTITY showCondensedAddresses.accesskey "S">

It seems this it our chance to align all these new strings. Align them all under "Size:" in entity size.label . Only the moved ones from here to the end of the file.
Attachment #8610172 - Flags: feedback+
(In reply to :aceman from comment #17)
> ::: mail/components/preferences/display.xul
> @@ +41,5 @@
> >        <!-- FONTS -->
> >        <preference id="font.language.group" name="font.language.group"
> >                    type="wstring" onchange="gDisplayPane._rebuildFonts();"/>
> > +
> > +      <!-- Advanced -->
> Can we actually call the tab "Reading" to keep the original meaning. Who
> decided on the "Advanced" label?

Well, since the "Show only display name for people in my address book" is in there (so it's not just reading preferences), and since it came from the Advanced tab, I would claim that "Advanced" is closer to the original meaning.  ("Advanced >> Display" turns into "Display >> Advanced".)
Also, it was me who decided on the "Advanced" label.  ;)

Later,
Blake.
(In reply to Blake Winton (:bwinton) from comment #18)
> Well, since the "Show only display name for people in my address book" is in
> there (so it's not just reading preferences), and since it came from the
> Advanced tab, I would claim that "Advanced" is closer to the original
> meaning.  ("Advanced >> Display" turns into "Display >> Advanced".)
Is it ["Advanced >> Reading & Display" turns into "Display >> Advanced"].
To me it also makes sense to do ["Advanced >> Reading & Display" turns into "Display >> Reading"] when most (even if not all) of the content is about "Reading".

> Also, it was me who decided on the "Advanced" label.  ;)
OK, no problem. Then please change all IDs and references to 'Advanced' instead of 'Display'.
(In reply to :aceman from comment #17)
> @@ +59,5 @@
> >      <tabbox id="displayPrefs" flex="1" onselect="gDisplayPane.tabSelectionChanged();">
> >        <tabs id="displayPrefsTabs">
> >          <tab id="formattingTab" label="&itemFormatting.label;"/>
> >          <tab id="tagTab" label="&itemTags.label;"/>
> > +        <tab id="displayTab" label="&itemDisplay.label;"/>
> 
> Make the id the same as the label, so maybe readingTab.

Well, probably keep this ID at displayTab to keep compatibility with addons. The contents have not changed regardless of the place the tab is in. We can change the ID when tab contents changes in a future bug.
Comments fixed with still using Advanced.
Attachment #8610172 - Attachment is obsolete: true
Attachment #8610172 - Flags: review?(rkent)
Attachment #8610172 - Flags: review?(bwinton)
Attachment #8610221 - Flags: review?(acelists)
Comment on attachment 8610175 [details] [diff] [review]
Patch for TB 38

I'll update this patch after the review of the c-c patch.
Attachment #8610175 - Attachment is obsolete: true
Attachment #8610175 - Flags: review?(rkent)
Attachment #8610175 - Flags: review?(bwinton)
Comment on attachment 8610221 [details] [diff] [review]
Bug1155545.patch

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

I like it now, thanks.

I give r+ because the patch is mostly only moving code around, so maybe my review will be enough.

Please run it through mozmill tests on try server to see if we have don't have any tests touching the prefs in the old location.
Attachment #8610221 - Flags: review?(acelists) → review+
Comment on attachment 8610175 [details] [diff] [review]
Patch for TB 38

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

I think this version for TB38 is fine, just fix the comma.
You do not change any IDs or string entity names. The comments are changed to <!-- Advanced --> which is fine.

I do not have a TB38 tree so still let rkent check if it applies correctly there.

::: mail/components/preferences/display.js
@@ +314,5 @@
> +    delayTextbox.disabled = !globalCheckbox.checked ||
> +                            !delayRadioOption.selected || intervalPref.locked;
> +    if (!delayTextbox.disabled && aFocusTextBox)
> +      delayTextbox.focus();
> +  },

Trailing comma.
Attachment #8610175 - Flags: feedback+
This patch is created on comm-esr38. Removed the comma.
Attachment #8610237 - Flags: review+
(In reply to Richard Marti (:Paenglab) from comment #25)
> Try:
> https://treeherder.mozilla.org/#/jobs?repo=try-comm-
> central&revision=5d1bd87399a8

I see no additional failures -> checkin-needed.
Keywords: checkin-needed
Comment on attachment 8610221 [details] [diff] [review]
Bug1155545.patch

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: Issues accessing all elements in prefs
Testing completed (on c-c, etc.): c-n set for c-c
Risk to taking this patch (and alternatives if risky): Low, only code move.
Attachment #8610221 - Flags: approval-comm-aurora?
Comment on attachment 8610237 [details] [diff] [review]
Patch for TB 38

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: Issues accessing all elements in prefs
Testing completed (on c-c, etc.): c-n set for c-c
Risk to taking this patch (and alternatives if risky): Low, only code move. No string changes.

There is no approval-comm-esr38 but it should also land there.
Attachment #8610237 - Flags: approval-comm-beta?
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 41.0
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.