If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

[FIX]"text-align: start" doesn't work on <th>

RESOLVED FIXED in mozilla1.9.2a1

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: Ehsan, Assigned: bz)

Tracking

({testcase})

Trunk
mozilla1.9.2a1
testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Created attachment 359558 [details]
Test case

See the attached test case.  "text-align: start" doesn't work correctly on <th> elements, but <td> elements are fine.  DOM Inspector shows no relevant text-align CSS rule other than the "text-align: start", but the computed style is "center" instead of "start", and the presentation is centered likewise.

Updated

9 years ago
OS: Windows Vista → All
Hardware: x86 → All
We have a post-resolve callback for <th> (see TableTHRule in nsHTMLStyleSheet.cpp) that does this:

119       const nsStyleText* parentStyleText = parentContext->GetStyleText();
120       PRUint8 parentAlign = parentStyleText->mTextAlign;
121       text->mTextAlign = (NS_STYLE_TEXT_ALIGN_DEFAULT == parentAlign)
122                               ? NS_STYLE_TEXT_ALIGN_CENTER : parentAlign;

That means we center unless the parent has a non-default text-align, in which case we inherit the parent's text-align.

That's pretty wacky behavior in some ways....
This should be normal rule mapping rather than a post-resolve callback.
But shouldn't that code get executed if the text alignment of the <th> itself is default?  Does setting it to "start" lead to the same effect?
Oh, I just noted that NS_STYLE_TEXT_ALIGN_DEFAULT actually means "start"...

<http://mxr.mozilla.org/mozilla-central/source/layout/style/nsCSSProps.cpp#1078>

I think we want to check if the "text-align: start" rule has been applied on the <th> itself, or if it's simply chosen as the default style because no text-align rule has been present.
David, the problem is that we want the <th> to follow the parent alignment if it's explicitly specified.  For example:

  <tr align="left"><th/></tr>

Should left-align.  If that's not specified, we should center, or so this code seems to think.  I suppose we could restrict the following to the align attribute and not left-align in this case:

  <tr style="text-align: left"><th/></tr>

Is that what you meant in comment 2?
Or did you mean to just keep the current rule but make it take effect at the preshint level so that more specific rules will override?
(In reply to comment #6)
> Or did you mean to just keep the current rule but make it take effect at the
> preshint level so that more specific rules will override?

Yes.
So presumably we'll need a magic -moz value for text-align that the rule will produce, and then the rulenode code will treat it specially.

Ehsan, want to take a stab at that?  ;)
Why would we need a magic value?
Er, sorry, never mind, we would.
(In reply to comment #8)
> So presumably we'll need a magic -moz value for text-align that the rule will
> produce, and then the rulenode code will treat it specially.
> 
> Ehsan, want to take a stab at that?  ;)

Yeah, sure, but I'm not sure if I understand what needs to be done yet.  :(

Can you please give an easier to read explanation?  :)

Updated

9 years ago
Blocks: 348233
Created attachment 360044 [details] [diff] [review]
Fix

Explanation by demonstration.  Ehsan, feel free to mail me with any questions about how the patch works, ok?

David, let me know if you think we should add a parseable value here too and move the rule to html.css.
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #360044 - Flags: superreview?(dbaron)
Attachment #360044 - Flags: review?(dbaron)
Summary: "text-align: start" doesn't work on <th> → [FIX]"text-align: start" doesn't work on <th>
Attachment #360044 - Flags: superreview?(dbaron)
Attachment #360044 - Flags: superreview-
Attachment #360044 - Flags: review?(dbaron)
Attachment #360044 - Flags: review-
Comment on attachment 360044 [details] [diff] [review]
Fix

>+#define NS_STYLE_TEXT_ALIGN_MOZ_MAYBE_CENTER    9   // Only used in the back end

I'd be more explicit:  it's allowed only in rule data structs; never present in style sheets or in computed data.

Maybe call the new value CENTER_OR_INHERIT instead of MAYBE_CENTER?

>diff --git a/layout/style/nsHTMLStyleSheet.cpp b/layout/style/nsHTMLStyleSheet.cpp

Remove the now-unused PostResolveCallback function.

>diff --git a/layout/style/nsRuleNode.cpp b/layout/style/nsRuleNode.cpp

You need to add a CheckTextCallback (see CheckColorCallback), and also a testcase testing what breaks with the current patch for lack of it.  (It breaks in the conditions under which COMPUTE_START_INHERITED sets parentdata_ = data_.)
I'm not sure I follow.  If parentdata_ = data_, then parentText->mTextAlign will be NS_STYLE_TEXT_ALING_DEFAULT and we'll end up centering, as we should, no?

Or can we get there in the aStartStruct case?  Or is something else going to go wrong?
We can get to parentData == data in the aRuleDetail == eRuleFullReset case.  In other words, if you specify every property in the text struct, the inheritance aspect of this will suddenly break.
Created attachment 360746 [details] [diff] [review]
Updated to the other comments, for now
Attachment #360044 - Attachment is obsolete: true
Created attachment 360853 [details] [diff] [review]
Updated to all comments, including test for the case that was broken with the first patch
Attachment #360746 - Attachment is obsolete: true
Attachment #360853 - Flags: superreview?(dbaron)
Attachment #360853 - Flags: review?(dbaron)
Attachment #360853 - Flags: superreview?(dbaron)
Attachment #360853 - Flags: superreview+
Attachment #360853 - Flags: review?(dbaron)
Attachment #360853 - Flags: review+
Comment on attachment 360853 [details] [diff] [review]
Updated to all comments, including test for the case that was broken with the first patch

>+== 475986-1a.html 475986-1-ref.html
>+== 475986-1b.html 475986-1-ref.html
>+== 475986-1c.html 475986-1-ref.html
>+== 475986-1d.html 475986-1-ref.html
>+== 475986-1e.html 475986-1-ref.html
>+== 475986-1f.html 475986-1-ref.html
>+== 475986-2a.html 475986-2-ref.html
>+== 475986-2b.html 475986-2-ref.html
>+== 475986-2c.html 475986-2-ref.html
>+== 475986-2d.html 475986-2-ref.html
>+== 475986-2e.html 475986-2-ref.html
>+== 475986-2f.html 475986-2-ref.html
>+== 475986-3a.html 475986-3-ref.html
>+== 475986-3b.html 475986-3-ref.html
>+== 475986-4.html 475986-4-ref.html

Maybe add some != tests between the various (1,2,3) references?

r+sr=dbaron
Did that (with the text all changed to say "Text" so the comparison is meaningful) and pushed http://hg.mozilla.org/mozilla-central/rev/5ab823ef8518
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Do we want this for the branch as well?
Target Milestone: --- → mozilla1.9.2a1
You mean for 1.9.1?  I suppose we could do that; I'd be a tiny smidgeon worried about compat issues arising, but if we land it in b3...

Do you want to take your various RTL fixes that depend on this on the 1.9.1 branch?
(In reply to comment #21)
> You mean for 1.9.1?  I suppose we could do that; I'd be a tiny smidgeon worried
> about compat issues arising, but if we land it in b3...

I don't think it would make it for Beta 3 (I already have quite a few patches waiting for approval for 1.9.1, but I guess the drivers team is only taking blockers for beta 3 at this stage).

> Do you want to take your various RTL fixes that depend on this on the 1.9.1
> branch?

Yes, definitely.  But it's only one fix (bug 348233), and we can take the current version of the patch on that bug which works around this issue for 1.9.1, so bug 348233 does not depend on this bug in order to land on 1.9.1.  (Bug 348233 also depnds on bug 299837 which also won't land for 1.9.1, so maybe that's what we should do anyway.)
You need to log in before you can comment on or make changes to this bug.