[FIX]add support for text-align: end

RESOLVED FIXED in mozilla1.9.2a1

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
12 years ago
8 years ago

People

(Reporter: Reuven Gonen, Assigned: bz)

Tracking

(Blocks: 1 bug, {dev-doc-complete, intl})

Trunk
mozilla1.9.2a1
x86
Windows XP
dev-doc-complete, intl
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

12 years ago
currently the attribute "text-align: start;" is supported, but attribute
"text-align: end;" isn't.
(Reporter)

Comment 1

12 years ago
Created attachment 188428 [details]
testcase
Assignee: dbaron → nobody
QA Contact: ian → style-system

Updated

10 years ago
Keywords: intl
Created attachment 360048 [details] [diff] [review]
Fix

The only worry here is whether this should be -moz-end...
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #360048 - Flags: superreview?(dbaron)
Attachment #360048 - Flags: review?(dbaron)
Summary: add support to text-align: end → [FIX]add support to text-align: end

Updated

9 years ago
Blocks: 348233
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
Attachment #360048 - Flags: superreview?(dbaron)
Attachment #360048 - Flags: superreview+
Attachment #360048 - Flags: review?(dbaron)
Attachment #360048 - Flags: review+
Actually, you should also update nsTextBoxFrame::CalcTextRect (for start too).

(It looks like nsCanvasRenderingContext2D already supports 'end'.)
... and maybe the mTextAlignment setting in nsTreeColumn::Invalidate (or code that uses it, but probably better earlier)
Summary: [FIX]add support to text-align: end → [FIX]add support for text-align: end
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.  :(
Attachment #360048 - Attachment is obsolete: true
Attachment #360734 - Flags: superreview?(dbaron)
Attachment #360734 - Flags: review?(ehsan.akhgari)
(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).
(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.
(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.  :)
Attachment #360734 - Flags: review?(ehsan.akhgari) → review+
Comment on attachment 360734 [details] [diff] [review]
Updated to comments

Forgot to mark this r+.  I think this is the correct thing to do.
> 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....
(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 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
Attachment #360734 - Flags: superreview?(dbaron)
Attachment #360734 - Flags: superreview+
Attachment #360734 - Flags: review+
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.
Depends on: 477113
Filed bug 477113 for that purpose.
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Er, and http://hg.mozilla.org/mozilla-central/rev/ff4937cb937d to point to the right reftest files.
Target Milestone: --- → mozilla1.9.2a1

Updated

9 years ago
Keywords: dev-doc-needed
Documented here: https://developer.mozilla.org/En/CSS/Text-align

Tagged as requiring Gecko 1.9.2.
Keywords: dev-doc-needed → dev-doc-complete

Updated

8 years ago
Blocks: 552486
You need to log in before you can comment on or make changes to this bug.