Bug 142266 – Flex and grid items should be painted as inline-blocks

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Manuel Rego Casasnovas]

From flexbox spec (http://dev.w3.org/csswg/css-flexbox/#painting):
"Flex items paint exactly the same as inline blocks"

From grid spec (http://dev.w3.org/csswg/css-grid/#z-order):
"The painting order of grid items is exactly the same as inline blocks"

This has been already fixed on Blink so we'd need to port https://codereview.chromium.org/23737004 and apply it to flex and grid.

[Manuel Rego Casasnovas]

Created attachment 248725 [details]
Patch

[Manuel Rego Casasnovas]

Adding reviewers. Please, could you take a look? Thanks!

[Darin Adler]

Comment on attachment 248725 [details]
Patch

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

> Source/WebCore/rendering/InlineElementBox.cpp:73
>      LayoutPoint childPoint = paintOffset;
>      if (is<RenderBox>(renderer()) && parent()->renderer().style().isFlippedBlocksWritingMode()) // Faster than calling containingBlock().
>          childPoint = renderer().containingBlock()->flipForWritingModeForChild(&downcast<RenderBox>(renderer()), childPoint);

Looks like we will do this computation even in cases where we won’t be painting. Seems unfortunate.

> Source/WebCore/rendering/RenderElement.cpp:1174
> +    bool preservePhase = paintInfo.phase == PaintPhaseSelection || paintInfo.phase == PaintPhaseTextClip;

The check for PaintPhaseTextClip here seems to be dead code. We return above unless the phase is foreground or selection.

> Source/WebCore/rendering/RenderElement.cpp:1187
> +    PaintInfo info(paintInfo);
> +    info.phase = preservePhase ? paintInfo.phase : PaintPhaseBlockBackground;
> +    paint(info, childPoint);
> +    if (!preservePhase) {
> +        info.phase = PaintPhaseChildBlockBackgrounds;
> +        paint(info, childPoint);
> +        info.phase = PaintPhaseFloat;
> +        paint(info, childPoint);
> +        info.phase = PaintPhaseForeground;
> +        paint(info, childPoint);
> +        info.phase = PaintPhaseOutline;
> +        paint(info, childPoint);
> +    }

The code here seems needlessly complicated. I would write:

    if (paintInfo.phase == PaintPhaseForeground) {
        paint(paintInfo, childPoint);
        return;
    }

Then the code to paint all the different phases. Also seems we could reuse the original PaintInfo and just set the phase back to PaintPhaseSelection when done rather than copying the PaintInfo. Also seems that a helper function that sets the phase would make the code more readable rather than ll the assignment statements followed by calls to pain.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:336
> +        if (!paintChild(*child, paintInfo, paintOffset, paintInfoForChild, usePrintRect, true))

We use enums rather than booleans in cases like this to avoid mysterious “true” like this one. Please do that here even though an enum might take a few more characters.

> Source/WebCore/rendering/RenderGrid.cpp:1069
> +        paintChild(*child, paintInfo, paintOffset, forChild, usePrintRect, true);

Ditto.

[Manuel Rego Casasnovas]

Created attachment 248923 [details]
Patch for landing

[Manuel Rego Casasnovas]

Thanks for the review!

(In reply to comment #3)
> > Source/WebCore/rendering/InlineElementBox.cpp:73
> >      LayoutPoint childPoint = paintOffset;
> >      if (is<RenderBox>(renderer()) && parent()->renderer().style().isFlippedBlocksWritingMode()) // Faster than calling containingBlock().
> >          childPoint = renderer().containingBlock()->flipForWritingModeForChild(&downcast<RenderBox>(renderer()), childPoint);
> 
> Looks like we will do this computation even in cases where we won’t be
> painting. Seems unfortunate.

Yeah, I've kept the early return to avid this.

> > Source/WebCore/rendering/RenderElement.cpp:1174
> > +    bool preservePhase = paintInfo.phase == PaintPhaseSelection || paintInfo.phase == PaintPhaseTextClip;
> 
> The check for PaintPhaseTextClip here seems to be dead code. We return above
> unless the phase is foreground or selection.

True, this is not needed anymore.

> > Source/WebCore/rendering/RenderElement.cpp:1187
> > +    PaintInfo info(paintInfo);
> > +    info.phase = preservePhase ? paintInfo.phase : PaintPhaseBlockBackground;
> > +    paint(info, childPoint);
> > +    if (!preservePhase) {
> > +        info.phase = PaintPhaseChildBlockBackgrounds;
> > +        paint(info, childPoint);
> > +        info.phase = PaintPhaseFloat;
> > +        paint(info, childPoint);
> > +        info.phase = PaintPhaseForeground;
> > +        paint(info, childPoint);
> > +        info.phase = PaintPhaseOutline;
> > +        paint(info, childPoint);
> > +    }
> 
> The code here seems needlessly complicated. I would write:
> 
>     if (paintInfo.phase == PaintPhaseForeground) {
>         paint(paintInfo, childPoint);
>         return;
>     }
> 
> Then the code to paint all the different phases. Also seems we could reuse
> the original PaintInfo and just set the phase back to PaintPhaseSelection
> when done rather than copying the PaintInfo. Also seems that a helper
> function that sets the phase would make the code more readable rather than
> ll the assignment statements followed by calls to pain.

Thanks for the suggestions, I've modified the code and added a helper method. Now, the code seems much better to me.

> > Source/WebCore/rendering/RenderFlexibleBox.cpp:336
> > +        if (!paintChild(*child, paintInfo, paintOffset, paintInfoForChild, usePrintRect, true))
> 
> We use enums rather than booleans in cases like this to avoid mysterious
> “true” like this one. Please do that here even though an enum might take a
> few more characters.

Done.

[WebKit Commit Bot]

Comment on attachment 248923 [details]
Patch for landing

Clearing flags on attachment: 248923

Committed r181691: <http://trac.webkit.org/changeset/181691>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.