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.
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? :)
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.
9 years ago
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
Created attachment 360853 [details] [diff] [review] Updated to all comments, including test for the case that was broken with the first patch
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
Do we want this for the branch as well?
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.)