Bug 50951

Summary: Add support for text-align:match-parent
Product: WebKit Reporter: Xiaomei Ji <xji>
Component: CSSAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: aharon, commit-queue, eric, mitz, playmobil, rniwa
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
URL: http://dev.w3.org/csswg/css3-text/#text-align
Bug Depends on:    
Bug Blocks: 50910    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch for landing
none
Patch for landing playmobil: commit-queue+

Description Xiaomei Ji 2010-12-13 12:05:42 PST
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’.
Comment 1 Jeremy Moskovich 2011-03-27 08:13:09 PDT
Created attachment 87070 [details]
Patch
Comment 2 Eric Seidel (no email) 2011-03-27 08:22:26 PDT
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.
Comment 3 Jeremy Moskovich 2011-03-27 08:58:57 PDT
Created attachment 87073 [details]
Patch
Comment 4 Jeremy Moskovich 2011-03-27 09:03:33 PDT
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.
Comment 5 Eric Seidel (no email) 2011-03-27 09:03:54 PDT
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.
Comment 6 Eric Seidel (no email) 2011-03-27 09:08:17 PDT
(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.
Comment 7 Eric Seidel (no email) 2011-03-27 23:02:39 PDT
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.
Comment 8 Eric Seidel (no email) 2011-03-28 01:02:18 PDT
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.
Comment 9 Jeremy Moskovich 2011-03-28 01:15:00 PDT
I've filed a Mozilla bug for this at https://bugzilla.mozilla.org/show_bug.cgi?id=645642
Comment 10 Ryosuke Niwa 2011-03-28 01:40:03 PDT
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>
Comment 11 Jeremy Moskovich 2011-03-28 01:57:47 PDT
Created attachment 87115 [details]
Patch
Comment 12 Eric Seidel (no email) 2011-03-28 02:09:34 PDT
Comment on attachment 87115 [details]
Patch

LGTM!  I look forward to seeing the <li> change!
Comment 13 WebKit Commit Bot 2011-03-28 05:17:48 PDT
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
Comment 14 Jeremy Moskovich 2011-03-28 05:33:46 PDT
Created attachment 87128 [details]
Patch
Comment 15 Jeremy Moskovich 2011-03-28 05:34:30 PDT
Patch updated in the face of r82105 (http://trac.webkit.org/changeset/82105) only changes are to CSSStyleSelector.cpp.
Comment 16 Eric Seidel (no email) 2011-03-28 05:36:33 PDT
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.
Comment 17 Ryosuke Niwa 2011-03-28 05:39:25 PDT
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.
Comment 18 Jeremy Moskovich 2011-03-28 06:10:18 PDT
Created attachment 87132 [details]
Patch for landing
Comment 19 WebKit Commit Bot 2011-03-28 23:57:53 PDT
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
Comment 20 Jeremy Moskovich 2011-03-29 07:00:16 PDT
Created attachment 87308 [details]
Patch for landing
Comment 21 WebKit Commit Bot 2011-03-29 07:39:12 PDT
The commit-queue encountered the following flaky tests while processing attachment 87308 [details]:

security/block-test.html bug 55741 (authors: beidson@apple.com, mrowe@apple.com, and sam@webkit.org)
The commit-queue is continuing to process your patch.
Comment 22 WebKit Commit Bot 2011-03-29 07:41:18 PDT
Comment on attachment 87308 [details]
Patch for landing

Clearing flags on attachment: 87308

Committed r82250: <http://trac.webkit.org/changeset/82250>
Comment 23 WebKit Commit Bot 2011-03-29 07:41:24 PDT
All reviewed patches have been landed.  Closing bug.