Bug 203848 - Support parsing, computed style and property style for offset, offset-path, offset-distance
Summary: Support parsing, computed style and property style for offset, offset-path, o...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dirk Schulze
URL:
Keywords:
Depends on:
Blocks: 203847 203967
  Show dependency treegraph
 
Reported: 2019-11-05 05:37 PST by Dirk Schulze
Modified: 2020-10-12 02:24 PDT (History)
14 users (show)

See Also:


Attachments
Patch (47.95 KB, patch)
2019-11-05 06:07 PST, Dirk Schulze
no flags Details | Formatted Diff | Diff
Patch (49.31 KB, patch)
2019-11-05 23:51 PST, Dirk Schulze
no flags Details | Formatted Diff | Diff
Patch (49.52 KB, patch)
2019-11-08 07:32 PST, Dirk Schulze
krit: review?
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Schulze 2019-11-05 05:37:01 PST
Support the properties  offset, offset-path (only none | path()), offset-distance.
Comment 1 Dirk Schulze 2019-11-05 06:07:57 PST
Created attachment 382818 [details]
Patch
Comment 2 Dirk Schulze 2019-11-05 23:51:32 PST
Created attachment 382898 [details]
Patch
Comment 3 Simon Fraser (smfr) 2019-11-07 13:02:31 PST
Comment on attachment 382898 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        This patch implements the parsing, computed style and property style
> +        for the
> +        longhand properties: offset-path, offset-distance
> +        shorthand properties: offset

Weird wrapping

> Source/WebCore/css/parser/CSSPropertyParser.cpp:2542
> +    CSSParserTokenRange rangeCopy = range;
> +    CSSParserTokenRange args = consumeFunction(rangeCopy);

What's happening with rangeCopy?

> Source/WebCore/css/parser/CSSPropertyParser.cpp:2544
> +    // FIXME-NEWPARSER: CSSBasicShape should be a CSSValue, and shapes should not be primitive values.

Why the FIXME-NEWPARSER in new code?

> Source/WebCore/css/parser/CSSPropertyParser.cpp:2550
> +    range = rangeCopy;

And here.

> Source/WebCore/page/RuntimeEnabledFeatures.h:184
> +    void setMotionEnabled(bool isEnabled) { m_motionEnabled = isEnabled; }
> +    bool motionEnabled() const { return m_motionEnabled; }
> +

Let's call this CSSMotionPath. so setCSSMotionPathEnabled etc

> Source/WebCore/rendering/style/RenderStyle.h:648
> +    const Length& offsetDistance() const { return m_rareNonInheritedData->offsetDistance; }

You can return a Length by value.

> Source/WebCore/rendering/style/RenderStyle.h:1175
> +    void setOffsetDistance(Length&& offsetDistance) { SET_VAR(m_rareNonInheritedData, offsetDistance, WTFMove(offsetDistance)); }

No point using r-value ref for a Length I think.

> Source/WebCore/rendering/style/StyleRareNonInheritedData.h:148
> +    RefPtr<BasicShape> offsetPath;
> +    Length offsetDistance;

How does this affect padding in the class? Is there a more optimal layout?
Comment 4 Dirk Schulze 2019-11-08 03:31:59 PST
(In reply to Simon Fraser (smfr) from comment #3)
> Comment on attachment 382898 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=382898&action=review
> 
> > Source/WebCore/ChangeLog:11
> > +        This patch implements the parsing, computed style and property style
> > +        for the
> > +        longhand properties: offset-path, offset-distance
> > +        shorthand properties: offset
> 
> Weird wrapping
> 
> > Source/WebCore/css/parser/CSSPropertyParser.cpp:2542
> > +    CSSParserTokenRange rangeCopy = range;
> > +    CSSParserTokenRange args = consumeFunction(rangeCopy);
> 
> What's happening with rangeCopy?

That is quite common in CSSPropertyParser. Some functions, like consumeFunction, increase the token iterator of the range while parsing the range. The copy allows parsing the range and, if parsing of the sub-range fails, the same range can be consumed by another function changing the range.

If successful, the original range gets replaced by the copy.

> 
> > Source/WebCore/css/parser/CSSPropertyParser.cpp:2544
> > +    // FIXME-NEWPARSER: CSSBasicShape should be a CSSValue, and shapes should not be primitive values.
> 
> Why the FIXME-NEWPARSER in new code?
> 
> > Source/WebCore/css/parser/CSSPropertyParser.cpp:2550
> > +    range = rangeCopy;

This is from the same FIXME as for the shape-outside property. Sadly there is not much to share between the two. I think the idea is to use CSSValue, CSSValueList, CSSPrimitiveValue as much as possible and not to create custom CSSValues (which CSSBasicShape is).

> 
> And here.
> 
> > Source/WebCore/page/RuntimeEnabledFeatures.h:184
> > +    void setMotionEnabled(bool isEnabled) { m_motionEnabled = isEnabled; }
> > +    bool motionEnabled() const { return m_motionEnabled; }
> > +
> 
> Let's call this CSSMotionPath. so setCSSMotionPathEnabled etc

Ok. But that will mean changing the runtime flag to cssMotionPath (instead of motion) as well since code gets generates.

> 
> > Source/WebCore/rendering/style/RenderStyle.h:648
> > +    const Length& offsetDistance() const { return m_rareNonInheritedData->offsetDistance; }
> 
> You can return a Length by value.

Length is special like a few other value types and used the same way in more than 70 cases.

> 
> > Source/WebCore/rendering/style/RenderStyle.h:1175
> > +    void setOffsetDistance(Length&& offsetDistance) { SET_VAR(m_rareNonInheritedData, offsetDistance, WTFMove(offsetDistance)); }
> 
> No point using r-value ref for a Length I think.

See above.

> 
> > Source/WebCore/rendering/style/StyleRareNonInheritedData.h:148
> > +    RefPtr<BasicShape> offsetPath;
> > +    Length offsetDistance;
> 
> How does this affect padding in the class? Is there a more optimal layout?

The offset-* properties do not affect layout:

The offset-path property defines possible definitions of an element along a given path.
The offset-distance property defines a position on this path.
The offset-rotate property defines the rotation of the object on the path at the specified position via offset-distance.

The idea is the following: A user animates the offset-distance property and the element will transition along the path specified for offset-path.

In a way, offset is just a different kind of transform. As a matter of fact, I intend to implement it as a transform initially: This is platform independent and will provide a way to performance test the offset implementation.
Comment 5 Dirk Schulze 2019-11-08 07:32:56 PST
Created attachment 383124 [details]
Patch
Comment 6 Simon Fraser (smfr) 2019-11-08 11:22:30 PST
> > 
> > > Source/WebCore/rendering/style/StyleRareNonInheritedData.h:148
> > > +    RefPtr<BasicShape> offsetPath;
> > > +    Length offsetDistance;
> > 
> > How does this affect padding in the class? Is there a more optimal layout?
> 
> The offset-* properties do not affect layout:

I mean the in-memory layout of StyleRareNonInheritedData

Try ./Tools/Scripts/dump-class-layout on a release build.
Comment 7 Dirk Schulze 2019-11-08 12:33:33 PST
+303 <  1>   [93m<PADDING: 1 byte>[0m
+304 <  8>     [94mWTF::RefPtr<WebCore::BasicShape, WTF::DumbPtrTraits<WebCore::BasicShape> >[0m offsetPath
+304 <  8>       WTF::DumbPtrTraits<WebCore::BasicShape>::StorageType m_ptr
+312 <  8>     [94mWebCore::Length[0m offsetDistance
+312 <  4>       WebCore::Length::(anonymous union) None
+316 <  1>       bool m_hasQuirk
+317 <  1>       unsigned char m_type
+318 <  1>       bool m_isFloat
+319 <  1>   [93m<PADDING: 1 byte>[0m
...


Total byte size: 496
Total pad bytes: 55
Padding percentage: 11.09 %
Comment 8 Dirk Schulze 2019-11-20 04:40:19 PST
Ping
Comment 9 Antoine Quint 2020-10-12 02:24:43 PDT
I think a first step before considering this patch would be to import the css/motion WPT tests with baseline results before any experimental support for the new CSS properties related to Motion Path. If the parsing WPT tests don't support the full breadth of testing added in the patch, we should make sure the WPT tests are updated to include them.

As for this patch, I think the run-time flag should be made a setting, which as I understand it is the recommended way to add an experimental flag that doesn't require being checked across processes. I added one recently for the support of individual transform operations, see CSSIndividualTransformPropertiesEnabled in WebPreferencesExperimental.yaml.