Bug 142266 - Flex and grid items should be painted as inline-blocks
Summary: Flex and grid items should be painted as inline-blocks
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Manuel Rego Casasnovas
URL:
Keywords:
Depends on:
Blocks: 60731
  Show dependency treegraph
 
Reported: 2015-03-04 01:05 PST by Manuel Rego Casasnovas
Modified: 2015-03-18 06:48 PDT (History)
11 users (show)

See Also:


Attachments
Patch (17.59 KB, patch)
2015-03-16 06:10 PDT, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch for landing (17.61 KB, patch)
2015-03-18 04:59 PDT, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Manuel Rego Casasnovas 2015-03-04 01:05:09 PST
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.
Comment 1 Manuel Rego Casasnovas 2015-03-16 06:10:43 PDT
Created attachment 248725 [details]
Patch
Comment 2 Manuel Rego Casasnovas 2015-03-17 08:43:21 PDT
Adding reviewers. Please, could you take a look? Thanks!
Comment 3 Darin Adler 2015-03-17 09:09:09 PDT
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.
Comment 4 Manuel Rego Casasnovas 2015-03-18 04:59:18 PDT
Created attachment 248923 [details]
Patch for landing
Comment 5 Manuel Rego Casasnovas 2015-03-18 05:01:52 PDT
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.
Comment 6 WebKit Commit Bot 2015-03-18 06:48:28 PDT
Comment on attachment 248923 [details]
Patch for landing

Clearing flags on attachment: 248923

Committed r181691: <http://trac.webkit.org/changeset/181691>
Comment 7 WebKit Commit Bot 2015-03-18 06:48:36 PDT
All reviewed patches have been landed.  Closing bug.