422369 - Make Calendar styles LTR (left-to-right) and RTL (right-to-left) agnostic

Status: RESOLVED FIXED

(Calendar :: General, defect)

Description

[Simon Paquet [:sipaq]]

We currently use CSS styles like margin-left, margin-right, padding-left or padding-right a lot in our codebase. We should substitute those with their direction-agnostic correspondents:

-moz-padding-start
-moz-padding-end
-moz-margin-start
-moz-margin-end

Comment 1

[Simon Paquet [:sipaq]]

Additionally we need to adjust all the "margin/padding: $Top $Right $Bottom $Left" statements, where $Right and $Left have different values.

Comment 2

[Simon Paquet [:sipaq]]

Attachment: The easy part - v1

This is the easy part, where I just searched and replaced

padding-left   -->  -moz-padding-start
padding-right  -->  -moz-padding-end
margin-left    -->  -moz-margin-start
margin-right   -->  -moz-margin-end

See also:
http://developer.mozilla.org/en/docs/CSS:-moz-padding-start
http://developer.mozilla.org/en/docs/CSS:-moz-padding-end
http://developer.mozilla.org/en/docs/CSS:-moz-margin-start
http://developer.mozilla.org/en/docs/CSS:-moz-margin-end

Comment 3

[Simon Paquet [:sipaq]]

Attachment: The complete package

This is a big patch, for which I apologize. I managed to put this all into one patch. This patch does

padding-left   -->  -moz-padding-start
padding-right  -->  -moz-padding-end
margin-left    -->  -moz-margin-start
margin-right   -->  -moz-margin-end
border-left    -->  -moz-border-start
border-right   -->  -moz-border-end

In addition I split up padding and margin rules, where left/right values differed. I also consolidated rules, where left/right and/or top/bottom rules did not differ.

Comment 4

[Berend Cornelius]

I looked through the patch and found everything looked Ok at first sight. Concerning your changes in "calHtmlExport.js" I don't think we should use mozilla-specific styles here, although on the other hand I admit that as far as I know there are not yet  standardized properties available that consider bi-directionality.
When applying the patch I received many warnings because of the "-moz-border-start" and -moz-border-end" properties ("Unknown property "-moz-border-start"...) I tried to investigate why this is because as far as I see it should be available in MOZILLA_1_8_BRANCH already, but could not get information about it.
Did you test this patch in a real localized testcase like arabic?

Comment 5

[Simon Paquet [:sipaq]]

(In reply to comment #4)
> Concerning your changes in "calHtmlExport.js" I don't think we should use
> mozilla-specific styles here,

You're right. I misinterpreted the use-case of this file. I'll post an updated patch tonight.

> When applying the patch I received many warnings because of the
> "-moz-border-start" and -moz-border-end" properties ("Unknown property
> "-moz-border-start"...) I tried to investigate why this is because as far as I
> see it should be available in MOZILLA_1_8_BRANCH already, but could not get
> information about it.

I'm sorry, I had implemented this patch only for the margin and padding stuff, 
when I read a mention of -moz-border-start and -end on devmo. I therefore 
assumed, that both were already implemented, but according to bug 260715 they 
aren't yet.

Sorry, for not retesting the patch after this last-minute change by me. I'll 
remove the -moz-border-stuff from the patch.

> Did you test this patch in a real localized testcase like arabic?
 
No, I lack such a test environment. I only tested this in a German and English 
environment, to make sure, that it does not break things in the LTR-world.

Comment 6

[Simon Paquet [:sipaq]]

Attachment: The complete package - v2

New patch based on feedback from Berend in comment 4.

Changes:
- Removed changes to calHtmlExport.js
- Removed border-left --> -moz-border-start change
- Removed border-right --> -moz-border-end change

Comment 7

[Berend Cornelius]

This is an important step to head for bidirectionality although I haven't tested it in an according environment either. There will certainly be further open issues for bidirectionality, e.g. due to previously mentioned border settings (see comment #4 and comment #5) and css attributes like "text-align" that don't consider this topic and many other problems. I looked again through the patch and tested it without detecting any abnormalities or entries in the error console under Sunbird and Lightning.

Here just some minor remarks:

In lightning.css and calendar-views.css (pinstripe and winstripe) I noticed several rules following the scheme like
> .next-button:hover:active {
> -    margin: 6px 14px 4px 16px;
> +    margin-top: 6px;
> +    margin-bottom: 4px;
> +    -moz-margin-start: 16px;
> +    -moz-margin-end: 14px;
>  }
or 
>  #multiweek-next-button:hover:active {
> -    margin: 6px 4px 4px 6px;
> +    margin-top: 6px;
> +    margin-bottom: 4px;
> +    -moz-margin-start: 6px;
> +    -moz-margin-end: 4px;
>  }
Maybe you could summarize these rules.
After your replacements the attributes were frequently not aligned properly anymore, e.g
-   margin-left             : 2px;
+   -moz-margin-start             : 2px;

Thanky you for your effort!

Comment 8

[(no longer active)]

Mass-assigning the new rtl keyword to RTL-related (see bug 349193).

Comment 9

[Simon Paquet [:sipaq]]

Berend, I checked in my patch without a change to get it out of my queue. I tried to fix the aligning issues before that, but don't understand your first comment. Could you please elaborate?

Comment 10

[Berend Cornelius]

>In lightning.css and calendar-views.css (pinstripe and winstripe) I noticed
>several rules following the scheme like
>> .next-button:hover:active {
>> -    margin: 6px 14px 4px 16px;
>> +    margin-top: 6px;
>> +    margin-bottom: 4px;
>> +    -moz-margin-start: 16px;
>> +    -moz-margin-end: 14px;
>>  }

I meant to say that you could consolidate the rules by implementing
.prev-button:hover:active,
.next-button:hover:active {
    margin: 6px 14px 4px 16px;
    margin-top: 6px;
    margin-bottom: 4px;
    -moz-margin-start: 16px;
    -moz-margin-end: 14px;
}

Then I found out that the rules in question happened not to be needed anymore - partly also because you removed Sunbird's calendar-views.css files in bug 427237. So the patch attached removes the remaining rules. But the reason for me to reopen the issue is that you deleted a closing bracket in the today-pane.css files.

Comment 11

[Berend Cornelius]

Attachment: additional patch

Simon, please handle this patch as you think it's best. Maybe you find something that could still be improved.

Comment 12

[Simon Paquet [:sipaq]]

Berend, I checked in a followup patch to add the closing brackets again. Sorry for missing those. I also filed bug 428798 for the obsolete CSS rules.