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

Status: RESOLVED FIXED

(Thunderbird :: Preferences, defect)

Description

[Andrew Janke]

Attachment: 38.0 Beta - Screen Shot 2015-04-17 at 2.46.32 AM.png

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.

Comment 1

[Andrew Janke]

Attachment: 31.6.0 - showing the panel fitting normally

Comment 2

[Wayne Mery (:wsmwk)]

prefs, or theme?

Comment 3

[:aceman]

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.

Comment 4

[Javi Rueda :javirid]

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.

Comment 5

[Richard Marti (:Paenglab)]

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?

Comment 6

[Wayne Mery (:wsmwk)]

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.

Comment 7

[Eckard Berberich]

Attachment: TB 38 beta FR Advanced pref pane

Comment 8

[Eckard Berberich]

(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;
}

Comment 9

[Andrew Janke]

Attachment: TB 38 beta 6 DSB Advanced prefs - initial.png

Comment 10

[Andrew Janke]

Attachment: TB 38 beta 6 DSB Advanced prefs - after cursing through tabs.png

Comment 11

[Andrew Janke]

(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.

Comment 12

[Richard Marti (:Paenglab)]

Attachment: Bug1155545.patch

This patch moves the "Reading & Display" tab to "Display" > "Advanced" like discussed in IRC.

Comment 13

[Richard Marti (:Paenglab)]

Attachment: Patch for TB 38

The same patch but without string changes for TB 38.

Comment 14

[Richard Marti (:Paenglab)]

Clearing n-i as it was discussed on IRC with Blake.

Comment 15

[Richard Marti (:Paenglab)]

Comment on attachment 8610172 [details] [diff] [review]
Bug1155545.patch

Adding Kent to r? as Blake is unsure he can review fast enough.

Comment 16

[Blake Winton (:bwinton) (:☕️)]

For the record, ui-r=me for the idea.

Comment 17

[:aceman]

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.

Comment 18

[Blake Winton (:bwinton) (:☕️)]

(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.

Comment 19

[:aceman]

(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'.

Comment 20

[:aceman]

(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.

Comment 21

[Richard Marti (:Paenglab)]

Attachment: Bug1155545.patch

Comments fixed with still using Advanced.

Comment 22

[Richard Marti (:Paenglab)]

Comment on attachment 8610175 [details] [diff] [review]
Patch for TB 38

I'll update this patch after the review of the c-c patch.

Comment 23

[:aceman]

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.

Comment 24

[:aceman]

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.

Comment 25

[Richard Marti (:Paenglab)]

Try: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=5d1bd87399a8

Comment 26

[Richard Marti (:Paenglab)]

Attachment: Patch for TB 38

This patch is created on comm-esr38. Removed the comma.

Comment 27

[Richard Marti (:Paenglab)]

(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.

Comment 28

[Richard Marti (:Paenglab)]

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.

Comment 29

[Richard Marti (:Paenglab)]

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.

Comment 30

[Kent James (:rkent)]

Comment on attachment 8610221 [details] [diff] [review]
Bug1155545.patch

Checked-in https://hg.mozilla.org/comm-central/rev/545aab8d1bc7

Comment 31

[Kent James (:rkent)]

Comment on attachment 8610221 [details] [diff] [review]
Bug1155545.patch

http://hg.mozilla.org/releases/comm-aurora/rev/05aab6c8e0e0

Comment 32

[Kent James (:rkent)]

Comment on attachment 8610237 [details] [diff] [review]
Patch for TB 38

[Triage Comment]

https://hg.mozilla.org/releases/comm-beta/rev/6ae174e4b229
https://hg.mozilla.org/releases/comm-esr38/rev/74ddecd15ee5