474807 - Margin and Padding CSS rules should be independent of UI direction (LTR or RTL)

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Simon Paquet [:sipaq]]

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

Comment 1

[Simon Paquet [:sipaq]]

Attachment: Patch v1 (for mail, mailnews, suite and calendar)

Comment 2

[Simon Paquet [:sipaq]]

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.

Comment 3

[Simon Paquet [:sipaq]]

Comment on attachment 358470 [details] [diff] [review]
Patch v1 (for mail, mailnews, suite and calendar)

Asking for Berend's review for the Calendar parts.

Comment 4

[[e-mail]]

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

[Berend Cornelius]

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

[Phil Ringnalda (:philor)]

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.

Comment 7

[Simon Paquet [:sipaq]]

(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

[Phil Ringnalda (:philor)]

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

[[e-mail]]

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

Comment 10

[Simon Paquet [:sipaq]]

Attachment: Patch v2 (for mail, mailnews, suite and calendar)

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.

Comment 11

[Simon Paquet [:sipaq]]

Attachment: Patch v2 - calendar part

Comment 12

[Simon Paquet [:sipaq]]

Attachment: Patch v2 - Thunderbird part

Comment 13

[Simon Paquet [:sipaq]]

Attachment: Patch v2 - Suite part

Comment 14

[[e-mail]]

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.

Comment 15

[Phil Ringnalda (:philor)]

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

Comment 16

[Berend Cornelius]

Comment on attachment 358694 [details] [diff] [review]
Patch v2 - calendar part

patch looks and works well. r=berend

Comment 17

[Simon Paquet [:sipaq]]

Changeset 0174b636d5ee pushed to comm-central.
Thanks for the fast turnaround with the reviews, guys!

Comment 18

[Ilja Sekler]

There is a colon missing on the line 409 of <https://bugzilla.mozilla.org/attachment.cgi?id=358695>:

>+  -moz-margin-start 0px;

Comment 19

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Yes, I found this too. please fix comment #18.

Comment 20

[Simon Paquet [:sipaq]]

Changeset 84b1310158e3 pushed to fix the colon issue. Thanks for noticing.

Comment 21

[Jens Hatlak (:InvisibleSmiley)]

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

[Phil Ringnalda (:philor)]

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

[Jens Hatlak (:InvisibleSmiley)]

(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

[[e-mail]]

I only agree with the three definite issues, which we can fix in bug 533348?