299837 - [FIX]add support for text-align: end

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Reuven Gonen]

currently the attribute "text-align: start;" is supported, but attribute
"text-align: end;" isn't.

Comment 1

[Reuven Gonen]

Attachment: testcase

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix

The only worry here is whether this should be -moz-end...

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 360048 [details] [diff] [review]
Fix

> // Note: make sure the numbers are less than the numbers that start
> // the vertical_align values below!

Maybe slide this comment to the end of the list (or duplicate it)?  I may well have missed it.

Please add the new value to layout/style/test/property_database.js.

I think we're ok without -moz- given http://www.w3.org/TR/2008/REC-SVGTiny12-20081222/text.html#TextAlignProperty

r+sr=dbaron

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Actually, you should also update nsTextBoxFrame::CalcTextRect (for start too).

(It looks like nsCanvasRenderingContext2D already supports 'end'.)

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

... and maybe the mTextAlignment setting in nsTreeColumn::Invalidate (or code that uses it, but probably better earlier)

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to comments

This updates labels and treecols to work correctly.  This means that in rtl they will no longer treat "right" as meaning "end", and labels in rtl will no longer treat "left" as meaning "start".  So this might break some existing rtl themes... On the other hand, "start" and "end" will now work correctly in both ltr and rtl, and this makes these two consumers behave like everything else does.  Ehsan, dos that seem reasonable?

Sorry I missed them; I'd searched for TEXT_ALIGN_DEFAULT and neither callsite mentions it.  :(

Comment 7

[(no longer active)]

(In reply to comment #6)
> This updates labels and treecols to work correctly.  This means that in rtl
> they will no longer treat "right" as meaning "end", and labels in rtl will no
> longer treat "left" as meaning "start".  So this might break some existing rtl
> themes... On the other hand, "start" and "end" will now work correctly in both
> ltr and rtl, and this makes these two consumers behave like everything else
> does.  Ehsan, dos that seem reasonable?

I'm sure some parts of the theme rely on this broken behavior, so we might need to update them in this patch.  I think the correct thing to do is to take your fix to correct the meaning of left/right in RTL.  I tried building a fa version of the browser with your patch, but it seems like it's relying on a newer version of a patch for bug 475986 to apply.  If you can send me that version of the patch, I'll be happy to test how this works in RTL builds.  Anyway we have to fix all the places in themes which this broken behavior is used (which I think will not be too many).

Comment 8

[Dão Gottwald [:dao]]

(In reply to comment #7)
> I'm sure some parts of the theme rely on this broken behavior, so we might need
> to update them in this patch.

Doesn't have to be /in this patch/, fwiw. Could be done in a new bug.

Comment 9

[(no longer active)]

(In reply to comment #8)
> Doesn't have to be /in this patch/, fwiw. Could be done in a new bug.

I think that depends on the amount of the theme that it's going to break for RTL locales.  But then again RTL users have been used to broken stuff in their themes for a long time.  :(

And whether or not we do this in this bug or a new one, I'm volunteering for the theme fixes.  :)

Comment 10

[(no longer active)]

Comment on attachment 360734 [details] [diff] [review]
Updated to comments

Forgot to mark this r+.  I think this is the correct thing to do.

Comment 11

[Boris Zbarsky [:bzbarsky]]

> but it seems like it's relying on a newer version of a patch for bug 475986 to
> apply.

Oh, right.  I uploaded that version in that bug.

If you're willing to do the theme fixes, that would rock.  And at least going forward the behavior will be sane....

Comment 12

[Dão Gottwald [:dao]]

(In reply to comment #9)
> I think that depends on the amount of the theme that it's going to break for
> RTL locales.  But then again RTL users have been used to broken stuff in their
> themes for a long time.  :(

It seems that this shouldn't land on branch anyway.
Otherwise we're pre-alpha, and things don't break in a way that would make the browser unusable, so I'd say that's ok. As long as we file that bug and don't forget about it...

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 360734 [details] [diff] [review]
Updated to comments

>@@ -313,19 +313,22 @@ nsTreeColumn::Invalidate()
>   mTextAlignment = textStyle->mTextAlign;
>-  if (mTextAlignment == 0 || mTextAlignment == 2) { // Left or Right
>-    if (vis->mDirection == NS_STYLE_DIRECTION_RTL)
>-      mTextAlignment = 2 - mTextAlignment; // Right becomes left, left becomes right.
>+  // DEFAULT or END alignment sometimes means RIGHT
>+  if ((mTextAlignment == NS_STYLE_TEXT_ALIGN_DEFAULT &&
>+       vis->mDirection == NS_STYLE_DIRECTION_RTL) ||
>+      (mTextAlignment == NS_STYLE_TEXT_ALIGN_END &&
>+       vis->mDirection == NS_STYLE_DIRECTION_LTR)) {
>+    mTextAlignment = NS_STYLE_TEXT_ALIGN_RIGHT;
>   }

This should also assign NS_STYLE_TEXT_ALIGN_LEFT for the other cases where textStyle->mTextAlign is ..._DEFAULT or ..._END, so that you don't have DEFAULT/END values in mTextAlignment at all.

r+sr=dbaron with that

Comment 14

[(no longer active)]

A very thorough look through the RTL UI with this patch, and I couldn't even spot a single place where this behavior change has broken the theme!  There is of course a number of places for me to check more closely, but I think it would be better off in a new bug, given that it doesn't have any obvious bad effects on RTL UI.

Comment 15

[(no longer active)]

Filed bug 477113 for that purpose.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/d8132c7bad92 and then http://hg.mozilla.org/mozilla-central/rev/7073e785f659 to address the review comment from comment 13.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Er, and http://hg.mozilla.org/mozilla-central/rev/ff4937cb937d to point to the right reftest files.

Comment 18

[Eric Shepherd [:sheppy]]

Documented here: https://developer.mozilla.org/En/CSS/Text-align

Tagged as requiring Gecko 1.9.2.