Bug 79914

Summary: The initial value of text-align should be start instead of -webkit-auto
Product: WebKit Reporter: Aharon (Vladimir) Lanin <aharon>
Component: CSSAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: atwilson, cmarcelo, darin, dglazkov, enrica, eric, gustavo, leviw, macpherson, menard, mitz, philn, playmobil, rniwa, shezbaig.wk, tkent, tony, webkit.review.bot, xan.lopez, xji
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 50910    
Attachments:
Description Flags
Patch
none
Fixed per Tony's comments
none
Archive of layout-test-results from ec2-cr-linux-02
none
Patch tony: review+

Description Aharon (Vladimir) Lanin 2012-02-29 07:52:45 PST
When used under an element with an explicit text-align:start (or text-align:end), text-align:match-parent works properly. However, it does not seem to have any effect when no ancestor of the element has an explicit text-align value.

Thus, data:text/html,<div dir=ltr style="text-align:start">a!<div dir=rtl style="text-align:-webkit-match-parent">b!</div></div> works properly.
But data:text/html,<div dir=ltr>a!<div dir=rtl style="text-align:-webkit-match-parent">b!</div></div> does not.
Comment 1 Ryosuke Niwa 2012-06-13 16:13:58 PDT
I think the real bug here is the default value of text-align is "auto" instead of "start" in WebKit:
http://www.w3.org/TR/css3-text/#text-align0
Comment 2 Ryosuke Niwa 2012-06-14 14:58:48 PDT
Created attachment 147658 [details]
Patch
Comment 3 Tony Chang 2012-06-14 16:09:12 PDT
Comment on attachment 147658 [details]
Patch

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

r- because GTK compile failure.

> Source/WebCore/css/StyleResolver.cpp:2065
> +        // FIXME: We shouldn't be overriding start/-webkit-auto like this.

Maybe mention editing html.css here?

> Source/WebCore/editing/EditingStyle.cpp:913
> +    if (textAlignResolvingStartAndEnd(styleAtPosition) == textAlignResolvingStartAndEnd(styleAtPosition))

This will always be true.

> Source/WebCore/rendering/RenderRubyText.cpp:58
> +    // FIXME: This check is bogus since user can set the initial value.
> +    if (textAlign != RenderStyle::initialTextAlign())

Should we file a bug for this?

> LayoutTests/fast/css/text-align-webkit-auto.html:5
>          window.layoutTestController.dumpAsText();

We might want to rename this test to text-align-initial.html or text-align-start.html.
Comment 4 Ryosuke Niwa 2012-06-14 17:04:16 PDT
Created attachment 147680 [details]
Fixed per Tony's comments
Comment 5 WebKit Review Bot 2012-06-15 02:13:01 PDT
Comment on attachment 147680 [details]
Fixed per Tony's comments

Attachment 147680 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12952851

New failing tests:
fast/lists/001-vertical.html
editing/pasteboard/data-transfer-items.html
fast/lists/001.html
fast/css/list-item-text-align.html
editing/pasteboard/onpaste-text-html.html
Comment 6 WebKit Review Bot 2012-06-15 02:13:07 PDT
Created attachment 147774 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 7 Tony Chang 2012-06-15 11:38:00 PDT
Comment on attachment 147680 [details]
Fixed per Tony's comments

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

The test failures look related.

> Source/WebCore/editing/EditingStyle.cpp:377
> +template<typename T>
> +static int textAlignResolvingStartAndEnd(T* style)

Using a template is cute, but wouldn't it be less generated code to just pass in the results of getIdentifierValue(style, CSSPropertyDirection) and getIdentifierValue(style, CSSPropertyTextAlign) as parameters?
Comment 8 Ryosuke Niwa 2012-06-15 12:02:26 PDT
(In reply to comment #7)
> (From update of attachment 147680 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=147680&action=review
> 
> The test failures look related.

Yeah, we just need to rebaseline them except list-item-text-align.html. My change to that file had gotten lost somehow :(

> > Source/WebCore/editing/EditingStyle.cpp:377
> > +template<typename T>
> > +static int textAlignResolvingStartAndEnd(T* style)
> 
> Using a template is cute, but wouldn't it be less generated code to just pass in the results of getIdentifierValue(style, CSSPropertyDirection) and getIdentifierValue(style, CSSPropertyTextAlign) as parameters?

Will do.
Comment 9 Ryosuke Niwa 2012-06-15 14:05:57 PDT
Created attachment 147895 [details]
Patch
Comment 10 Ryosuke Niwa 2012-06-15 14:54:53 PDT
Committed r120495: <http://trac.webkit.org/changeset/120495>
Comment 11 Andrew Wilson 2012-06-15 18:07:46 PDT
It looks like this may have broken fast/lists/001-vertical.html and  fast/lists/001.html:

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&tests=fast%2Flists%2F001-vertical.html%2Cfast%2Flists%2F001.html
Comment 12 Andrew Wilson 2012-06-15 18:10:15 PDT
My error - only rebaselining is required.
Comment 13 Aharon (Vladimir) Lanin 2012-06-17 00:18:13 PDT
Is this bug supposed to be fixed in Chrome 21.0.1177.0 canary? The original test case (data:text/html,<div dir=ltr>a!<div dir=rtl style="text-align:-webkit-match-parent">b!</div></div>) still does not work.
Comment 14 Ryosuke Niwa 2012-06-17 12:26:14 PDT
(In reply to comment #13)
> Is this bug supposed to be fixed in Chrome 21.0.1177.0 canary? The original test case (data:text/html,<div dir=ltr>a!<div dir=rtl style="text-align:-webkit-match-parent">b!</div></div>) still does not work.

It works on the nightly build of WebKit for me. Wait one more week & check it again or test it using nightly builds: http://nightly.webkit.org/
Comment 15 Glenn Adams 2012-10-02 01:28:27 PDT
*** Bug 48803 has been marked as a duplicate of this bug. ***