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

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[(no longer active)]

Attachment: 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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

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....

Comment 2

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

This should be normal rule mapping rather than a post-resolve callback.

Comment 3

[(no longer active)]

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?

Comment 4

[(no longer active)]

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.

Comment 5

[Boris Zbarsky [:bzbarsky]]

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?

Comment 6

[Boris Zbarsky [:bzbarsky]]

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?

Comment 7

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

(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.

Comment 8

[Boris Zbarsky [:bzbarsky]]

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?  ;)

Comment 9

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

Why would we need a magic value?

Comment 10

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

Er, sorry, never mind, we would.

Comment 11

[(no longer active)]

(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?  :)

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: 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.

Comment 13

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

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_.)

Comment 14

[Boris Zbarsky [:bzbarsky]]

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?

Comment 15

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

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.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to the other comments, for now

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to all comments, including test for the case that was broken with the first patch

Comment 18

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

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

Comment 19

[Boris Zbarsky [:bzbarsky]]

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

Comment 20

[(no longer active)]

Do we want this for the branch as well?

Comment 21

[Boris Zbarsky [:bzbarsky]]

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?

Comment 22

[(no longer active)]

(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.)