[CSS Exclusions] shape-outside on floats for rectangle shapes positioning

• Component: CSS
• Status: RESOLVED FIXED

[Bem Jones-Bey]

This bug tracks the implementation of shape-outside on floats for rectangular shape outsides. This is for properly implementing the positioning of the shape, so that it can be at something other than 0,0

[Bem Jones-Bey]

Created attachment 182805 [details]
Patch

For informal review

[Bem Jones-Bey]

Created attachment 182812 [details]
Patch

For informal review, added FIXME bugs, added a test case for accelerated rendering path

[Bem Jones-Bey]

Comment on attachment 182812 [details]
Patch

I think this could have the shiny happy r=?

[Hans Muller]

(In reply to comment #1)
> Created an attachment (id=182805) [details]
> Patch
> 
> For informal review

I'm no layout expert so it's difficult to judge the correctness of these changes.  If the ChangeLog provided an overview of what you're doing, that would help.

The FIXME additions don't match the prevalent convention: "FIXME: <explanation>".  You don't seem to have included the colon.

The changes for RenderBlock::flipFloatForWritingModeForChild() seem to assume that the caller is going to paint the child, rather than do some computation based on its boundary.  I assume that's always the case now, but could this become a landmine?

I thought xPositionForFloatIncludingMarginForPaint() was a little bit difficult to read because of the #ifdefing.  It might be worth collecting the CSS_EXCLUSIONS code in one place:

    LayoutUnit xPositionForFloatIncludingMarginForPaint(const FloatingObject* child) const
    {
#if ENABLE(CSS_EXCLUSIONS)
        ExclusionShapeOutsideInfo *shapeOutside = child->renderer()->exclusionShapeOutsideInfo();
        if (shapeOutside) 
            return child->x() - (isHorizontalWritingMode() ? shapeOutside->shapeLogicalLeft() : shapeOutside->shapeLogicalTop());
#endif
        if (isHorizontalWritingMode())
            return child->x() + child->renderer()->marginLeft();

        return child->x() + marginBeforeForChild(child->renderer());
    }

Likewise for LayoutUnit yPositionForFloatIncludingMarginForPaint()

Shouldn't the test cover vertical-lr writing-mode?

[Bem Jones-Bey]

(In reply to comment #4)
> (In reply to comment #1)
> > Created an attachment (id=182805) [details] [details]
> > Patch
> > 
> > For informal review
> 
> I'm no layout expert so it's difficult to judge the correctness of these changes.  If the ChangeLog provided an overview of what you're doing, that would help.

Ok, I'll add an overview.

> 
> The FIXME additions don't match the prevalent convention: "FIXME: <explanation>".  You don't seem to have included the colon.

Yeah, I did FIXBE Bug 33333: instead of FIXME: Bug 33333. I wish the check style script would catch that.

> 
> The changes for RenderBlock::flipFloatForWritingModeForChild() seem to assume that the caller is going to paint the child, rather than do some computation based on its boundary.  I assume that's always the case now, but could this become a landmine?

You're entirely right. I've fixed this by refactoring the code so that there's a bool that changes the behavior of xPositionForFloatIncludingMargin and yPositionForFloatIncludingMargin, since otherwise I think we'd be diving way too deeply into code duplication land.

> 
> I thought xPositionForFloatIncludingMarginForPaint() was a little bit difficult to read because of the #ifdefing.  It might be worth collecting the CSS_EXCLUSIONS code in one place:
> 
>     LayoutUnit xPositionForFloatIncludingMarginForPaint(const FloatingObject* child) const
>     {
> #if ENABLE(CSS_EXCLUSIONS)
>         ExclusionShapeOutsideInfo *shapeOutside = child->renderer()->exclusionShapeOutsideInfo();
>         if (shapeOutside) 
>             return child->x() - (isHorizontalWritingMode() ? shapeOutside->shapeLogicalLeft() : shapeOutside->shapeLogicalTop());
> #endif
>         if (isHorizontalWritingMode())
>             return child->x() + child->renderer()->marginLeft();
> 
>         return child->x() + marginBeforeForChild(child->renderer());
>     }
> 
> Likewise for LayoutUnit yPositionForFloatIncludingMarginForPaint()

I agree, that is much better. I've removed those methods, but I kept this refactoring.

> 
> Shouldn't the test cover vertical-lr writing-mode?

Sure, I wasn't sure that actually added anything, but I've added the tests.

[Bem Jones-Bey]

Created attachment 183571 [details]
Patch

Updated to respect feedback

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
Source/WebCore/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:10:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:11:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:17:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:18:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:31:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 7 in 7 files


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

[Bem Jones-Bey]

Created attachment 183572 [details]
Patch

Updated to get rid of annoying and stupid tabs. Must fix vim's braindamage that thinks that ChangeLogs should have tabs.

[Dave Hyatt]

Comment on attachment 183572 [details]
Patch

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

r-

> Source/WebCore/rendering/RenderBlock.cpp:3115
> -LayoutPoint RenderBlock::flipFloatForWritingModeForChild(const FloatingObject* child, const LayoutPoint& point) const
> +LayoutPoint RenderBlock::flipFloatForWritingModeForChild(const FloatingObject* child, const LayoutPoint& point, bool paintTime) const

The "ForChild" here is actually meant to imply the 2 * multiplier. It is analogous to RenderBox flipForWritingModeForChild. I don't think we want to introduce an inscrutable boolean here whose meaning cannot be discerned when looking at call sites.

If you want to be consistent with the RenderBox methods, I would just make a second method called flipFloatForWritingMode that doesn't do the double subtraction.

I'd also be fine with an enum instead of a boolean so that the purpose of the value at call sites is clear. Either approach works for me.

[Bem Jones-Bey]

Created attachment 183814 [details]
Patch

Update for review feedback

[Bem Jones-Bey]

(In reply to comment #9)
> (From update of attachment 183572 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=183572&action=review
> 
> r-
> 
> > Source/WebCore/rendering/RenderBlock.cpp:3115
> > -LayoutPoint RenderBlock::flipFloatForWritingModeForChild(const FloatingObject* child, const LayoutPoint& point) const
> > +LayoutPoint RenderBlock::flipFloatForWritingModeForChild(const FloatingObject* child, const LayoutPoint& point, bool paintTime) const
> 
> The "ForChild" here is actually meant to imply the 2 * multiplier. It is analogous to RenderBox flipForWritingModeForChild. I don't think we want to introduce an inscrutable boolean here whose meaning cannot be discerned when looking at call sites.
> 
> If you want to be consistent with the RenderBox methods, I would just make a second method called flipFloatForWritingMode that doesn't do the double subtraction.
> 
> I'd also be fine with an enum instead of a boolean so that the purpose of the value at call sites is clear. Either approach works for me.

I decided to opt for the flag because I think I will need it in other locations in the future, since even if I did create a flipFloatForWritingMode method, it needs to use a different box for the child depending on if it's looking at the shape (layout) or the float (paint).

[Dave Hyatt]

Comment on attachment 183814 [details]
Patch

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

r-

> Source/WebCore/rendering/RenderBlock.h:696
> +    enum FloatRenderingState { FloatLayout, FloatPaint };

I wouldn't describe this as layout vs. painting. I'd just be more explicit about the math, since really it's kind of a hack that painting does this double subtraction at all. We just need to re-architect paint at some point to get rid of it.

I'd just make the enum something like IncludeChildCoordinates vs. DoNotIncludeChildCoordinates. :)

[Dave Hyatt]

Comment on attachment 183814 [details]
Patch

r=me

[WebKit Review Bot]

Comment on attachment 183814 [details]
Patch

Clearing flags on attachment: 183814

Committed r140365: <http://trac.webkit.org/changeset/140365>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.