Last Comment Bug 299837 - [FIX]add support for text-align: end
: [FIX]add support for text-align: end
Status: RESOLVED FIXED
: dev-doc-complete, intl
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: x86 Windows XP
: -- normal with 1 vote (vote)
: mozilla1.9.2a1
Assigned To: Boris Zbarsky [:bz] (Out June 25-July 6)
:
Mentors:
Depends on: 477113
Blocks: 552486 348233
  Show dependency treegraph
 
Reported: 2005-07-06 07:50 PDT by Reuven Gonen
Modified: 2010-03-15 12:54 PDT (History)
10 users (show)
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (280 bytes, text/html)
2005-07-06 08:29 PDT, Reuven Gonen
no flags Details
Fix (8.19 KB, patch)
2009-02-01 21:32 PST, Boris Zbarsky [:bz] (Out June 25-July 6)
dbaron: review+
dbaron: superreview+
Details | Diff | Review
Updated to comments (19.10 KB, patch)
2009-02-05 08:43 PST, Boris Zbarsky [:bz] (Out June 25-July 6)
ehsan: review+
dbaron: review+
dbaron: superreview+
Details | Diff | Review

Description Reuven Gonen 2005-07-06 07:50:18 PDT
currently the attribute "text-align: start;" is supported, but attribute
"text-align: end;" isn't.
Comment 1 Reuven Gonen 2005-07-06 08:29:43 PDT
Created attachment 188428 [details]
testcase
Comment 2 Boris Zbarsky [:bz] (Out June 25-July 6) 2009-02-01 21:32:07 PST
Created attachment 360048 [details] [diff] [review]
Fix

The only worry here is whether this should be -moz-end...
Comment 3 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2009-02-02 14:50:10 PST
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-7 (review requests must explain patch) 2009-02-02 14:54:53 PST
Actually, you should also update nsTextBoxFrame::CalcTextRect (for start too).

(It looks like nsCanvasRenderingContext2D already supports 'end'.)
Comment 5 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2009-02-02 14:57:17 PST
... and maybe the mTextAlignment setting in nsTreeColumn::Invalidate (or code that uses it, but probably better earlier)
Comment 6 Boris Zbarsky [:bz] (Out June 25-July 6) 2009-02-05 08:43:00 PST
Created attachment 360734 [details] [diff] [review]
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 :Ehsan Akhgari (busy, don't ask for review please) 2009-02-05 09:21:34 PST
(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] 2009-02-05 09:28:53 PST
(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 :Ehsan Akhgari (busy, don't ask for review please) 2009-02-05 09:35:42 PST
(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 :Ehsan Akhgari (busy, don't ask for review please) 2009-02-05 09:38:55 PST
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 [:bz] (Out June 25-July 6) 2009-02-05 09:49:33 PST
> 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] 2009-02-05 10:04:03 PST
(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-7 (review requests must explain patch) 2009-02-05 10:31:33 PST
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 :Ehsan Akhgari (busy, don't ask for review please) 2009-02-05 13:26:46 PST
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 :Ehsan Akhgari (busy, don't ask for review please) 2009-02-05 13:29:40 PST
Filed bug 477113 for that purpose.
Comment 16 Boris Zbarsky [:bz] (Out June 25-July 6) 2009-02-05 19:53:39 PST
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 [:bz] (Out June 25-July 6) 2009-02-05 22:21:11 PST
Er, and http://hg.mozilla.org/mozilla-central/rev/ff4937cb937d to point to the right reftest files.
Comment 18 Eric Shepherd [:sheppy] 2009-02-11 12:43:08 PST
Documented here: https://developer.mozilla.org/En/CSS/Text-align

Tagged as requiring Gecko 1.9.2.

Note You need to log in before you can comment on or make changes to this bug.