[CSS Grid Layout] Ignore ::first-letter pseudo-element

[CSS Grid Layout] Ignore ::first-letter pseudo-element

According to the spec the ::first-line pseudo-element do not apply to
grid containers (neither to flexboxes).

Modified RenderBlock::findFirstLetterBlock() to use isRenderBlockFlow()
instead of isFlexibleBox(). This change has been already made before in
several parts of the code, but there was one missing case here.

Fix issue in RenderBlock::updateFirstLetter() that applies to both grids
and flexboxes. Basically if the grid's or flexbox's container was
defining the ::first-line pseudo-element and the grid or flexbox itself
too, the value from the grid or flexbox was being applied to the items.
Add the proper check to avoid this.

Added two new tests for grid and modified flexbox test to cover the
issue explained above.

TEST=css3/flexbox/flexbox-ignore-container-firstLetter.html
TEST=fast/css-grid-layout/grid-container-ignore-first-letter.html
TEST=fast/css-grid-layout/grid-item-first-letter-valid.html

BUG=395788
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=180043

[Julien - ping for review, 2014-08-07 20:55:57]

lgtm

https://codereview.chromium.org/440233002/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderBlock.cpp (right):

https://codereview.chromium.org/440233002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBlock.cpp:4179: } else if (currChild->isReplaced()
|| currChild->isRenderButton() || currChild->isMenuList() ||
(!firstLetterBlock->isRenderBlockFlow() && !firstLetterBlock->isRenderButton()))
{
This is repeated several now so we should put it into its own function:

firstLetterBlock->isRenderBlockFlow() || firstLetterBlock->isRenderButton()

The upside is that you could also hoist the existing comment about why we have a
isRenderButton() check.

[Manuel Rego, 2014-08-07 22:19:47]

Thanks for the review.

https://codereview.chromium.org/440233002/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderBlock.cpp (right):

https://codereview.chromium.org/440233002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBlock.cpp:4179: } else if (currChild->isReplaced()
|| currChild->isRenderButton() || currChild->isMenuList() ||
(!firstLetterBlock->isRenderBlockFlow() && !firstLetterBlock->isRenderButton()))
{
On 2014/08/07 20:55:57, Julien Chaffraix - PST wrote:
> This is repeated several now so we should put it into its own function:
> 
> firstLetterBlock->isRenderBlockFlow() || firstLetterBlock->isRenderButton()
> 
> The upside is that you could also hoist the existing comment about why we have
a
> isRenderButton() check.

Done.

[Manuel Rego, 2014-08-07 22:19:53]

The CQ bit was checked by rego@igalia.com

[commit-bot: I haz the power, 2014-08-07 22:24:02]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rego@igalia.com/440233002/20001

[commit-bot: I haz the power, 2014-08-07 23:24:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-07 23:24:20]

Failed to request the patch to try. Please note that binary files are still
unsupported at the moment, this is being worked on.

Thanks for your patience.

Transient error: <urlopen error [Errno 104] Connection reset by peer>

[cbiesinger, 2014-08-08 03:28:12]

https://codereview.chromium.org/440233002/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderBlock.cpp (right):

https://codereview.chromium.org/440233002/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:4178: } else if (currChild->isReplaced()
|| currChild->isRenderButton() || currChild->isMenuList() ||
!isRenderBlockFlowOrRenderButton(firstLetterBlock)) {
I'm confused, why do we want a break here if the firstLetterBlock is a
grid/flexbox? Maybe there should be a comment explaining that?

[Manuel Rego, 2014-08-08 12:55:27]

I've uploaded a new version, moving the check out of the while.
Please @cbiesinger could you take a new look? Thanks.

https://codereview.chromium.org/440233002/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderBlock.cpp (right):

https://codereview.chromium.org/440233002/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:4178: } else if (currChild->isReplaced()
|| currChild->isRenderButton() || currChild->isMenuList() ||
!isRenderBlockFlowOrRenderButton(firstLetterBlock)) {
On 2014/08/08 03:28:12, cbiesinger wrote:
> I'm confused, why do we want a break here if the firstLetterBlock is a
> grid/flexbox? Maybe there should be a comment explaining that?

Yeah, I agree that it's confusing to add this in the middle of the while.

It seems simpler to move it after the while, and just check that
firstLetterBlock
is not a flexbox/grid there. Like, we've already done before in
findFirstLetterBlock().

I guess the new code is simpler to understand and the comment is already in
isRenderBlockFlowOrRenderButton().

BTW, if we don't do this, the example with a container will fail, and
::first-letter
pseudo-element from the flexbox/grid will be used in the items.
Because of, as the container has already defined ::first-letter, while
navigating
the children in the previous while, we'll be reaching the flex/grid item, and
we'll
be using the ::first-letter definition from the flexbox/grid. Which is wrong.

[cbiesinger, 2014-08-12 01:18:15]

lgtm. Thanks!

[Manuel Rego, 2014-08-12 07:17:14]

The CQ bit was checked by rego@igalia.com

[commit-bot: I haz the power, 2014-08-12 07:17:44]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rego@igalia.com/440233002/60001

[commit-bot: I haz the power, 2014-08-12 08:23:45]

Change committed as 180043