Bug 233344 – Support ray() shape in offset-path

• Component: CSS
• Status: RESOLVED FIXED

[Kiet Ho]

Support ray() shape in offset-path. This includes:

* Support for parsing ray() (https://bugs.webkit.org/show_bug.cgi?id=233153)
* Use offset-position as the starting position of a ray() (this patch)
* Figure out the location on a ray() path to move an element to (this patch)

[Radar WebKit Bug Importer]

<rdar://problem/85611980>

[Nikos Mouchtaris]

Created attachment 456773 [details]
wip

[Nikos Mouchtaris]

Created attachment 458841 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 458841 [details]
Patch

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

> Source/WebCore/rendering/PathOperation.cpp:58
> +static double toPositiveAngle(double angle)
> +{
> +    angle = fmod(angle, 360);
> +    while (angle < 0)
> +        angle += 360.0;
> +    return angle;
> +}

This is the same as this code in CSSGradientAngle:
    angleDeg = fmodf(angleDeg, 360);
    if (angleDeg < 0)
        angleDeg += 360;

> Source/WebCore/rendering/PathOperation.cpp:60
> +double RayPathOperation::getLengthForPath() const

We don't use 'get' in function names, so just 'lengthForPath()"

> Source/WebCore/rendering/PathOperation.cpp:86
> +        // Get possible intersection sides
> +        double s1 = cos(radians) >= 0 ? top : bottom;
> +        double s2 = sin(radians) >= 0 ? right : left;
> +        // Get intersection side - based on length of tan() * s1
> +        double intersectionSide = sin(radians) * s1 > cos(radians) * s2 ? s1 : s2;

If you can factor bits of this into reusable functions in GeometryUtilities.cpp it would be easier to read.

> Source/WebCore/rendering/PathOperation.cpp:88
> +        double angle = deg2rad(sin(radians) * s1 > cos(radians) * s2 ? 90 - std::fmod(toPositiveAngle(m_angle), 90) : std::fmod(toPositiveAngle(m_angle), 90));

Would prefer not to wrap that very complex expression in deg2rad(), for readability. Use an intermediate variable.

> Source/WebCore/rendering/PathOperation.h:230
>      float m_angle;

This should be initialized: float m_angle { 0 }

> Source/WebCore/rendering/PathOperation.h:232
>      bool m_isContaining;

bool m_isContaining { false }

> Source/WebCore/rendering/RenderLayer.cpp:1326
> +    } else if (pathOperation && is<RayPathOperation>(pathOperation)) {

I'm surprised that pathOperation is in scope here.

This function would be better with earlier returns:

  auto pathOperation = renderer().style().offsetPath()
  if (!pathOperation)
    return;

  if (is<BoxPathOperation>(*pathOperation)) {
    ...
    return;
  }

  if (is<RayPathOperation>(*operation) {
   ...
  return;
  }

> Source/WebCore/rendering/RenderLayer.h:511
> +    

whitespace

[Nikos Mouchtaris]

Created attachment 458919 [details]
Patch

[Nikos Mouchtaris]

(In reply to Simon Fraser (smfr) from comment #4)
> Comment on attachment 458841 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=458841&action=review
> 
> > Source/WebCore/rendering/PathOperation.cpp:58
> > +static double toPositiveAngle(double angle)
> > +{
> > +    angle = fmod(angle, 360);
> > +    while (angle < 0)
> > +        angle += 360.0;
> > +    return angle;
> > +}
> 
> This is the same as this code in CSSGradientAngle:
>     angleDeg = fmodf(angleDeg, 360);
>     if (angleDeg < 0)
>         angleDeg += 360;

Added shared function in GeometryUtilities.

> 
> > Source/WebCore/rendering/PathOperation.cpp:60
> > +double RayPathOperation::getLengthForPath() const
> 
> We don't use 'get' in function names, so just 'lengthForPath()"
> 
Fixed.
> > Source/WebCore/rendering/PathOperation.cpp:86
> > +        // Get possible intersection sides
> > +        double s1 = cos(radians) >= 0 ? top : bottom;
> > +        double s2 = sin(radians) >= 0 ? right : left;
> > +        // Get intersection side - based on length of tan() * s1
> > +        double intersectionSide = sin(radians) * s1 > cos(radians) * s2 ? s1 : s2;
> 
> If you can factor bits of this into reusable functions in
> GeometryUtilities.cpp it would be easier to read.
> 
Added 3 functions in GeometryUtilities that calculate the intermediate values and the final value. Added another function that is used earlier here.
> > Source/WebCore/rendering/PathOperation.cpp:88
> > +        double angle = deg2rad(sin(radians) * s1 > cos(radians) * s2 ? 90 - std::fmod(toPositiveAngle(m_angle), 90) : std::fmod(toPositiveAngle(m_angle), 90));
> 
> Would prefer not to wrap that very complex expression in deg2rad(), for
> readability. Use an intermediate variable.
> 
Fixed.
> > Source/WebCore/rendering/PathOperation.h:230
> >      float m_angle;
> 
> This should be initialized: float m_angle { 0 }
> 
Fixed.
> > Source/WebCore/rendering/PathOperation.h:232
> >      bool m_isContaining;
> 
> bool m_isContaining { false }
> 
Fixed.
> > Source/WebCore/rendering/RenderLayer.cpp:1326
> > +    } else if (pathOperation && is<RayPathOperation>(pathOperation)) {
> 
> I'm surprised that pathOperation is in scope here.
> 
> This function would be better with earlier returns:
> 
>   auto pathOperation = renderer().style().offsetPath()
>   if (!pathOperation)
>     return;
> 
>   if (is<BoxPathOperation>(*pathOperation)) {
>     ...
>     return;
>   }
> 
>   if (is<RayPathOperation>(*operation) {
>    ...
>   return;
>   }

Fixed
> 
> > Source/WebCore/rendering/RenderLayer.h:511
> > +    
> 
> whitespace
Fixed.

[Simon Fraser (smfr)]

Comment on attachment 458919 [details]
Patch

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

> Source/WebCore/platform/graphics/GeometryUtilities.h:69
> +double lengthOfRayIntersectionWithBoundingBox(const FloatRect&, const FloatSize&, double);

Name the arguments.

> Source/WebCore/platform/graphics/GeometryUtilities.h:73
> +double lengthOfPointToSideOfIntersection(const FloatRect&, const FloatSize&, double);

I can't tell what the arguments are from the signature. I'd expect a FloatRect, and a FloatPoint + FloatSize for the ray?

> Source/WebCore/platform/graphics/GeometryUtilities.h:77
> +double angleOfPointToSideOfIntersection(const FloatRect&, const FloatSize&, double, double);

Name the arguments (clarifying if angles are degrees or radians)

> Source/WebCore/platform/graphics/GeometryUtilities.h:80
> +std::tuple<double, double, double, double> distanceOfPointToSidesOfRect(const FloatRect&, const FloatSize&);

Consider RectEdges<double>

[Nikos Mouchtaris]

Created attachment 458923 [details]
Patch

[Nikos Mouchtaris]

(In reply to Simon Fraser (smfr) from comment #7)
> Comment on attachment 458919 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=458919&action=review
> 
> > Source/WebCore/platform/graphics/GeometryUtilities.h:69
> > +double lengthOfRayIntersectionWithBoundingBox(const FloatRect&, const FloatSize&, double);
> 
> Name the arguments.
> 
Fixed.
> > Source/WebCore/platform/graphics/GeometryUtilities.h:73
> > +double lengthOfPointToSideOfIntersection(const FloatRect&, const FloatSize&, double);
> 
> I can't tell what the arguments are from the signature. I'd expect a
> FloatRect, and a FloatPoint + FloatSize for the ray?
> 
Fixed.
> > Source/WebCore/platform/graphics/GeometryUtilities.h:77
> > +double angleOfPointToSideOfIntersection(const FloatRect&, const FloatSize&, double, double);
> 
> Name the arguments (clarifying if angles are degrees or radians)
> 
Fixed.
> > Source/WebCore/platform/graphics/GeometryUtilities.h:80
> > +std::tuple<double, double, double, double> distanceOfPointToSidesOfRect(const FloatRect&, const FloatSize&);
> 
> Consider RectEdges<double>

Fixed.

[Simon Fraser (smfr)]

Comment on attachment 458923 [details]
Patch

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

> Source/WebCore/platform/graphics/GeometryUtilities.h:69
> +double lengthOfRayIntersectionWithBoundingBox(const FloatRect& boundingRect, const FloatSize& position, double angle);

Normally a position would be a FloatPoint, but I guess the position is relative to the rect location? I think it would be better as a FloatPoint in the same coordinate space as the FloatRect.

You have 'angle' here and 'radians' below. I'd suggest 'angleInRadians'. You could also do a `using std::pair<FloatPosition, double> = Ray` which might make these clearer.

> Source/WebCore/platform/graphics/GeometryUtilities.h:73
> +double lengthOfPointToSideOfIntersection(const FloatRect& boundingRect, const FloatSize& position, double radians);

Same comments as above.

> Source/WebCore/platform/graphics/GeometryUtilities.h:76
> +// Given a box and a ray (described by a starting position and angle in degrees), compute the acute angle between the ray
> +// and the line segment from the starting  point to the closest point on the side that the ray intersects with.

Is the 'length' argument to this function the value returned from lengthOfPointToSideOfIntersection()? Can we just compute it inside this function?

[Nikos Mouchtaris]

Created attachment 458930 [details]
Patch

[Nikos Mouchtaris]

(In reply to Simon Fraser (smfr) from comment #10)
> Comment on attachment 458923 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=458923&action=review
> 
> > Source/WebCore/platform/graphics/GeometryUtilities.h:69
> > +double lengthOfRayIntersectionWithBoundingBox(const FloatRect& boundingRect, const FloatSize& position, double angle);
> 
> Normally a position would be a FloatPoint, but I guess the position is
> relative to the rect location? I think it would be better as a FloatPoint in
> the same coordinate space as the FloatRect.
> 
> You have 'angle' here and 'radians' below. I'd suggest 'angleInRadians'. You
> could also do a `using std::pair<FloatPosition, double> = Ray` which might
> make these clearer.
> 
I think std::pair works well here.
> > Source/WebCore/platform/graphics/GeometryUtilities.h:73
> > +double lengthOfPointToSideOfIntersection(const FloatRect& boundingRect, const FloatSize& position, double radians);
> 
> Same comments as above.
> 
Fixed.
> > Source/WebCore/platform/graphics/GeometryUtilities.h:76
> > +// Given a box and a ray (described by a starting position and angle in degrees), compute the acute angle between the ray
> > +// and the line segment from the starting  point to the closest point on the side that the ray intersects with.
> 
> Is the 'length' argument to this function the value returned from
> lengthOfPointToSideOfIntersection()? Can we just compute it inside this
> function?
Now computed inside instead of passed.

[Nikos Mouchtaris]

Created attachment 458987 [details]
Patch

[Nikos Mouchtaris]

Created attachment 459087 [details]
Patch

[Nikos Mouchtaris]

Created attachment 459092 [details]
Patch

[EWS]

Committed r294001 (250437@main): <https://commits.webkit.org/250437@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 459092 [details].