Bug 44915 - Need to handle CSSValueAuto in CSSPrimitiveValue::operator ETextAlign()
Summary: Need to handle CSSValueAuto in CSSPrimitiveValue::operator ETextAlign()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Justin Garcia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-08-30 17:50 PDT by Justin Garcia
Modified: 2010-08-31 23:14 PDT (History)
3 users (show)

See Also:


Attachments
patch (1.78 KB, patch)
2010-08-30 18:33 PDT, Justin Garcia
darin: review-
Details | Formatted Diff | Diff
patch (2.97 KB, patch)
2010-08-31 19:47 PDT, Justin Garcia
mitz: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Justin Garcia 2010-08-30 17:50:11 PDT
When we map a CSSPrimitiveValue to an ETextAlign value, we need to handle CSSValueAuto, since we map TAAUTO to CSSValueAuto when going in the opposite direction in CSSPrimitiveValue::CSSPrimitiveValue(ETextAlign e).
Comment 1 mitz 2010-08-30 18:21:39 PDT
'auto' is not a vaild text-align value. We use TAAUTO internally for the “nameless value that acts as 'left' if 'direction' is 'ltr', 'right' if 'direction' is 'rtl'” from CSS2.1.

I’m just not sure where you’re going with this :)
Comment 2 Justin Garcia 2010-08-30 18:33:38 PDT
Created attachment 65994 [details]
patch
Comment 3 Darin Adler 2010-08-30 22:32:30 PDT
Comment on attachment 65994 [details]
patch

> +        No new tests because we don't appear to cast a CSSPrimitiveValue to an ETextAlign anywhere currently.

We convert a CSSPrimitiveValue to an ETextAlign in CSSStyleSelector::applyProperty's case for CSSPropertyTextAlign. You should be able to create a test case based on using text-align: auto.

> Index: WebCore.xcodeproj/project.pbxproj
> ===================================================================
> --- WebCore.xcodeproj/project.pbxproj	(revision 66441)
> +++ WebCore.xcodeproj/project.pbxproj	(working copy)
> @@ -20491,6 +20491,7 @@
>  			isa = PBXProject;
>  			buildConfigurationList = 149C284308902B11008A9EFC /* Build configuration list for PBXProject "WebCore" */;
>  			compatibilityVersion = "Xcode 2.4";
> +			developmentRegion = English;
>  			hasScannedForEncodings = 1;
>  			knownRegions = (
>  				English,

Please do not check this project change in.

review- for lack of test case
Comment 4 mitz 2010-08-30 22:41:53 PDT
(In reply to comment #3)
> (From update of attachment 65994 [details])
> > +        No new tests because we don't appear to cast a CSSPrimitiveValue to an ETextAlign anywhere currently.
> 
> We convert a CSSPrimitiveValue to an ETextAlign in CSSStyleSelector::applyProperty's case for CSSPropertyTextAlign. You should be able to create a test case based on using text-align: auto.

text-algin: auto is invalid and the parser rejects it. 

CSSPrimitiveValue::operator ETextAlign() currently maps -webkit-auto to TAAUTO. It would be wrong to map auto to the same value. I think the bug is in CSSPrimitiveValue::CSSPrimitiveValue(ETextAlign). I don’t understand why it maps TAAUTO back to the invalid value auto.
Comment 5 Darin Adler 2010-08-30 22:44:38 PDT
(In reply to comment #4)
> I think the bug is in CSSPrimitiveValue::CSSPrimitiveValue(ETextAlign). I don’t understand why it maps TAAUTO back to the invalid value auto.

I think you are right! Should be easy to test with getComputedStyle.
Comment 6 Justin Garcia 2010-08-31 19:47:40 PDT
Created attachment 66160 [details]
patch

Map TAAuto to -webkit-auto instead of auto.
Comment 7 mitz 2010-08-31 19:53:09 PDT
Comment on attachment 66160 [details]
patch

OK. I just checked what Firefox does, and apparently it returns the modern value 'start', but for now WebKit can do this.
Comment 8 Justin Garcia 2010-08-31 23:14:26 PDT
http://trac.webkit.org/changeset/66581