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

RESOLVED FIXED in Thunderbird 41.0

Status

Thunderbird
Preferences
--
minor
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Andrew Janke, Assigned: Paenglab)

Tracking

({regression})

38 Branch
Thunderbird 41.0
x86
Mac OS X
regression

Thunderbird Tracking Flags

(thunderbird38+ fixed, thunderbird39 fixed, thunderbird40 fixed)

Details

Attachments

(7 attachments, 2 obsolete attachments)

(Reporter)

Description

2 years ago
Created attachment 8593786 [details]
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.
(Reporter)

Comment 1

2 years ago
Created attachment 8593787 [details]
31.6.0 -  showing the panel fitting normally
prefs, or theme?
Severity: normal → minor
Component: Untriaged → Preferences

Comment 3

2 years ago
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

2 years ago
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
(Assignee)

Comment 5

2 years ago
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.

Comment 7

2 years ago
Created attachment 8609822 [details]
TB 38 beta FR Advanced pref pane

Comment 8

2 years ago
(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: 1132405
(Reporter)

Comment 9

2 years ago
Created attachment 8609894 [details]
TB 38 beta 6 DSB Advanced prefs - initial.png
(Reporter)

Comment 10

2 years ago
Created attachment 8609895 [details]
TB 38 beta 6 DSB Advanced prefs - after cursing through tabs.png
(Reporter)

Comment 11

2 years ago
(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.
(Assignee)

Comment 12

2 years ago
Created attachment 8610172 [details] [diff] [review]
Bug1155545.patch

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)
(Assignee)

Comment 13

2 years ago
Created attachment 8610175 [details] [diff] [review]
Patch for TB 38

The same patch but without string changes for TB 38.
Attachment #8610175 - Flags: review?(bwinton)
(Assignee)

Comment 14

2 years ago
Clearing n-i as it was discussed on IRC with Blake.
Flags: needinfo?(josiah)
(Assignee)

Comment 15

2 years ago
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)
(Assignee)

Updated

2 years ago
Attachment #8610175 - Flags: review?(rkent)
For the record, ui-r=me for the idea.

Comment 17

2 years ago
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.

Comment 19

2 years ago
(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

2 years ago
(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.
(Assignee)

Comment 21

2 years ago
Created attachment 8610221 [details] [diff] [review]
Bug1155545.patch

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)
(Assignee)

Comment 22

2 years ago
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 23

2 years ago
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 24

2 years ago
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+
(Assignee)

Comment 25

2 years ago
Try: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=5d1bd87399a8
(Assignee)

Comment 26

2 years ago
Created attachment 8610237 [details] [diff] [review]
Patch for TB 38

This patch is created on comm-esr38. Removed the comma.
Attachment #8610237 - Flags: review+

Updated

2 years ago
status-thunderbird38: --- → affected
tracking-thunderbird38: --- → +
(Assignee)

Comment 27

2 years ago
(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
(Assignee)

Comment 28

2 years ago
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?
(Assignee)

Comment 29

2 years ago
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?

Comment 30

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

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

Updated

2 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 41.0

Updated

2 years ago
Keywords: checkin-needed

Comment 31

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

http://hg.mozilla.org/releases/comm-aurora/rev/05aab6c8e0e0
Attachment #8610221 - Flags: approval-comm-aurora? → approval-comm-aurora+

Comment 32

2 years ago
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
Attachment #8610237 - Flags: approval-comm-esr38+
Attachment #8610237 - Flags: approval-comm-beta?
Attachment #8610237 - Flags: approval-comm-beta+

Updated

2 years ago
status-thunderbird38: affected → fixed
status-thunderbird39: --- → fixed
status-thunderbird40: --- → fixed
You need to log in before you can comment on or make changes to this bug.