first-letter pseudo-element from ancestors is not being ignored in grids and flexboxes

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Manuel Rego Casasnovas]

Created attachment 241028 [details]
Test case to reproduce the issue

From the grid and flexbox specs:
"the ::first-line and ::first-letter pseudo-elements do not apply to grid/flex containers"

The ::first-letter pseudo-element is being ignored when it's defined directly in the grid/flex. However if it was defined by an ancestor is not ignored.

Attaching an example file to reproduce the issue.

However the ::first-line pseudo-element is working as expected.

This has been already fixed in Blink (see https://codereview.chromium.org/686173006/).

[Manuel Rego Casasnovas]

Created attachment 241029 [details]
Patch

[WebKit Commit Bot]

Attachment 241029 [details] did not pass style-queue:


ERROR: Source/WebCore/rendering/RenderBlock.cpp:3212:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/rendering/RenderBlock.cpp:3217:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Manuel Rego Casasnovas]

Please, could someone take a look to this patch? Thanks!

[Benjamin Poulain]

Comment on attachment 241029 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        "the ::first-line and ::first-letter pseudo-elements do not apply to

You mention both ::first-line and ::first-letter, but only have tests for ::first-letter.
Is there good coverage for ::first-line? If there is, it would be good to mention it in the changelog.

> Source/WebCore/rendering/RenderBlock.cpp:3217
> +             return;

It is not obvious to me why an early return would be correct here.

For example, isFloatingOrOutOfFlowPositioned() will happily let you skip the early return of Flex and Grid.

> LayoutTests/ChangeLog:14
> +        * css3/flexbox/flexbox-ignore-container-firstLetter-expected.txt:
> +        * css3/flexbox/flexbox-ignore-container-firstLetter.html:
> +        * fast/css-grid-layout/grid-container-ignore-first-letter-expected.txt:
> +        * fast/css-grid-layout/grid-container-ignore-first-letter.html:

I would like tests
-Covering the various combinations of RenderBlock::getFirstLetter().
-Covering ::first-line.
-Covering ::first-line and ::first-letter applied on a ::before/::after with display flex/grid.
-Setting the display through ::first-letter and ::first-line. E.g.
    ::first-letter {
       display: flex;
    }

[Benjamin Poulain]

Comment on attachment 241029 [details]
Patch

Let's move out of review until we cover everything with tests.

[Manuel Rego Casasnovas]

Created attachment 244381 [details]
Patch

[Manuel Rego Casasnovas]

(In reply to comment #4)
> Comment on attachment 241029 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=241029&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        "the ::first-line and ::first-letter pseudo-elements do not apply to
> 
> You mention both ::first-line and ::first-letter, but only have tests for
> ::first-letter.
> Is there good coverage for ::first-line? If there is, it would be good to
> mention it in the changelog.

Nope, there's not enough coverage.
I've reported a different bug #139256 to import another patch from Blink to increase the coverage for ::first-line.
I'll modify the ChangeLog to avoid misunderstandings regarding ::first-line.

> > Source/WebCore/rendering/RenderBlock.cpp:3217
> > +             return;
> 
> It is not obvious to me why an early return would be correct here.
> 
> For example, isFloatingOrOutOfFlowPositioned() will happily let you skip the
> early return of Flex and Grid.

Actually the erly return is wrong and it should go to the next sibling.
I confirmed this behavior in the the CSS WG mailing list:
http://lists.w3.org/Archives/Public/www-style/2014Dec/0305.html

> > LayoutTests/ChangeLog:14
> > +        * css3/flexbox/flexbox-ignore-container-firstLetter-expected.txt:
> > +        * css3/flexbox/flexbox-ignore-container-firstLetter.html:
> > +        * fast/css-grid-layout/grid-container-ignore-first-letter-expected.txt:
> > +        * fast/css-grid-layout/grid-container-ignore-first-letter.html:
> 
> I would like tests
> -Covering the various combinations of RenderBlock::getFirstLetter().

Some part was already covered (for example: fast/css/first-letter-skip-out-of-flow.html).
I've added some new tests to cover the missing ones.

> -Covering ::first-line.

This'll be done in as part of bug #139256.

> -Covering ::first-line and ::first-letter applied on a ::before/::after with
> display flex/grid.

Added for ::first-letter.

> -Setting the display through ::first-letter and ::first-line. E.g.
>     ::first-letter {
>        display: flex;
>     }

Done.

Please, take a look to the new patch. Thanks.

[WebKit Commit Bot]

Attachment 244381 [details] did not pass style-queue:


ERROR: Source/WebCore/rendering/RenderBlock.cpp:3212:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Benjamin Poulain]

Comment on attachment 244381 [details]
Patch

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

Great patch.

Do you have coverage for ancestor with ::first-letter + floating grid/flex? I did not see it.

> Source/WebCore/ChangeLog:13
> +        http://dev.w3.org/csswg/css-grid/#grid-containers
> +        http://dev.w3.org/csswg/css-flexbox/#flex-containers

IMHO, it would be useful to reference http://lists.w3.org/Archives/Public/www-style/2014Dec/0305.html
That description is the least unambiguous.

> Source/WebCore/ChangeLog:20
> +        Also created some new tests to incerase coverage

Typo: incerase

[Manuel Rego Casasnovas]

Created attachment 245053 [details]
Patch

[Manuel Rego Casasnovas]

(In reply to comment #9)
> Comment on attachment 244381 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=244381&action=review
> 
> Great patch.

Thanks for the review.

> Do you have coverage for ancestor with ::first-letter + floating grid/flex?
> I did not see it.

Not specifically, so I've added a few more cases checking this.

> > Source/WebCore/ChangeLog:13
> > +        http://dev.w3.org/csswg/css-grid/#grid-containers
> > +        http://dev.w3.org/csswg/css-flexbox/#flex-containers
> 
> IMHO, it would be useful to reference
> http://lists.w3.org/Archives/Public/www-style/2014Dec/0305.html
> That description is the least unambiguous.

Done.

> > Source/WebCore/ChangeLog:20
> > +        Also created some new tests to incerase coverage
> 
> Typo: incerase

Fixed.

[WebKit Commit Bot]

Attachment 245053 [details] did not pass style-queue:


ERROR: Source/WebCore/rendering/RenderBlock.cpp:3212:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 245053 [details]
Patch

Clearing flags on attachment: 245053

Committed r178822: <http://trac.webkit.org/changeset/178822>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.