It isn't specified by in the spec, but it should be possible to come up with a reasonable interpolation behavior anyways.
Created attachment 218610 [details] Patch
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.
(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".
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.
(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.
(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?
Created attachment 218795 [details] Patch
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?
(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.
Comment on attachment 218795 [details] Patch Oops, should have obsoleted once I made the new bug for this.
Created attachment 219336 [details] Patch
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.
(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.
Created attachment 219451 [details] Patch
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.
Created attachment 219455 [details] Patch
Comment on attachment 219455 [details] Patch lgtm. :-)
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.
Landed in r160770.
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.
(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.
Fixed with bug 127010 and further changes to the spec.