Closed
Bug 474807
Opened 15 years ago
Closed 15 years ago
Margin and Padding CSS rules should be independent of UI direction (LTR or RTL)
Categories
(Thunderbird :: Mail Window Front End, defect)
Thunderbird
Mail Window Front End
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 3.0b2
People
(Reporter: sipaq, Assigned: sipaq)
References
Details
(Keywords: rtl)
Attachments
(4 files, 1 obsolete file)
231.46 KB,
patch
|
Details | Diff | Splinter Review | |
7.98 KB,
patch
|
berend.cornelius09
:
review+
|
Details | Diff | Splinter Review |
88.77 KB,
patch
|
philor
:
review+
|
Details | Diff | Splinter Review |
134.60 KB,
patch
|
neil
:
review+
neil
:
superreview+
|
Details | Diff | Splinter Review |
We should use direction-independent CSS rules throughout the UI. I will cover Calendar and SeaMonkey with my patches as well. Here are links to many occurrences of this bug in our codebase: Use of padding-left (should be replaced with -moz-padding-start): - http://mxr.mozilla.org/comm-central/search?string=padding-left&find=mail&tree=comm-central - http://mxr.mozilla.org/comm-central/search?string=padding-left&find=calendar&tree=comm-central - http://mxr.mozilla.org/comm-central/search?string=padding-left&find=suite&tree=comm-central Use of padding-right (should be replaced with -moz-padding-end): - http://mxr.mozilla.org/comm-central/search?string=padding-right&find=mail&tree=comm-central - http://mxr.mozilla.org/comm-central/search?string=padding-right&find=calendar&tree=comm-central - http://mxr.mozilla.org/comm-central/search?string=padding-right&find=suite&tree=comm-central Use of margin-left (should be replaced with -moz-margin-start): - http://mxr.mozilla.org/comm-central/search?string=margin-left&find=mail&tree=comm-central - http://mxr.mozilla.org/comm-central/search?string=margin-left&find=calendar&tree=comm-central - http://mxr.mozilla.org/comm-central/search?string=margin-left&find=suite&tree=comm-central Use of margin-right (should be replaced with -moz-margin-end): - http://mxr.mozilla.org/comm-central/search?string=margin-right&find=mail&tree=comm-central - http://mxr.mozilla.org/comm-central/search?string=margin-right&find=calendar&tree=comm-central - http://mxr.mozilla.org/comm-central/search?string=margin-right&find=suite&tree=comm-central In addition all "margin:" and "padding:" styles rules with four (4) values need to be audited for differing left and right values. Those style rules will need to be replaced with appropriate *-top, *-bottom, *-start and *-end rules
Flags: wanted-thunderbird3?
Assignee | ||
Comment 1•15 years ago
|
||
Assignee | ||
Updated•15 years ago
|
Attachment #358470 -
Flags: superreview?(neil)
Attachment #358470 -
Flags: review?(philringnalda)
Assignee | ||
Comment 2•15 years ago
|
||
Comment on attachment 358470 [details] [diff] [review] Patch v1 (for mail, mailnews, suite and calendar) Asking for review from Phil for the Thunderbird parts and from Neil for the Suite parts.
Assignee | ||
Updated•15 years ago
|
Attachment #358470 -
Flags: review?(Berend.Cornelius)
Assignee | ||
Comment 3•15 years ago
|
||
Comment on attachment 358470 [details] [diff] [review] Patch v1 (for mail, mailnews, suite and calendar) Asking for Berend's review for the Calendar parts.
Assignee | ||
Updated•15 years ago
|
Attachment #358470 -
Flags: review?(neil)
Comment 4•15 years ago
|
||
Comment on attachment 358470 [details] [diff] [review] Patch v1 (for mail, mailnews, suite and calendar) I hate the fact that there's no shortcut syntax for rtl-aware padding/margin. Just think about all the extra CSS parsing that has to be done :-( Is there going to be another bug for border rules? There were five places in Classic where the left and right padding or margin as appropriate were the same, yet you still converted them? > .tab-drop-indicator-bar { > height: 11px; > margin-top: -11px; >- margin-left: -8px; >+ -moz-margin-start: -8px; This might not actually be correct. The tab drop indicator is left-aligned with the drop point, and this margin moves the indicator to the left so that it will be centred on the drop point. (The indicator is 17px wide?)
Comment 5•15 years ago
|
||
Simon, just by glancing over the patch I could see that you are using a mozilla-specific property in places where you must not use them. The use in the css files seems Ok but you cannot use them in the source code files Please see https://developer.mozilla.org/en/DOM/window.getComputedStyle for further information. With 'calHtmlExport.js' it is a bit different. This file exports "common" html, so Mozilla - specific terms should also not turn up here.
Comment 6•15 years ago
|
||
The start.xhtml hunk is wrong - you're setting 25px of padding start, then setting 25px of padding-end for body[dir="rtl"] so in rtl it'll be squeezed from both sides - but it's also dead code that will be gone before you can get review. I also very much doubt that a single patch is a good idea here: when you're doing things like changing forked strings for shared XUL it's reasonable, but when the only connection is that it's the same sort of thing done by the same person to completely separate code in three different apps, it just makes your reviewers' lives more difficult and slows you down.
Assignee | ||
Comment 7•15 years ago
|
||
(In reply to comment #4) > Is there going to be another bug for border rules? There will be no bug for border styles, because as far as I know -moz-border-start and -moz-border-left haven't been implemented yet in Gecko. Although looking at it more closely, I'm getting a little confused. Bug 260715 is still open, but bug 260715 comment 3 bug sasy that -moz-border-start and -moz-border-end were already implemented in bug 74880. I'll further investigate this. > There were five places in Classic where the left and right padding or > margin as appropriate were the same, yet you still converted them? My mistake. I'll fix this and provide a new patch. >> .tab-drop-indicator-bar { >> height: 11px; >> margin-top: -11px; >>- margin-left: -8px; >>+ -moz-margin-start: -8px; > This might not actually be correct. The tab drop indicator is left-aligned > with the drop point, and this margin moves the indicator to the left so that > it will be centred on the drop point. (The indicator is 17px wide?) I do not fully understand this. Are you saying that this margin always has to be on the left, regardless of UI direction?
Comment 8•15 years ago
|
||
Yes, -moz-border-start and -end exist, but things like -moz-border-radius-topstart do not, which means you have to use separate chromedir selectors instead when you have rounded corners.
Comment 9•15 years ago
|
||
(In reply to comment #7) >(In reply to comment #4) >>> .tab-drop-indicator-bar { >>> height: 11px; >>> margin-top: -11px; >>>- margin-left: -8px; >>>+ -moz-margin-start: -8px; >>This might not actually be correct. The tab drop indicator is left-aligned >>with the drop point, and this margin moves the indicator to the left so that >>it will be centred on the drop point. (The indicator is 17px wide?) >I do not fully understand this. Are you saying that this margin always has to >be on the left, regardless of UI direction? Actually, I think there's a bug in our calculations. Let me investigate.
Assignee | ||
Comment 10•15 years ago
|
||
New patch, which incorporates the feedback I've got so far. I'll also attach this patch in three parts (for calendar, mail and suite) to make review easier.
Attachment #358470 -
Attachment is obsolete: true
Attachment #358470 -
Flags: superreview?(neil)
Attachment #358470 -
Flags: review?(philringnalda)
Attachment #358470 -
Flags: review?(neil)
Attachment #358470 -
Flags: review?(Berend.Cornelius)
Assignee | ||
Comment 11•15 years ago
|
||
Attachment #358694 -
Flags: review?(Berend.Cornelius)
Assignee | ||
Comment 12•15 years ago
|
||
Attachment #358695 -
Flags: review?(philringnalda)
Assignee | ||
Comment 13•15 years ago
|
||
Attachment #358696 -
Flags: superreview?(neil)
Attachment #358696 -
Flags: review?(neil)
Updated•15 years ago
|
Flags: wanted-thunderbird3? → wanted-thunderbird3+
Comment 14•15 years ago
|
||
Comment on attachment 358696 [details] [diff] [review] Patch v2 - Suite part I looked through the whole patch for equal padding and margin and I also found a typo. r+sr=me with them all fixed. Thanks for doing the work! > #sidebar-panel-picker > .toolbarbutton-dropmarker { >- padding-left: 2px; >- padding-right: 2px; >+ -moz-padding-start: 2px; >+ -moz-padding-end: 2px; Equal padding. > #ColorPreview { > border: 1px inset #CCCCCC; >- margin-left: 10px; >- padding-left: 5px; >- padding-right: 5px; >+ -moz-margin-start: 10px; >+ -moz-padding-start: 5px; >+ -moz-padding-end: 5px; Equal padding. > #CardViewInnerBox { > margin-top: 2px; > margin-bottom: 2px; >- padding-left: 8px; >- padding-right: 8px; >+ -moz-padding-start: 8px; >+ -moz-padding-end: 8px; Equal padding. > #tagList > listitem > listcell > { >- padding-left: 16px; >- padding-right: 16px; >+ -moz-padding-start: 16px; >+ -moz-padding-end: 16px; Equal padding. > #ColorPreview { >- margin-left: 10px; >+ -moz-margin-start: 10px; > border: 1px inset #B4C3D4; >- padding-left: 5px; >- padding-right: 5px; >+ -moz-padding-start: 5px; >+ -moz-padding-end: 5px; Equal padding. > .source-editor, > .source-editor:focus { >- margin: 0px 5px 5px 0px; >+ margin-top: 9px; Typo? > .alertTextBox { > margin-top: 4px; >- margin-right: 6px; >- margin-left: 6px; >+ -moz-margin-end: 6px; >+ -moz-margin-start: 6px; Equal margin. > #warningBox > { > background-color: #C7D0D9; > color: #22262F; > border: 1px solid #494F5D; > -moz-border-radius: 10px; > padding: 3em; > -moz-padding-start: 30px; >- margin-left: 1em; >- margin-right: 1em; >+ -moz-margin-start: 1em; >+ -moz-margin-end: 1em; Equal margin. > separator.groove[orient="vertical"] { > border-left: 1px solid #7A8490; > border-right: 1px solid #FEFEFE; >- margin-left: 0.4em; >- margin-right: 0.4em; >+ -moz-margin-start: 0.4em; >+ -moz-margin-end: 0.4em; Equal margin. > div#outside { > text-align: justify; > width: 90%; >- margin-left: 5%; >- margin-right: 5%; >+ -moz-margin-start: 5%; >+ -moz-margin-end: 5%; Equal margin. > #CardViewInnerBox { > margin-top: 2px; > margin-bottom: 2px; >- padding-right: 8px; >- padding-left: 8px; >+ -moz-padding-end: 8px; >+ -moz-padding-start: 8px; Equal padding. > #tagList > listitem > listcell > { >- padding-left: 16px; >- padding-right: 16px; >+ -moz-padding-start: 16px; >+ -moz-padding-end: 16px; Equal padding. > #feedListbox > richlistitem { > padding-top: 6px; > padding-bottom: 6px; >- padding-left: 7px; >- padding-right: 7px; >+ -moz-padding-start: 7px; >+ -moz-padding-end: 7px; Equal padding.
Attachment #358696 -
Flags: superreview?(neil)
Attachment #358696 -
Flags: superreview+
Attachment #358696 -
Flags: review?(neil)
Attachment #358696 -
Flags: review+
Comment 15•15 years ago
|
||
Comment on attachment 358695 [details] [diff] [review] Patch v2 - Thunderbird part r=me with a couple of little things fixed: >+++ b/mail/themes/gnomestripe/mail/editContactOverlay.css > #editContactPanelTitle { > font-size: 130%; > font-weight: bold; >- margin: 8px 12px 0px 9px; >+ margin-top: 8px; >+ margin-bottom: 0px; >+ -moz-margin-start: 9py; >+ -moz-margin-end: 12px; > } Much as I like Python, 9py isn't a very good CSS length. >+++ b/mail/themes/gnomestripe/mail/newmailalert.css > .folderSummary-message-row > { > /* This max width ends up dictating the overall width of the alert window > because it controls how large the preview, subject and sender text can be > before cropping kicks in */ > max-width: 450px; >- padding: 0px 5px 0px 5px; >+ padding: 0px 5px; > } Trailing whitespace, and in the /qute/ copy where that came from, too. (I was tempted to say you had to s/0px/0/g, but we have such a hodgepodge of the two styles it would be impossible to say when to stop, so that can wait until a sucker comes along who will do them all at once.)
Attachment #358695 -
Flags: review?(philringnalda) → review+
Comment 16•15 years ago
|
||
Comment on attachment 358694 [details] [diff] [review] Patch v2 - calendar part patch looks and works well. r=berend
Attachment #358694 -
Flags: review?(Berend.Cornelius) → review+
Assignee | ||
Comment 17•15 years ago
|
||
Changeset 0174b636d5ee pushed to comm-central. Thanks for the fast turnaround with the reviews, guys!
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•15 years ago
|
Target Milestone: --- → Thunderbird 3.0b2
Comment 18•15 years ago
|
||
There is a colon missing on the line 409 of <https://bugzilla.mozilla.org/attachment.cgi?id=358695>: >+ -moz-margin-start 0px;
Comment 19•15 years ago
|
||
Yes, I found this too. please fix comment #18.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 20•15 years ago
|
||
Changeset 84b1310158e3 pushed to fix the colon issue. Thanks for noticing.
Status: REOPENED → RESOLVED
Closed: 15 years ago → 15 years ago
Resolution: --- → FIXED
Comment 21•15 years ago
|
||
Changeset 0174b636d5ee contained some incorrect shorthands, one of which caused bug 533348. Below is a list of definite and possible issues. Some of the line references might be wrong because I snipped all non-issues. Definite issues: * #acctCentralHeaderRow: padding: 10px 0px 10px is not equivalent to padding: 10px 0px 10px 10px * .alertTextBox: margin 0 6px is not equivalent to margin-top: 4px; margin-right: 6px; margin-left: 6px; * #feedListbox > richlistitem: padding: 0 7px is not equivalent to padding-top: 6px; padding-bottom: 6px; padding-left: 7px; padding-right: 7px; Possible issues: * all other listed cases where margin-left:X; margin-right:X were combined into margin: 0 X (equally for padding). Setting top/bottom margins/paddings to zero is not equivalent to not setting them (think inheritance!). I think we need to correct at least the definite issues, probably also on the branch. These only concern SeaMonkey's Modern theme so we should handle the three in one or more SM bugs. For the possible issues I'll leave it to the respective reviewers or application developers whether they want to check them. The safest thing would probably be to undo those changes. +++ b/calendar/base/themes/pinstripe/calendar-occurrence-prompt.css Mon Jan 26 22:29:03 2009 +0100 @@ -45,8 +45,7 @@ #occurrence-prompt-header { height: 50px; - padding-left: 15px; - padding-right: 15px; + padding: 0 15px; border-bottom: 2px groove ThreeDFace; background-color: window; color: windowtext; @@ -57,8 +56,7 @@ } #accept-buttons-box { - padding-left: 18px; - padding-right: 18px; + padding: 0 18px; border-bottom: 2px groove ThreeDFace; } +++ b/calendar/base/themes/winstripe/calendar-occurrence-prompt.css Mon Jan 26 22:29:03 2009 +0100 @@ -45,8 +45,7 @@ #occurrence-prompt-header { height: 50px; - padding-left: 15px; - padding-right: 15px; + padding: 0 15px; border-bottom: 2px groove ThreeDFace; background-color: window; color: windowtext; @@ -57,8 +56,7 @@ } #accept-buttons-box { - padding-left: 18px; - padding-right: 18px; + padding: 0 18px; border-bottom: 2px groove ThreeDFace; } +++ b/mail/base/content/credits.xhtml Mon Jan 26 22:29:03 2009 +0100 @@ -89,8 +89,7 @@ overflow: hidden; z-index: 1; width: 280px; - margin-left: 10px; - margin-right: 10px; + margin: 0 10px; font-family: Arial, sans-serif; font-size: small; } +++ b/mail/themes/gnomestripe/editor/EditorDialog.css Mon Jan 26 22:29:03 2009 +0100 #ColorPreview { border: 1px inset #CCCCCC; -moz-margin-start: 10px; - padding-left: 5px; - padding-right: 5px; + padding: 0 5px; min-width: 100px; min-height: 50px; } +++ b/mail/themes/gnomestripe/mail/accountManage.css Mon Jan 26 22:29:03 2009 +0100 .selectForOfflineUseButton > .button-box > .button-icon { - margin-left: 4px; - margin-right: 4px; + margin: 0 4px; } +++ b/mail/themes/gnomestripe/mail/addrbook/addressbook.css Mon Jan 26 22:29:03 2009 +0100 #CardViewInnerBox { margin-top: 2px; margin-bottom: 2px; - padding-left: 8px; - padding-right: 8px; + padding: 0 8px; } +++ b/mail/themes/pinstripe/editor/EditorDialog.css Mon Jan 26 22:29:03 2009 +0100 #ColorPreview { border: 1px inset #CCCCCC; - margin-left: 10px; - padding-left: 5px; - padding-right: 5px; + -moz-margin-start: 10px; + padding: 0 5px; min-width: 100px; min-height: 50px; } +++ b/mail/themes/pinstripe/mail/compose/messengercompose.css Mon Jan 26 22:29:03 2009 +0100 #italicButton, #olButton, #outdentButton { background: url("chrome://messenger/skin/messengercompose/mid-button-base.png") no-repeat; - margin-left: 0px; - margin-right: 0px; + margin: 0; width: 27px !important; } +++ b/mail/themes/pinstripe/mail/mailWindow1.css Mon Jan 26 22:29:03 2009 +0100 @@ -352,8 +355,7 @@ treechildren::-moz-tree-row(dummy) { background-repeat: repeat-x; background-color: rgb(246, 246, 246); border: 1px solid rgb(197, 199, 202); - margin-right: 1px; - margin-left: 1px; + margin: 0 1px; } @@ -361,8 +363,7 @@ treechildren::-moz-tree-row(dummy, focus border-color: #3874d1; background-image: none !important; margin: 0px; - padding-right: 1px; - padding-left: 1px; + padding: 0 1px; } +++ b/mail/themes/qute/editor/EditorDialog.css Mon Jan 26 22:29:03 2009 +0100 #ColorPreview { border: 1px inset #CCCCCC; -moz-margin-start: 10px; - padding-left: 5px; - padding-right: 5px; + padding: 0 5px; min-width: 100px; min-height: 50px; } +++ b/mail/themes/qute/mail/accountManage.css Mon Jan 26 22:29:03 2009 +0100 .selectForOfflineUseButton > .button-box > .button-icon { - margin-left: 4px; - margin-right: 4px; + margin: 0 4px; } +++ b/mail/themes/qute/mail/addrbook/addressbook.css Mon Jan 26 22:29:03 2009 +0100 #CardViewInnerBox { margin-top: 2px; margin-bottom: 2px; - padding-left: 8px; - padding-right: 8px; + padding: 0 8px; } +++ b/suite/themes/classic/communicator/sidebar/sidebar.css Mon Jan 26 22:29:03 2009 +0100 #sidebar-panel-picker > .toolbarbutton-dropmarker { - padding-left: 2px; - padding-right: 2px; + padding: 0 2px; list-style-image: url("chrome://global/skin/arrow/arrow-dn.gif"); } +++ b/suite/themes/classic/editor/EditorDialog.css Mon Jan 26 22:29:03 2009 +0100 #ColorPreview { border: 1px inset #CCCCCC; - margin-left: 10px; - padding-left: 5px; - padding-right: 5px; + -moz-margin-start: 10px; + padding: 0 5px; min-width: 100px; min-height: 50px; } +++ b/suite/themes/classic/messenger/addressbook/addressbook.css Mon Jan 26 22:29:03 2009 +0100 #CardViewInnerBox { margin-top: 2px; margin-bottom: 2px; - padding-left: 8px; - padding-right: 8px; + padding: 0 8px; } +++ b/suite/themes/modern/editor/EditorDialog.css Mon Jan 26 22:29:03 2009 +0100 #ColorPreview { - margin-left: 10px; + -moz-margin-start: 10px; border: 1px inset #B4C3D4; - padding-left: 5px; - padding-right: 5px; + padding: 0 5px; min-width: 100px; min-height: 50px; } +++ b/suite/themes/modern/global/alerts/alert.css Mon Jan 26 22:29:03 2009 +0100 .alertTextBox { margin-top: 4px; - margin-right: 6px; - margin-left: 6px; + margin 0 6px; } +++ b/suite/themes/modern/global/config.css Mon Jan 26 22:29:03 2009 +0100 @@ -60,8 +60,7 @@ -moz-border-radius: 10px; padding: 3em; -moz-padding-start: 30px; - margin-left: 1em; - margin-right: 1em; + margin: 0 1em; } +++ b/suite/themes/modern/global/global.css Mon Jan 26 22:29:03 2009 +0100 @@ -274,8 +274,7 @@ separator.groove[orient="horizontal"] { separator.groove[orient="vertical"] { border-left: 1px solid #7A8490; border-right: 1px solid #FEFEFE; - margin-left: 0.4em; - margin-right: 0.4em; + margin: 0 .4em; } +++ b/suite/themes/modern/global/plugins.css Mon Jan 26 22:29:03 2009 +0100 div#outside { text-align: justify; width: 90%; - margin-left: 5%; - margin-right: 5%; + margin: 0 5%; } +++ b/suite/themes/modern/messenger/accountCentral.css Mon Jan 26 22:29:03 2009 +0100 #acctCentralHeaderRow { - padding: 10px 0px 10px 10px; + padding: 10px 0px 10px; font-size: 180%; font-weight: bold; color: #000000; +++ b/suite/themes/modern/messenger/addressbook/addressbook.css Mon Jan 26 22:29:03 2009 +0100 #CardViewInnerBox { margin-top: 2px; margin-bottom: 2px; - padding-right: 8px; - padding-left: 8px; + padding: 0 8px; } +++ b/suite/themes/modern/messenger/prefPanels.css Mon Jan 26 22:29:03 2009 +0100 -#tagList > listitem > listcell -{ - padding-left: 16px; - padding-right: 16px; +#tagList > listitem > listcell { + padding: 0 16px; } +++ b/suite/themes/modern/navigator/pageInfo.css Mon Jan 26 22:29:03 2009 +0100 #feedListbox > richlistitem { padding-top: 6px; padding-bottom: 6px; - padding-left: 7px; - padding-right: 7px; + padding: 0 7px; min-height: 25px; border-bottom: 1px dotted #C7D0D9; }
Comment 22•15 years ago
|
||
According to my copy of CSS 2.1, both padding and margin have initial value: 0 and inherited: no. Does yours say something different than that, or did you mean "(think cascade)"? I certainly intended to do that at the time, but I may not have succeeded at doing it.
Comment 23•15 years ago
|
||
(In reply to comment #22) > According to my copy of CSS 2.1, both padding and margin have initial value: 0 > and inherited: no. Does yours say something different than that, or did you > mean "(think cascade)"? The latter, I was thinking OOP here, I'm not a CSS crack. ;-)
Comment 24•15 years ago
|
||
I only agree with the three definite issues, which we can fix in bug 533348?
You need to log in
before you can comment on or make changes to this bug.
Description
•