In the preference panel related to mailnews message visualisation, these two options: "Wrap text to fit window width" and "Display emoticons as graphics" are on the same line. This may be good for English since the two strings are short, but in Italian the result is that the string of the second option is truncated and there's a need to enlarge the windows to read it all (see attached screenshot). I've updated the localisation to try to use a shorter string for both the first and second prefs (we'll see it it's enough), but since there are no particular vertical restrains, I believe that the best thing to do is to move the second option in a new line below the first one, so that there's plenty of horizontal space for both texts.

This should be a fairly simple fix around http://mxr.mozilla.org/comm-central/source/suite/mailnews/prefs/pref-viewing_messages.xul#145

This is straight-forward indeed, especially since you've pointed me already to the correct line. :-) Just removing the <hbox> will do the trick already. I'm wondering too why they were put on the same line to start with, given that these are two independent settings (thus, it's more logical to have them on two lines regardless of the horizontal space problem). There isn't much vertical space left now at the bottom of that pane, but it still fits with a bit to spare.

Hmm, HG Blame points to bug 608103 where issues were reported on Windows 7. While I haven't seen this initially using my default settings (which include the Windows Classic desktop and not using hardware acceleration), and it still looked fine using the Windows 7 default desktop theme alone (1st image), I could reproduce it with left hardware acceleration re-enabled (2nd image). I've added Jens and Edmund as they were involved in the previous solution and may have some ideas. I could remove a couple of separators if that's the case, but that would make the pane look less structured given that they separate distinct blocks. It solves the problem almost, still leaving the lower border of the groupbox a bit hidden, but at least all UI elements remain accessible (3rd image). The current patch looks fine on Linux with KDE4 desktop.

Since the first version evidently isn't going to work under certain conditions, here the patch with the two thin separators removed as well.

Locales can always tweak the pref window size on each platform e.g. http://mxr.mozilla.org/comm-central/source/suite/locales/en-US/chrome/common/pref/win/platformPrefOverlay.dtd#12

Comment on attachment 744898 [details] [diff] [review] Proposed fix (v2) Seems reasonable to me. If you're really tight on space, another option might be to switch the [ ] Automatically mark messages as read ( ) Immediately on display ( ) After displaying for [ ] seconds radiogroup to a checkbox, something like this: [ ] Automatically mark messages as read [ ] Only after displaying for [ ] seconds

The solution in attachment 744898 [details] [diff] [review] misses 8px as measured in the screenshots for the groupbox to appear properly on the machine tested (where I can't tell how representative that is). For the first patch, about 20px more are needed. I wouldn't like to increase prefWindow.size for all preference panes on Windows, if that's what Ian suggests in comment #5. That file hasn't been touched since that entity was established in 2006, thus I'm rather reluctant to change that now. Putting the "mark messages as read" options onto a single line sounds like a good idea as well and would retain the current space requirement of the pane. To keep the current logic, just reducing the label for the first option might do the trick: [x] Automatically mark messages as read: (*) Immediately ( ) After displaying for [__] seconds This would avoid the need for a checkbox-to-radio mapping, but might move the string-length issue just to a different option for locales that need longer strings.

(In reply to rsx11m from comment #7) > This would avoid the need for a checkbox-to-radio mapping, but might move > the string-length issue just to a different option for locales that need > longer strings. Exactly. The point here is to let everything fit horizontally. Are you sure we can't just increase a bit the prefs window? After all, since 2006, screens have become bigger and with higher resolution, I don't think that 8 px (or even 20 px for that matter) will actually be an issue.

Well, desktop screen sizes indeed have increased substantially, more in width than in height though. Also keep in mind that there are more small devices around these days, thus height limits are still an important consideration (the current height is 530px on my system using the Windows 7 default desktop theme, which may have been based on an 800x600 minimum resolution at that time; that's also an accessibility issue for users with impaired eye sight using this resolution). (In reply to neil@parkwaycc.co.uk from comment #6) > radiogroup to a checkbox, something like this: > [ ] Automatically mark messages as read > [ ] Only after displaying for [ ] seconds I intend to give Neil's suggestion a try, the logic for this shouldn't be overly complex. It would simplify the pane which looks a bit crowded anyway, frees up an accesskey and gains the line that's added in the plain-text group. If that doesn't work, we can still go with the squeezed patch removing the separators and/or increase the 41em height to 42em.

This implements comment #6. It was much easier than expected, given that the preference set by the radiogroup is boolean already rather than an integer, thus no mapping of the checkbox is needed and it can be directly wired. Some other fixes included in this patch: - The "seconds" label now has an accesskey for the textbox control, thus all accesskeys are recycled; I left the focus on clicking the checkbox in but that could be removed to match the behavior of similar constructs. - "secondsLabel" observes the main checkbox rather than the textbox, thus it stays ungrayed when the box is just unchecked (matching similar constructs). - I've removed a redundant <vbox> around these settings and a redundant <hbox> around the upper checkbox (which was the only element); I'm starting to wonder if those may have served some purpose, be it for code grouping of related elements for better reading of the code, or some add-on access? They don't have IDs associated, but let me know if you want them back.

By the way: I've opened bug 868419 for updating the Help content for this preference pane, given that it looks quite outdated. I'll work on that once everything is settled here.

Showing attachment 745184 [details] [diff] [review]. Same conditions as before, Windows 7 default desktop theme with hardware acceleration enabled.

(In reply to Iacopo Benesperi [:iacchi] from comment #8) > Exactly. The point here is to let everything fit horizontally. Are you sure > we can't just increase a bit the prefs window? After all, since 2006, > screens have become bigger and with higher resolution, I don't think that 8 > px (or even 20 px for that matter) will actually be an issue. I've found more pref panes with insufficient-height issues when hardware acceleration is enabled and filed bug 868495 to investigate this. Increasing the height from 41em to 43em would fit all current pref panes, including the (v2) patch still up for review here (not sure about the first version). However, I still think that the alternative patch to save some space and UI complexity in general should be the better way to go, thus I'd prefer that solution.

(In reply to rsx11m from comment #10) > - The "seconds" label now has an accesskey for the textbox control, thus all > accesskeys are recycled; I left the focus on clicking the checkbox in but > that could be removed to match the behavior of similar constructs. I think all the similar constructs move the focus when they enable the textbox (one in the Cookies panel uses a radio while the others are all checkboxes). > - "secondsLabel" observes the main checkbox rather than the textbox, thus it > stays ungrayed when the box is just unchecked (matching similar constructs). Interestingly the accesskey still triggers the textbox even when the checkbox is locked true. I couldn't find any similar constructs, they always keep the label ungrayed. (Worth a GFB?) > - I've removed a redundant <vbox> around these settings and a redundant <hbox> > around the upper checkbox (which was the only element); I'm starting to > wonder if those may have served some purpose, be it for code grouping of > related elements for better reading of the code, or some add-on access? > They don't have IDs associated, but let me know if you want them back. Some of those preference panels are quite old. Unfortunately the disk with my CVS checkout on it failed, but MXR shows that there are several redundant boxes in the version included with SeaMonkey 1.

Comment on attachment 745184 [details] [diff] [review] Alternative patch >+++ b/suite/locales/en-US/chrome/mailnews/pref/pref-viewing_messages.dtd >@@ -18,28 +18,27 @@ > <!ENTITY displayPlainText.caption "Plain text messages"> > <!ENTITY fontPlainText.label "Font:"> > <!ENTITY fontPlainText.accesskey "F"> > <!ENTITY displayQuoted.label "Settings for quoted messages:"> > <!ENTITY wrapInMsg.label "Wrap text to fit window width"> > <!ENTITY wrapInMsg.accesskey "W"> > <!-- LOCALIZATION NOTE : (convertEmoticons.label) 'Emoticons' are also known as 'Smileys', e.g. :-) --> > <!ENTITY convertEmoticons.label "Display emoticons as graphics"> >-<!ENTITY convertEmoticons.accesskey "D"> >+<!ENTITY convertEmoticons.accesskey2 "m"> You don't need to make this change, see below. > <!ENTITY generalMessageDisplay.caption "General"> > <!ENTITY autoMarkAsRead.label "Automatically mark messages as read"> > <!ENTITY autoMarkAsRead.accesskey "A"> >+<!-- LOCALIZATION NOTE (markAsReadAfter.label): This will concatenate to >+ "Only after displaying for [___] seconds", >+ using (markAsReadAfter.label) and a number (secondsLabel.label). --> >+<!ENTITY markAsReadAfter.label "Only after displaying for"> >+<!ENTITY markAsReadAfter.accesskey "t"> > <!ENTITY secondsLabel.label "seconds"> >+<!ENTITY secondsLabel.accesskey "d"> I don't believe this is required as the text box gets focus as soon as you check the preference, so no need to make the change to the accesskey for convertEmoticons. >+++ b/suite/mailnews/prefs/pref-viewing_messages.js >@@ -7,20 +7,20 @@ > // <prefpane id="viewing_messages_pane">! > > function Startup() > { > var autoPref = document.getElementById("mailnews.mark_message_read.auto"); > UpdateMarkAsReadOptions(autoPref.value); > } > >-function UpdateMarkAsReadOptions(aEnableRadioGroup) >+function UpdateMarkAsReadOptions(aEnableReadDelay) > { >- EnableElementById("markAsReadAutoPreferences", aEnableRadioGroup, false); >+ EnableElementById("markAsReadAfterPreferences", aEnableReadDelay, false); > // ... and the extras! > var delayPref = document.getElementById("mailnews.mark_message_read.delay"); >- UpdateMarkAsReadTextbox(aEnableRadioGroup && delayPref.value, false); >+ UpdateMarkAsReadTextbox(aEnableReadDelay && delayPref.value, false); > } > > function UpdateMarkAsReadTextbox(aEnable, aFocus) > { > EnableElementById("markAsReadDelay", aEnable, aFocus); > } >+++ b/suite/mailnews/prefs/pref-viewing_messages.xul >- <vbox> >- <hbox> >- <checkbox id="automaticallyMarkAsRead" >- preference="mailnews.mark_message_read.auto" >- label="&autoMarkAsRead.label;" >- accesskey="&autoMarkAsRead.accesskey;" >- oncommand="UpdateMarkAsReadOptions(this.checked)"/> >- </hbox> >+ <checkbox id="automaticallyMarkAsRead" >+ preference="mailnews.mark_message_read.auto" >+ label="&autoMarkAsRead.label;" >+ accesskey="&autoMarkAsRead.accesskey;" >+ oncommand="UpdateMarkAsReadOptions(this.checked)"/> Nit: missing ; after ) Will the help changes be in another bug/patch? r=me with those addressed/answered.

Comment on attachment 744898 [details] [diff] [review] Proposed fix (v2) Cancelling this review request as the other patch is my preferred route.

(In reply to neil@parkwaycc.co.uk from comment #14) > (In reply to rsx11m from comment #10) > > - The "seconds" label now has an accesskey for the textbox control, thus all > > accesskeys are recycled; I left the focus on clicking the checkbox in but > > that could be removed to match the behavior of similar constructs. > I think all the similar constructs move the focus when they enable the > textbox (one in the Cookies panel uses a radio while the others are all > checkboxes). (In reply to Ian Neal from comment #15) > > <!ENTITY secondsLabel.label "seconds"> > >+<!ENTITY secondsLabel.accesskey "d"> > I don't believe this is required as the text box gets focus as soon as you > check the preference, so no need to make the change to the accesskey for > convertEmoticons. You are correct, I thought that this was only the practice for radiogroups, but I see that with checkboxes as well. I've removed the "seconds" accesskey and reinstated "D" for "Display emoticons as graphics". (In reply to neil@parkwaycc.co.uk from comment #14) > Interestingly the accesskey still triggers the textbox even when the > checkbox is locked true. I couldn't find any similar constructs, they always > keep the label ungrayed. (Worth a GFB?) I'm relying on EnableElementById() doing the right thing, so that would be indeed a more general bug if there is a glitch in its implementation. > Some of those preference panels are quite old. Unfortunately the disk with > my CVS checkout on it failed, but MXR shows that there are several redundant > boxes in the version included with SeaMonkey 1. Ok, I was concerned that they were put there for a reason I've missed, but apparently this is not the case. (In reply to Ian Neal from comment #15) > >+ oncommand="UpdateMarkAsReadOptions(this.checked)"/> > Nit: missing ; after ) Taken care of. > Will the help changes be in another bug/patch? Yes, see comment #11. It's outdated enough that it needs more work than the scope of this bug, thus I'll work out a separate patch for Help in bug 868419 soon. Carrying forward ui-r=Neil, r=IanN from attachment 745184 [details] [diff] [review].

Push for comm-central, please.

Just out of curiosity, is there a reason why this change can't be applied to beta and aurora, too? At least Aurora.

(In reply to Iacopo Benesperi [:iacchi] from comment #20) > Just out of curiosity, is there a reason why this change can't be applied to > beta and aurora, too? At least Aurora. As there are changes to locale files, we tend not to put late l10n changes into beta though occasionally do into aurora.

Ok, sorry, I thought the patch was just a reordering of existing strings and not a modification of them. That's why I made the question.

It reverts the change made by bug 608103 (which indeed just rearranged the XUL elements) but also modifies the markAsReadDelay string. That's the main reason for the patch not qualifying for aurora and up. Removal of markAsReadNoDelay alone may have disqualified it already, I don't know, but sounds likely.

(In reply to neil@parkwaycc.co.uk from comment #14) > Interestingly the accesskey still triggers the textbox even when the > checkbox is locked true. I couldn't find any similar constructs, they always > keep the label ungrayed. (Worth a GFB?) Neil, I'm not sure if I understand exactly what you mean. Can you go ahead and file a new bug with steps to reproduce as I don't know what the expected/observed behavior is supposed to be? I'd expect that even if the pref associated with the checkbox or radiogroup is locked, the textbox for the value should still be accessible (unless that pref is locked too) and can even be focused with the accesskey of the locked preference, thus I wouldn't necessarily call this a bug.

(In reply to rsx11m from comment #24) > (In reply to neil@parkwaycc.co.uk from comment #14) > > Interestingly the accesskey still triggers the textbox even when the > > checkbox is locked true. I couldn't find any similar constructs, they always > > keep the label ungrayed. (Worth a GFB?) > > Neil, I'm not sure if I understand exactly what you mean. Can you go ahead > and file a new bug with steps to reproduce as I don't know what the > expected/observed behavior is supposed to be? > > I'd expect that even if the pref associated with the checkbox or radiogroup > is locked, the textbox for the value should still be accessible (unless that > pref is locked too) and can even be focused with the accesskey of the locked > preference, thus I wouldn't necessarily call this a bug. I think he is saying, that potentially there are other places in the prefs that could also have this label being grayed out.

Oh, so this is about the <observes> construct added to the textbox <label> here by bug 447696 to accomplish graying of "seconds" along with the checkbox label, and porting it to other pref panes where this wasn't done?

(In reply to rsx11m from comment #26) > Oh, so this is about the <observes> construct added to the textbox <label> > here by bug 447696 to accomplish graying of "seconds" along with the > checkbox label, and porting it to other pref panes where this wasn't done? I believe so, hence the GFB (Good First Bug) reference.

Ok, I'll have a look for some instances which keep those secondary/dependent labels ungrayed on locking of the main preference.

Bug 869116 submitted.