Closed Bug 645642 Opened 13 years ago Closed 9 years ago

Add support for text-align:match-parent

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: playmobil, Assigned: smontagu, NeedInfo)

References

(Blocks 2 open bugs, )

Details

(Keywords: dev-doc-complete)

Attachments

(3 files, 2 obsolete files)

User-Agent:       Mozilla/5.0 (Macintosh; Intel Mac OS X 10_6_6) AppleWebKit/534.27 (KHTML, like Gecko) Chrome/12.0.712.0 Safari/534.27
Build Identifier: 

text-align:match-parent is part of the CSS3 spec.

"""
match-parent
This value behaves the same as ‘inherit’ except that an inherited value of ‘start’ or ‘end’ is calculated against its parent's ‘direction’ value and results in a computed value of either ‘left’ or ‘right’.
"""

WebKit is tracking implementation in https://bugs.webkit.org/show_bug.cgi?id=50951

Reproducible: Always
> text-align:match-parent is part of the CSS3 spec.

More precisely, it's an early proposal present in a CSS3 editor's draft.  There's a huge difference.  ;)

David, do we want to do this?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Blocks: html5bidi
(In reply to Boris Zbarsky (:bz) from comment #1)
> > text-align:match-parent is part of the CSS3 spec.
> 
> More precisely, it's an early proposal present in a CSS3 editor's draft. 
> There's a huge difference.  ;)
> 
> David, do we want to do this?

To update, it's in the current CSS Text Level 3 Working Draft (http://www.w3.org/TR/2011/WD-css3-text-20110901/), and has been in the Editor's Drafts for over a year now (http://www.w3.org/TR/2010/WD-css3-text-20101005/). I am not sure it can be called an early proposal anymore.

The idea behind match-parent is that one sometimes wants to set an element's dir without affecting its alignment (since the content needs to work well visually with other "fields"). There is currently no way to do that without knowing the parent's computed text-align and, if it is "start" or "end", its computed direction. The new value thus makes it easier to support both LTR and RTL versions of a page.
Oh, and it's been implemented in WebKit.
Blocks: 762270
No longer blocks: 762270
Blocks: css3test
CSS Text Level 3 is in last call, and match-parent is still there with no changes.
Blocks: 1152309
Attached patch Patch v.0 (obsolete) — Splinter Review
I have had a patch for this sitting in my queue for some time, but never got around to writing tests for it. Since we see in bug 1152309 that sites are beginning to depend on it to some extent, let's move forward with this.
Assignee: nobody → smontagu
Attachment #8591758 - Attachment is obsolete: true
Attachment #8599537 - Flags: review?(dbaron)
Attachment #8599539 - Flags: review?(dbaron)
Attachment #8599538 - Attachment is obsolete: true
Attachment #8599538 - Flags: review?(dbaron)
Attachment #8599542 - Flags: review?(dbaron)
Comment on attachment 8599537 [details] [diff] [review]
Patch v.1: implement text-align: -moz-match-parent

This should not be prefixed.  Please just implement unprefixed.


>+  } else if (eCSSUnit_Enumerated == textAlignValue->GetUnit() &&
>+             NS_STYLE_TEXT_ALIGN_MATCH_PARENT ==
>+             textAlignValue->GetIntValue()) {

Please indent the third of these lines by 2 more spaces.

>+    uint8_t parentDirection =
>+      aContext->GetParent()->StyleVisibility()->mDirection;

This will crash when 'match-parent' is used on the root element.

Given the literal wording in the spec, 'match-parent' on the root
should compute to 'start' (the initial value) since inherit uses
the initial value, and there is no "inherited 'start' or 'end' keyword".

I think you should just special-case that by checking aContext->GetParent(),
(and putting all of the contents of the "else if" in the opposite branch
except for the canStoreInRuleTree = false.

Please add a test for that case as well.

r=dbaron with that
Attachment #8599537 - Flags: review?(dbaron) → review+
Comment on attachment 8599542 [details] [diff] [review]
Patch 2: use text-align: -moz-match-parent in <li> and <input type="file">

> /* Try to make RTL <input type='file'> look nicer. */
>-/* TODO: use text-align: match-parent when bug 645642 is fixed. */
> input[type="file"]:-moz-dir(rtl) > xul|label {
>   -moz-padding-start: 0px;
>   -moz-padding-end: 5px;
>-  text-align: right;

Should these just be padding-left and padding-right so you don't need this override?  (Or does this actually work for vertical?)  Or is there some better fix?  It seems like there should probably be a TODO pointing to a bug number on getting rid of this selector in the UA style sheet.
> li {
>   display: list-item;
>+  text-align: -moz-match-parent;
> }

Should we also do the same for option, as http://dev.w3.org/csswg/css-text/#default-stylesheet suggests?

(If not, should we raise an issue on the spec.)

r=dbaron with that
Attachment #8599542 - Flags: review?(dbaron) → review+
Comment on attachment 8599539 [details] [diff] [review]
Patch 3: reftests

Seems a little more like testing things that are equivalent if test 04 sets width to "" instead of "100%", perhaps?

r=dbaron
Attachment #8599539 - Flags: review?(dbaron) → review+
Comment on attachment 8599539 [details] [diff] [review]
Patch 3: reftests

Also, maybe the tests could go in layout/reftests/w3c-css/submitted/text-3, with appropriate CSS test suite metadata?  (If so, don't forget to add the reftest.list to its parent directory's manifest.)
(In reply to David Baron [:dbaron] ⏰UTC+2 from comment #11)
> > /* Try to make RTL <input type='file'> look nicer. */
> >-/* TODO: use text-align: match-parent when bug 645642 is fixed. */
> > input[type="file"]:-moz-dir(rtl) > xul|label {
> >   -moz-padding-start: 0px;
> >   -moz-padding-end: 5px;
> >-  text-align: right;
> 
> Should these just be padding-left and padding-right so you don't need this
> override?  (Or does this actually work for vertical?)  Or is there some
> better fix?  It seems like there should probably be a TODO pointing to a bug
> number on getting rid of this selector in the UA style sheet.

I had the same thought about padding-left and padding-right, but it doesn't work out: what we really need is something like padding-start-match-parent, because the label itself is always ltr, but we want padding-left or right depending on the parent direction.

A better fix these days might be to use unicode-bidi: plaintext instead of direction: ltr to force direction on the label -- I've filed bug 1161482 and I'll add the TODO. IIANM, in that case we wouldn't need the text-align: match-parent at all.

> Should we also do the same for option, as
> http://dev.w3.org/csswg/css-text/#default-stylesheet suggests?

Yes, good catch (that was added in CSS after my original patch and I didn't check if it had changed).
(In reply to David Baron [:dbaron] ⏰UTC+2 (busy, returning May 21) from comment #13)
> Comment on attachment 8599539 [details] [diff] [review]
> Patch 3: reftests
> 
> Also, maybe the tests could go in layout/reftests/w3c-css/submitted/text-3,
> with appropriate CSS test suite metadata?  (If so, don't forget to add the
> reftest.list to its parent directory's manifest.)

https://hg.mozilla.org/integration/mozilla-inbound/rev/57c740d1586a
Keywords: dev-doc-needed
Hmmm.  With use of onload rather than MozReftestInvalidate, maybe you should have something that causes a layout flush before the script runs, to ensure it's actually testing change handling?
Flags: needinfo?(smontagu)
(In reply to David Baron :dbaron: ⌚️UTC-4 from comment #10)
> >+    uint8_t parentDirection =
> >+      aContext->GetParent()->StyleVisibility()->mDirection;
> 
> This will crash when 'match-parent' is used on the root element.
> 
> Given the literal wording in the spec, 'match-parent' on the root
> should compute to 'start' (the initial value) since inherit uses
> the initial value, and there is no "inherited 'start' or 'end' keyword".
> 
> I think you should just special-case that by checking aContext->GetParent(),
> (and putting all of the contents of the "else if" in the opposite branch
> except for the canStoreInRuleTree = false.
> 
> Please add a test for that case as well.

So this review comment wasn't addressed correctly, since no else branch for the if (parent) was added to actually make 'match-parent' on the root be 'start'.  This violates a basic invariant of Compute*Data functions, and can lead to bugs if an aStartStruct is provided (which, for the root, could only happen on dynamic changes).

(This based on a discussion I had with Manish in #layout about implementing match-parent in Servo.)
You need to log in before you can comment on or make changes to this bug.