645642 - Add support for text-align:match-parent

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[playmobil]

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

Comment 1

[Boris Zbarsky [:bzbarsky]]

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

Comment 2

[Aharon (Vladimir) Lanin]

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

Comment 3

[Aharon (Vladimir) Lanin]

Oh, and it's been implemented in WebKit.

Comment 4

[Aharon (Vladimir) Lanin]

CSS Text Level 3 is in last call, and match-parent is still there with no changes.

Comment 5

[Simon Montagu :smontagu]

Attachment: Patch v.0

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.

Comment 6

[Simon Montagu :smontagu]

Attachment: Patch v.1: implement text-align: -moz-match-parent

Comment 7

[Simon Montagu :smontagu]

Attachment: Patch 2: use text-align: -moz-match-parent in <li> and <input type="file">

Comment 8

[Simon Montagu :smontagu]

Attachment: Patch 3: reftests

Comment 9

[Simon Montagu :smontagu]

Attachment: Patch 2: use text-align: -moz-match-parent in <li> and <input type="file">

Comment 10

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

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

Comment 11

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

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

Comment 12

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

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

Comment 13

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

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

Comment 14

[Simon Montagu :smontagu]

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

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a28f0621cf7e
https://hg.mozilla.org/integration/mozilla-inbound/rev/956a03448bbe
https://hg.mozilla.org/integration/mozilla-inbound/rev/6267c4d1de60

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a28f0621cf7e
https://hg.mozilla.org/mozilla-central/rev/956a03448bbe
https://hg.mozilla.org/mozilla-central/rev/6267c4d1de60

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/57c740d1586a

Comment 18

[Simon Montagu :smontagu]

(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

Comment 19

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

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?

Comment 20

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/57c740d1586a

Comment 21

[Jean-Yves Perrier [:teoli]]

Updated:
https://developer.mozilla.org/en-US/docs/Web/CSS/text-align
and
https://developer.mozilla.org/en-US/Firefox/Releases/40#CSS

Comment 22

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

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

Comment 23

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

(conversation with Manish related to bug 1341714 comment 25.)