Bug 125108 – [CSS Shapes] Implement interpolation between keywords in basic shapes

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

[Bem Jones-Bey]

It isn't specified by in the spec, but it should be possible to come up with a reasonable interpolation behavior anyways.

[Rob Buis]

Created attachment 218610 [details]
Patch

[Dirk Schulze]

Comment on attachment 218610 [details]
Patch

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

> Source/WebCore/rendering/style/BasicShapes.cpp:137
> +    return (radius().canBlend(otherCircle->radius())

the extra braces seem unnecessary.

> Source/WebCore/rendering/style/BasicShapes.h:71
> +        // FIXME: Support animations between different shapes in the future.
> +        return type() == other->type();

I wonder how you want to realize that with the current refactoring... It seems like interpolation between different kind of shapes gets less likely.

[Rob Buis]

(In reply to comment #2)
> (From update of attachment 218610 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=218610&action=review
> 
> > Source/WebCore/rendering/style/BasicShapes.cpp:137
> > +    return (radius().canBlend(otherCircle->radius())
> 
> the extra braces seem unnecessary.

Thanks.

> > Source/WebCore/rendering/style/BasicShapes.h:71
> > +        // FIXME: Support animations between different shapes in the future.
> > +        return type() == other->type();
> 
> I wonder how you want to realize that with the current refactoring... It seems like interpolation between different kind of shapes gets less likely.

I am not sure what you mean? Right now this fallback makes sense since we have depricated circle and ellipse. Once those are removed then this one could maybe become pure virtual. Ultimately each basic shape would have to decide if it can blend with "other".

[Dirk Schulze]

Comment on attachment 218610 [details]
Patch

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

> Source/WebCore/rendering/style/BasicShapes.cpp:215
> +    return (radiusX().canBlend(otherEllipse->radiusX())

Ditto. braces.

>>> Source/WebCore/rendering/style/BasicShapes.h:71
>>> +        return type() == other->type();
>> 
>> I wonder how you want to realize that with the current refactoring... It seems like interpolation between different kind of shapes gets less likely.
> 
> I am not sure what you mean? Right now this fallback makes sense since we have depricated circle and ellipse. Once those are removed then this one could maybe become pure virtual. Ultimately each basic shape would have to decide if it can blend with "other".

Exactly. I don't think that this makes it necessarily easier. It may blow up the source code because it is harder to share code.

[Rob Buis]

(In reply to comment #4)
> (From update of attachment 218610 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=218610&action=review
> 
> > Source/WebCore/rendering/style/BasicShapes.cpp:215
> > +    return (radiusX().canBlend(otherEllipse->radiusX())
> 
> Ditto. braces.
> 
> >>> Source/WebCore/rendering/style/BasicShapes.h:71
> >>> +        return type() == other->type();
> >> 
> >> I wonder how you want to realize that with the current refactoring... It seems like interpolation between different kind of shapes gets less likely.
> > 
> > I am not sure what you mean? Right now this fallback makes sense since we have depricated circle and ellipse. Once those are removed then this one could maybe become pure virtual. Ultimately each basic shape would have to decide if it can blend with "other".
> 
> Exactly. I don't think that this makes it necessarily easier. It may blow up the source code because it is harder to share code.

Never mind, it seems the patch is not needed, interpolation between different types of shapes is not in the spec anymore. I just got triggered by the existing comment:

FIXME: Support animations between different shapes in the future.

[Dirk Schulze]

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 218610 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=218610&action=review
> > 
> > > Source/WebCore/rendering/style/BasicShapes.cpp:215
> > > +    return (radiusX().canBlend(otherEllipse->radiusX())
> > 
> > Ditto. braces.
> > 
> > >>> Source/WebCore/rendering/style/BasicShapes.h:71
> > >>> +        return type() == other->type();
> > >> 
> > >> I wonder how you want to realize that with the current refactoring... It seems like interpolation between different kind of shapes gets less likely.
> > > 
> > > I am not sure what you mean? Right now this fallback makes sense since we have depricated circle and ellipse. Once those are removed then this one could maybe become pure virtual. Ultimately each basic shape would have to decide if it can blend with "other".
> > 
> > Exactly. I don't think that this makes it necessarily easier. It may blow up the source code because it is harder to share code.
> 
> Never mind, it seems the patch is not needed, interpolation between different types of shapes is not in the spec anymore. I just got triggered by the existing comment:
> 
> FIXME: Support animations between different shapes in the future.

Ah right... In this case... could you remove the FIXME please and take a look if there is more cleanup possible?

[Rob Buis]

Created attachment 218795 [details]
Patch

[Dirk Schulze]

Comment on attachment 218795 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        [CSS Shapes] Implement interpolation between keywords in basic shapes

The patch looks fine for me and (on fixing a snippet) I would r+ it. However, could you open a new bug report for it? This one is not the right bug report for this fix.

I noticed that even the previous patch to not match the idea of this bug report. This bug report is about interpolation between things like circle(farthest-side) to circle(200px). So keyword to another keyword/value.

> LayoutTests/animations/resources/animation-test-helpers.js:227
>          matches = s.match("polygon\\(nonzero, (.*)\\s+(.*)\\s*,\\s*(.*)\\s+(.*)\\s*,\\s*(.*)\\s+(.*)\\s*,\\s*(.*)\\s+(.*)\\)");
> +        if (!matches)
> +            matches = s.match("polygon\\(evenodd, (.*)\\s+(.*)\\s*,\\s*(.*)\\s+(.*)\\s*,\\s*(.*)\\s+(.*)\\s*,\\s*(.*)\\s+(.*)\\)");

you should be able to use [...|...] instead of using an if, no?

[Rob Buis]

(In reply to comment #8)
> (From update of attachment 218795 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=218795&action=review
> 
> > Source/WebCore/ChangeLog:3
> > +        [CSS Shapes] Implement interpolation between keywords in basic shapes
> 
> The patch looks fine for me and (on fixing a snippet) I would r+ it. However, could you open a new bug report for it? This one is not the right bug report for this fix.
> 
> I noticed that even the previous patch to not match the idea of this bug report. This bug report is about interpolation between things like circle(farthest-side) to circle(200px). So keyword to another keyword/value.

Agreed, I created https://bugs.webkit.org/show_bug.cgi?id=125508 for this patch instead.

> > LayoutTests/animations/resources/animation-test-helpers.js:227
> >          matches = s.match("polygon\\(nonzero, (.*)\\s+(.*)\\s*,\\s*(.*)\\s+(.*)\\s*,\\s*(.*)\\s+(.*)\\s*,\\s*(.*)\\s+(.*)\\)");
> > +        if (!matches)
> > +            matches = s.match("polygon\\(evenodd, (.*)\\s+(.*)\\s*,\\s*(.*)\\s+(.*)\\s*,\\s*(.*)\\s+(.*)\\s*,\\s*(.*)\\s+(.*)\\)");
> 
> you should be able to use [...|...] instead of using an if, no?

Yes, initially I tried that but ran into problems. I took another look and the framework assumed all floats, that was the problem. In my new patch for 125508 I changed a framework a bit so it knows how to deal with shape polygon.

[Rob Buis]

Comment on attachment 218795 [details]
Patch

Oops, should have obsoleted once I made the new bug for this.

[Rob Buis]

Created attachment 219336 [details]
Patch

[Bem Jones-Bey]

Comment on attachment 219336 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        Length values of 0% and 100%.

What about center coordinates specified like "top 50% bottom 10px"? It doesn't seem like you handle those in this patch, as those keywords can't be resolved until layout time.

[Rob Buis]

(In reply to comment #12)
> (From update of attachment 219336 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=219336&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        Length values of 0% and 100%.
> 
> What about center coordinates specified like "top 50% bottom 10px"? It doesn't seem like you handle those in this patch, as those keywords can't be resolved until layout time.

I failed to see that part of the spec before :) Will upload shortly a new patch that should be able to handle that.

[Rob Buis]

Created attachment 219451 [details]
Patch

[WebKit Commit Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/shapes/parsing/parsing-shape-inside-expected.txt', u'LayoutTests/fast/shapes/parsing/parsing-shape-outside-expected.txt', u'LayoutTests/fast/shapes/parsing/parsing-test-utils.js', u'LayoutTests/fast/shapes/shape-outside-floats/shape-outside-animation-expected.txt', u'LayoutTests/fast/shapes/shape-outside-floats/shape-outside-animation.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/page/animation/CSSPropertyAnimation.cpp', u'Source/WebCore/rendering/style/BasicShapes.cpp', u'Source/WebCore/rendering/style/BasicShapes.h', '--commit-queue']" exit_code: 1
ERROR: Source/WebCore/rendering/style/BasicShapes.cpp:88:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 10 files


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

[Rob Buis]

Created attachment 219455 [details]
Patch

[Bem Jones-Bey]

Comment on attachment 219455 [details]
Patch

lgtm. :-)

[Simon Fraser (smfr)]

Comment on attachment 219455 [details]
Patch

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

> Source/WebCore/rendering/style/BasicShapes.cpp:76
> +FloatSize BasicShape::referenceBoxSize(RenderBox* renderer) const

RenderBox should be a reference if it cannot be null.

[Rob Buis]

Landed in r160770.

[Rob Buis]

Reopening this bug. There is a cleaning up patch I need to do and I think some of the shapes other than circle/ellipse need keyword interpolation too.

[Dirk Schulze]

(In reply to comment #20)
> Reopening this bug. There is a cleaning up patch I need to do and I think some of the shapes other than circle/ellipse need keyword interpolation too.

You may use this as master bug and upload the followups on a different bug report.

[Bear Travis]

Fixed with bug 127010 and further changes to the spec.