Bug 50951

Created attachment 87070 [details] Patch

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.

Created attachment 87073 [details] Patch

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

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

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.

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

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>

Created attachment 87115 [details] Patch

Comment on attachment 87115 [details] Patch LGTM! I look forward to seeing the <li> change!

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

Created attachment 87128 [details] Patch

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

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

Created attachment 87132 [details] Patch for landing

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

Created attachment 87308 [details] Patch for landing

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 on attachment 87308 [details] Patch for landing Clearing flags on attachment: 87308 Committed r82250: <http://trac.webkit.org/changeset/82250>

All reviewed patches have been landed. Closing bug.