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

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Javier Fernandez]

This is an intermediate step towards the full implementation of the CSS Box Alignment Module Level 3. 

The justify-self is the first property to be implemented, so after that we can adapt and share code between the other properties of the specs (align-self and align-items). After having a share source code for all of them, the justify-items property should be implemented to complete the specification.

[Javier Fernandez]

Created attachment 234031 [details]
Patch

[Build Bot]

Comment on attachment 234031 [details]
Patch

Attachment 234031 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6108163447193600

New failing tests:
fast/canvas/canvas-color-serialization.html

[Build Bot]

Created attachment 234037 [details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-02  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 234031 [details]
Patch

Attachment 234031 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5934996246233088

New failing tests:
fast/canvas/canvas-color-serialization.html

[Build Bot]

Created attachment 234039 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 234031 [details]
Patch

Attachment 234031 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5167190873473024

New failing tests:
fast/canvas/canvas-color-serialization.html

[Build Bot]

Created attachment 234043 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 234031 [details]
Patch

Attachment 234031 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4729116557312000

New failing tests:
media/W3C/video/networkState/networkState_during_loadstart.html
fast/canvas/canvas-color-serialization.html

[Build Bot]

Created attachment 234047 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-16  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Javier Fernandez]

Created attachment 234070 [details]
patch rebased

[Build Bot]

Comment on attachment 234070 [details]
patch rebased

Attachment 234070 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5198563973791744

New failing tests:
fast/canvas/canvas-color-serialization.html

[Build Bot]

Created attachment 234071 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 234070 [details]
patch rebased

Attachment 234070 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5093312872579072

New failing tests:
fast/canvas/canvas-color-serialization.html
media/W3C/audio/canPlayType/canPlayType_application_octet_stream_with_codecs_1.html

[Build Bot]

Created attachment 234073 [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 234070 [details]
patch rebased

Attachment 234070 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5849289636970496

New failing tests:
fast/canvas/canvas-color-serialization.html

[Build Bot]

Created attachment 234075 [details]
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-05  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 234070 [details]
patch rebased

Attachment 234070 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4784338025578496

New failing tests:
fast/canvas/canvas-color-serialization.html

[Build Bot]

Created attachment 234077 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-12  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Javier Fernandez]

Created attachment 234103 [details]
Fixed root cause of the canvas-color test failure.

[Dean Jackson]

Comment on attachment 234103 [details]
Fixed root cause of the canvas-color test failure.

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.

> Source/WebCore/css/CSSParser.cpp:3123
> +            if (value->id == CSSValueTrue || value->id == CSSValueSafe)
> +                overflowAlignmentKeyword = cssValuePool().createIdentifierValue(value->id);
> +            else
> +                return false;

Please write this the other way around, as an early return.

if (id != True && id != Safe)
  return false;

overflowAlignmentKeyword = ...

> Source/WebCore/css/CSSParser.cpp:3132
> +            if (isItemPositionKeyword(value->id))
> +                position = cssValuePool().createIdentifierValue(value->id);
> +            else
> +                return false;

Same here.

> Source/WebCore/css/CSSParser.cpp:3144
> +        addProperty(propId, createPrimitiveValuePair(position, overflowAlignmentKeyword), important);
> +    else
> +        addProperty(propId, position.release(), important);

I assume createPrimitiveValuePair releases position? Do you even need to do this?

> Source/WebCore/css/CSSValueKeywords.in:563
> +
> +

Nit: double space

[Javier Fernandez]

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

[Javier Fernandez]

Created attachment 234664 [details]
Patch

Parch rebased. Also, removed the canvas color fix because it was already fixed in r170933.

[Javier Fernandez]

Created attachment 234754 [details]
Patch

Parch rebase and ready for landing.

[Build Bot]

Comment on attachment 234754 [details]
Patch

Attachment 234754 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5090585635454976

New failing tests:
media/media-fragments/TC0001.html

[Build Bot]

Created attachment 234758 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-16  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Javier Fernandez]

Created attachment 234774 [details]
Patch

Retrying the same patch.

[WebKit Commit Bot]

Comment on attachment 234774 [details]
Patch

Clearing flags on attachment: 234774

Committed r171010: <http://trac.webkit.org/changeset/171010>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.