Bug 50951 – Add support for text-align:match-parent

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Xiaomei Ji]

Following is from the 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’.

[Jeremy Moskovich]

Created attachment 87070 [details]
Patch

[Eric Seidel (no email)]

This looks reasonable to me.

Two questions:
- Should this be -webkit-match-parent?  (I ask because I'm not sure what our policy is wrt to CSS3 additions.)
- Have other engins implemented this?  Do we match their behavior?  Do they have any tests we could import?  Does the CSS3 spec have any tests we could import?

I guess that was more than 2 questions.

Mostly I'm concerned about our testing coverage.  But this looks fine, honestly.

[Jeremy Moskovich]

Created attachment 87073 [details]
Patch

[Jeremy Moskovich]

Thanks Eric,

- I'm checking if there's consensus around this feature, if there is - should I land without the webkit prefix or should we land with the prefix and give it some bake time?

- I think we're the first to implement this and I wasn't able to find any tests, if anyone knows of any I'd be glad to add them.

[Eric Seidel (no email)]

Comment on attachment 87073 [details]
Patch

This looks fine.  We should probably change to -webkit-match-parent since this spec is not yet official yet.  I don't know how we make that decision though.

[Eric Seidel (no email)]

(In reply to comment #4)
> - I'm checking if there's consensus around this feature, if there is - should I land without the webkit prefix or should we land with the prefix and give it some bake time?

I'm not sure it matters.  It's easy to add or remove the prefix.  Landing with the prefix is probably "safer", but I've asked on webkit-dev to figure out what the official policy is. :)

> - I think we're the first to implement this and I wasn't able to find any tests, if anyone knows of any I'd be glad to add them.

OK.

[Eric Seidel (no email)]

Comment on attachment 87073 [details]
Patch

We should land this with -webkit-.  I'm happy to review said change, or you can just make the change before you land.

[Eric Seidel (no email)]

Jeremy just asked how an author would use -webkit-match-parent and work across many browsers.  I believe this is the answer:

div {
  text-align: -webkit-match-parent;
  text-align: -moz-match-parent;
  text-align: match-parent;
}

IIRC invalid values are ignored for CSS, so the above CSS should work across all iterations of our support across both WebKit and Mozilla.  Please anyone correct me if my recollection is incorrect.

[Jeremy Moskovich]

I've filed a Mozilla bug for this at https://bugzilla.mozilla.org/show_bug.cgi?id=645642

[Ryosuke Niwa]

I talked with Jeremy in person and we concluded that we need one more test case where match-parent is nested with different directionalities as in:
<div dir="rtl" style="text-align: start;"><div dir="ltr" style="text-align: match-parent;"><div style="text-align: match-parent">Right aligned</div></div></div>

[Jeremy Moskovich]

Created attachment 87115 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 87115 [details]
Patch

LGTM!  I look forward to seeing the <li> change!

[WebKit Commit Bot]

Comment on attachment 87115 [details]
Patch

Rejecting attachment 87115 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-8', 'apply-..." exit_code: 2

Last 500 characters of output:
bkit-match-parent.html
patching file Source/WebCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebCore/css/CSSParser.cpp
patching file Source/WebCore/css/CSSStyleSelector.cpp
Hunk #1 FAILED at 4626.
1 out of 1 hunk FAILED -- saving rejects to file Source/WebCore/css/CSSStyleSelector.cpp.rej
patching file Source/WebCore/css/CSSValueKeywords.in

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Eric Seidel', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/8217090

[Jeremy Moskovich]

Created attachment 87128 [details]
Patch

[Jeremy Moskovich]

Patch updated in the face of r82105 (http://trac.webkit.org/changeset/82105) only changes are to CSSStyleSelector.cpp.

[Eric Seidel (no email)]

Comment on attachment 87128 [details]
Patch

You can always use webkit-patch "land-safely" to have it re-upload and re cq+ the patch for you.  (Assuming you are a committer, which I believe you are).  No need for a second review.

[Ryosuke Niwa]

Comment on attachment 87128 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=87128&action=review

I'd like to see some tests that verify that the computed value of text-align property is as expected.

> Source/WebCore/css/CSSParser.cpp:791
> +        // left | right | center | justify | webkit_left | webkit_right | webkit_center | webkit_match_parent | start |
> +        // end | <string> | inherit

Nit: you should probably put both start and end on the same line.

[Jeremy Moskovich]

Created attachment 87132 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 87132 [details]
Patch for landing

Rejecting attachment 87132 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-8', 'build-..." exit_code: 2

Last 500 characters of output:
...................................................................................................................................................................................................................................................................
fast/css/text-align-webkit-match-parent.html -> failed

Exiting early after 1 failures. 6824 tests run.
160.31s total testing time

6823 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
4 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/8285092

[Jeremy Moskovich]

Created attachment 87308 [details]
Patch for landing

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 87308 [details]:

security/block-test.html bug 55741 (authors: [e-mail], [e-mail], and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 87308 [details]
Patch for landing

Clearing flags on attachment: 87308

Committed r82250: <http://trac.webkit.org/changeset/82250>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.