Bug 138424

Created attachment 241029 [details] Patch

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.

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

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; }

Comment on attachment 241029 [details] Patch Let's move out of review until we cover everything with tests.

Created attachment 244381 [details] Patch

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

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.

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

Created attachment 245053 [details] Patch

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

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.

Comment on attachment 245053 [details] Patch Clearing flags on attachment: 245053 Committed r178822: <http://trac.webkit.org/changeset/178822>

All reviewed patches have been landed. Closing bug.