[Webkit-unassigned] [Bug 134419] [CSS Grid Layout] Implement justify-self css property

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=134419





--- Comment #21 from Javier Fernandez <jfernandez at igalia.com>  2014-07-06 01:16:30 PST ---
(In reply to comment #20)
> (From update of attachment 234103 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=234103&action=review
> 
> > Source/WebCore/css/CSSParser.cpp:1371
> > -    color = document->page()->theme().systemColor(id).rgb();
> > -    return true;
> > +    Color parsedColor = document->page()->theme().systemColor(id);
> > +    color = parsedColor.rgb();
> > +    return parsedColor.isValid();
> 
> This seems fine, but I'm quite concerned that there was a bug that only manifests with this patch applied and that we can't reproduce normally.
> 

I've been investigating and finally got the answer to this weird issue. First of all, I think the portion patch specific to the parseSystemColor is clearly necessary, since the function should return whether the color value, passed as string, is a valid parsed color or not. Current implementation returns false only if the string is not resolved to a valid CSSValueKeywordID. 

The canvas-color-serialization.html test fails when trying invalid color values, assuming they are not valid keywords, when passing "true" as color. The reason why this patch, apparently unrelated to the color parsing logic, makes the test to fail is that the CSSValueTrue keyword is defined for the overflow-alignment "true". Hence, "true" is now a valid CSSValueKeywordID and current implementation does not detect, because of insufficient/wrong checks, that is not a valid color. 

So, the parseSystemColor should return parsedColor.isValid(); instead. I filed a separated bug (#f134661) or this issue, so I could remove that portion of the reviewed patch and land a cleaner one, focusing only in the justify-self parsing logic.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.