552486 - Use text-align: start or end instead of left or right in SeaMonkey themes

Status: ASSIGNED

(SeaMonkey :: Themes, defect)

Description

[[e-mail]]

Since bug 299837 it is now possible to use text-align: start or end for automatic RTL compatibility.

Comment 1

[[e-mail]]

Although now that I think about it aren't numbers always text-align: right;?

Comment 2

[Tony Mechelynck [:tonymec]]

Arabic is an RTL language where numbers (even when written in Arabic-Indic digits) are LTR. I think the same applies to Persian and Urdu, which, although Indo-European, use variants of the Arabic script.

In Hebrew (another RTL language) numbers are often written with letters: "number one" would be alef; "number two", beth; "number nine", teth; "number ten", yod; "number eleven", yod-alef; "number twelve", yod-beth; "number twenty-six", kaf-vav; etc. In that case "numbers" are RTL too but I suppose this kind of numbering is not used in the chrome.

I'm not sure about Syriac (aka Aramaean), an RTL language which has its own script, different from both Hebrew "square script" and Arabic "flowing hand".

Comment 3

[Sarup Banskota]

Hi all,

I'm new to Mozilla and Open Source in general. I would like to take up this bug, and will need help. So, how do I start?

Comment 4

[Philip Chee]

Hi

First have you set up a build environment? If not please see:
https://developer.mozilla.org/en-US/docs/Simple_SeaMonkey_build

If you can please join us on IRC:  irc://moznet/seamonkey  where we can discuss this further. Most of our developers are either in Europe or Asia so we may not be around at certain times.

Comment 5

[harsh chhabra]

hi sir,
i am interested in solving the above bug, can i ?

Comment 6

[Philip Chee]

Hi harsh chhabra! I think Sarup Banskota was here first so he should be allowed to fix this bug. If Sarup is unable to work on this bug, I'll reassign it to you.

Comment 7

[Sarup Banskota]

Hi, 

I'm sorry for getting back late. I haven't set up an environment yet, because I have a sad internet connection for a few days.
And sure, I will join the IRC channel and seek help :) Thanks!

Comment 8

[Josh Matthews [:jdm]]

Sarup, could you confirm that you're still working on this?

Comment 9

[Sarup Banskota]

Josh, I am working on my GSoC proposal right now, so you can assign this to someone else :)

Comment 10

[Frank Wein [:mcsmurf]]

Resetting assignee status

Comment 11

[Bodhiswattwa Tewari]

Hi,
I am a computer engineering student from India. I am very much interested in FOSS.I want to try fixing this bug. What shall I do?

Comment 12

[Frank Wein [:mcsmurf]]

Bodhiswattwa: Sorry for the late response, are you still interested? If yes, check out Comment 4 in this bug. I think a good first step would be to setup a build env, you'll need it later. After or while doing that, you might want to take a look at this page: http://mxr.mozilla.org/comm-central/search?string=text-align&find=suite*.css&findi=\.css&filter=^[^\0]*%24&hitlimit=&tree=comm-central (this is basically the SeaMonkey code repository and you can search throught the code with the help of that page). There you can see some of the places that would need to get fixed. If you're done, report back :). As written Comment 4, you can also talk to us on IRC.

Comment 13

[Bodhiswattwa Tewari]

Yes I am still interested in it. I already have Microsoft Visual Studio 2012. I hope this will come out to be compatible with my work. I am looking forward to fix this bug.

Comment 14

[Samiullah]

Hi there

Can I be assigned this bug as I am interested in attempting to fix it, my knowledge in HTML is sufficient to apply certain changes to the bug.

Comment 15

[Philip Chee]

(In reply to Samiullah from comment #14)
> Hi there
> 
> Can I be assigned this bug as I am interested in attempting to fix it, my
> knowledge in HTML is sufficient to apply certain changes to the bug.

Hi! You've been assigned on Bug 812050. Lets start with that first. After fixing that we'll look again at which bugs you want to work on.

Comment 16

[alexandre-cote]

Hello, 
I would like to be assigned to this bug, is it possible ?
There is already somebody working on it but it was decided last year. Did he fix it ?

Comment 17

[Stefan [:stefanh]]

(In reply to Alex from comment #16)
> Hello, 
> I would like to be assigned to this bug, is it possible ?
> There is already somebody working on it but it was decided last year. Did he
> fix it ?

Hi Alex,
Of course, thanks for looking at this :-) (it's not fixed) - assigning to you.

Comment 18

[alexandre-cote]

Attachment: rev1

Comment 19

[Neil Lee [:neilio]]

Comment on attachment 8415992 [details] [diff] [review]
rev1

Review of attachment 8415992 [details] [diff] [review]:
-----------------------------------------------------------------

Needs to be sent to the correct person.

Comment 20

[Stefan [:stefanh]]

Alex,
I think you ment to ask another neil for review: [e-mail] :-)

But it looks like you have attached the wrong patch - the files you have changed doesn't exist in the comm-central repository (the SeaMonkey-specific code is located in comm-central).

Comment 21

[Brian Grinstead [:bgrins]]

Comment on attachment 8415992 [details] [diff] [review]
rev1

Review of attachment 8415992 [details] [diff] [review]:
-----------------------------------------------------------------

Hi Alex, it looks like this patch is applying to DevTools.  I've opened Bug 1004679 so that we can work on getting this landed without adding a bunch of noise to this bug.  Would you mind uploading it there instead?

Comment 22

[alexandre-cote]

(In reply to Stefan [:stefanh] from comment #20)
> Alex,
> I think you ment to ask another neil for review: [e-mail] :-)
> 
> But it looks like you have attached the wrong patch - the files you have
> changed doesn't exist in the comm-central repository (the SeaMonkey-specific
> code is located in comm-central).

Hello,

Indeed, I corrected the wrong files (in mozilla-central), my bad :(... 
I should be able to submit a new patch soon. However when I submitted the patch, the only reviewer I was able to select in the list was this person. How can I ask for [e-mail] ?

Comment 23

[Josh Matthews [:jdm]]

(In reply to Alex from comment #22)
> Hello,
> 
> Indeed, I corrected the wrong files (in mozilla-central), my bad :(... 
> I should be able to submit a new patch soon. However when I submitted the
> patch, the only reviewer I was able to select in the list was this person.
> How can I ask for [e-mail] ?

There's a free-form text entry right next to the list.

Comment 24

[alexandre-cote]

Attachment: rev2

I modified the files in /comm-central this time.

I did the SeaMonkey build as explained in this tutorial : https://developer.mozilla.org/en-US/docs/Simple_SeaMonkey_builds.

Each file is contained in a /devtools subfolder.

Comment 25

[[e-mail]]

Comment on attachment 8416966 [details] [diff] [review]
rev2

It looks as if you may not have understood.

Although they are necessarily checked out during a SeaMonkey build, those devtools files don't actually form part of SeaMonkey.

Now this doesn't mean that you were wrong to patch them, and bug 1004679 has been created in order that your patch may be correctly assessed. Feel free to concentrate on fixing the devtools files using that bug for that purpose.

However for any patch to be relevant to this particular bug, you need to patch files in comm-central's suite directory tree.

It is of course completely up to you to choose which set of files to work on.

Comment 26

[alexandre-cote]

(In reply to [e-mail] from comment #25)
> Comment on attachment 8416966 [details] [diff] [review]
> rev2
> 
> It looks as if you may not have understood.
> 
> Although they are necessarily checked out during a SeaMonkey build, those
> devtools files don't actually form part of SeaMonkey.
> 
> Now this doesn't mean that you were wrong to patch them, and bug 1004679 has
> been created in order that your patch may be correctly assessed. Feel free
> to concentrate on fixing the devtools files using that bug for that purpose.
> 
> However for any patch to be relevant to this particular bug, you need to
> patch files in comm-central's suite directory tree.
> 
> It is of course completely up to you to choose which set of files to work on.

Ok, I submitted my patch for bug 1004679...

But yes, I didn't understand, and I'm really trying to...
The files I modified in this bug are in my /comm-central repository. And these are the only files in comm-central/mozilla/brower/themes which need to be changed about the text-align modification.
I'm sorry for being ignorant but I need more explanations...

Comment 27

[Stefan [:stefanh]]

SeaMonkey-specific css files are to be found at comm-central/suite/themes. Thunderbird-specific css files can be found at comm-central/mail/themes. Files in comm-central/mozilla/browser/themes are Firefox-specific and they are part of the mozilla-central repository. When you check out comm-central with the python script, you get the mozilla-central repository as well (that's the mozilla directory in comm-central). The comm-central repository looks like this: http://hg.mozilla.org/comm-central/file and the mozilla-central repository looks like this: http://hg.mozilla.org/mozilla-central/file.

Comment 28

[alexandre-cote]

Attachment: new try

Comment 29

[alexandre-cote]

Attachment: new try

Comment 30

[alexandre-cote]

(In reply to Alex from comment #29)
> Created attachment 8420677 [details] [diff] [review]
> new try

Could it be possible to have a review soon ?

Comment 31

[Philip Chee]

> Could it be possible to have a review soon ?
This is a relatively simple patch. I'm sure Neil will be reviewing it soon.

Comment 32

[[e-mail]]

Comment on attachment 8420677 [details] [diff] [review]
new try

Very sorry not to be able to get to this earlier.

Some of these rules apply to numbers rather than text. Unfortunately I was not able to get a definitive answer as to how numbers should be aligned, so for now I'd like them to stay as right rather than end. As such, I would appreciate it if you could remove the following numeric-related changes from your patch. Thank you.

>diff --git a/suite/themes/classic/messenger/folderPane.css b/suite/themes/classic/messenger/folderPane.css
>diff --git a/suite/themes/classic/messenger/messageBody.css b/suite/themes/classic/messenger/messageBody.css
>diff --git a/suite/themes/classic/messenger/threadPane.css b/suite/themes/classic/messenger/threadPane.css
>diff --git a/suite/themes/modern/global/datetimepicker.css b/suite/themes/modern/global/datetimepicker.css
>diff --git a/suite/themes/modern/global/numberbox.css b/suite/themes/modern/global/numberbox.css
>diff --git a/suite/themes/modern/messenger/folderPane.css b/suite/themes/modern/messenger/folderPane.css
>diff --git a/suite/themes/modern/messenger/messageBody.css b/suite/themes/modern/messenger/messageBody.css
>diff --git a/suite/themes/modern/messenger/threadPane.css b/suite/themes/modern/messenger/threadPane.css

Comment 33

[[e-mail]]

The definitive answer is indeed text-align: right; for numbers (as I in fact seem to have already mentioned in comment #1) so that once the patch has been updated it should be ready. (Apparently in HTML you would also want to override the unicode bidi to plaintext but I don't think we have to worry about that here.)

Comment 34

[thanhluan07]

hello

Comment 35

[thanhluan07]

Hello, 
I would like to be assigned to this bug, is it possible ? Or it is already fixed??

Comment 36

[Philip Chee]

(In reply to thanhluan07 from comment #35)
> Hello, 
> I would like to be assigned to this bug, is it possible ? Or it is already
> fixed??
Hi! This bug hasn't been fixed. So it's available. However we normally don't formally assign GFBs until a patch has been submitted.

The first step for you then is to build SeaMonkey if you haven't already done so.
https://developer.mozilla.org/en-US/docs/Simple_SeaMonkey_build

Comment 37

[thanhluan07]

Hello 
I have a question for this bug. Actually , I built Seamonkey and I would like to have a exemple ( page web )to see the problem and test my changes.

Comment 38

[:Hb]

(In reply to thanhluan07 from comment #37)
> ...  I would like to have a example (web page)to see the problem and test my changes.

As far as I understood this bug is not about external web pages but about Seamonkeys internal CSS files.  Please see comment 27 for more.

When asking on IRC: irc://moznet/seamonkey only expect an answer within hours.  Most people seen there do other work while being logged in.

Comment 39

[Philip Chee]

(In reply to thanhluan07 from comment #37)
> Hello 
> I have a question for this bug. Actually , I built Seamonkey and I would
> like to have a exemple ( page web )to see the problem and test my changes.
This bug is about the SeaMonkey themes. If your patch works correctly then you should see *no* change in the UI - that's how you can tell it's working. Also you should install the Force RTL addon to emulate RTL locales. Your patch should also work correctly in RTL locales.

Comment 40

[thanhluan07]

Attachment: path1.diff

I have made some modifications in the "themes" files of SeaMonkey to solve the present bug (Bug 552486 - Use text-align: start or end instead of left or right in SeaMonkey themes). Here the path with the related changes.

Comment 41

[Philip Chee]

This looks good. Some things can be improved.
> -  text-align: right;
> +  text-align: end;

> +  /*text-align: right;*/
don't leave the original code commented out, just delete it. This is all you need:
> -  text-align: right;
> +  text-align: end;

> @@ -153,11 +154,13 @@
 
>  #collapseddateValue {
>    margin: 0 .5em;
> -  text-align: right;
> +  text-align: end;
> +  /*text-align: right;*/
To make reviews easier, please use eight lines of context. Currently you are using the default which is three.

> -DIST_FILES += ['install.rdf']
> -
This is part of some other bug.

Comment 42

[thanhluan07]

Attachment: patch2.diff

Thanks you for the review and for the recommendations. 
Here is the new path with the modifications that have been suggested in the precedent comment.

Comment 43

[Philip Chee]

Thank you for the new patch.

> -DIST_FILES += ['install.rdf']
> -
This hasn't been removed from your patch.
diff context still at 3 lines. Use  -U8 to increase context to 8 lines before and after.

Comment 44

[thanhluan07]

Attachment: patch4.diff

Here I update the path for the current bug: the diff context line have been changed from 3 lines (default value) to 8 lines (-U8). Thank you for review.

Comment 45

[thanhluan07]

Do you have any feedback / review about the last path submitted?

Comment 46

[Frank-Rainer Grahl (:frg)]

Comment on attachment 8609244 [details] [diff] [review]
patch4.diff

Clearing NI for no longer active contributors.