Bug 217311 – clip-path: path() ignores page zooming (Command-+)

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Noam Rosenthal]

When zooming the page, the content gets zoomed but the clip-path doesn't.

See w3c test: https://github.com/web-platform-tests/wpt/pull/25981

[Noam Rosenthal]

Created attachment 410512 [details]
Very simple manual test case

[Radar WebKit Bug Importer]

<rdar://problem/69977271>

[Simon Fraser (smfr)]

Command-+ zooming, not pinch zooming.

[Noam Rosenthal]

(In reply to Simon Fraser (smfr) from comment #3)
> Command-+ zooming, not pinch zooming.

Correct.

[Noam Rosenthal]

Created attachment 410743 [details]
Patch

[Noam Rosenthal]

(In reply to Noam Rosenthal from comment #5)
> Created attachment 410743 [details]
> Patch

The patch is big because it also includes importing a lot of w3c tests.

[Darin Adler]

Comment on attachment 410743 [details]
Patch

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

> Source/WebCore/rendering/style/BasicShapes.cpp:64
> +struct SVGPathTransformedByteStream {

Not new to this patch: this struct should not use m_ prefixes on its public data members.

> Source/WebCore/rendering/style/BasicShapes.cpp:65
> +    SVGPathTransformedByteStream(const FloatPoint& offset, float zoom, const SVGPathByteStream& rawStream)

Should be able to use aggregate initialization instead of a constructor.

> Source/WebCore/rendering/style/BasicShapes.cpp:123
> +struct TransformedByteStreamPathPolicy : public TinyLRUCachePolicy<SVGPathTransformedByteStream, Path> {

No need for public here

> Source/WebCore/rendering/style/BasicShapes.cpp:124
>  public:

No need for public here

> LayoutTests/imported/w3c/ChangeLog:10
> +        Some of them don't pass yet for unrelated reasons, skipped in TestExpectations.

Expect failure, not skipped. Which is good/better.

[Noam Rosenthal]

Created attachment 410762 [details]
Patch for landing

[EWS]

Committed r268138: <https://trac.webkit.org/changeset/268138>

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

[Simon Fraser (smfr)]

Comment on attachment 410762 [details]
Patch for landing

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

> LayoutTests/imported/w3c/web-platform-tests/css/css-masking/clip-path/clip-path-path-with-zoom.html:22
> +    zoom: 2;

zoom is not a standardized property, and is not supported by Firefox so I don't think it's appropriate to use it in WPT tests.

[Noam Rosenthal]

(In reply to Simon Fraser (smfr) from comment #10)
> Comment on attachment 410762 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=410762&action=review
> 
> > LayoutTests/imported/w3c/web-platform-tests/css/css-masking/clip-path/clip-path-path-with-zoom.html:22
> > +    zoom: 2;
> 
> zoom is not a standardized property, and is not supported by Firefox so I
> don't think it's appropriate to use it in WPT tests.

there is no current way to represent page zoom in Selenium/WPT. There is an open issue for it here: https://github.com/w3c/webdriver/issues/1378

So the zoom property here is not used as a standard presentation property, but rather as a way to simulate a user-initiated page zoom. What would you suggest to do instead?

[Simon Fraser (smfr)]

(In reply to Noam Rosenthal from comment #11)
> (In reply to Simon Fraser (smfr) from comment #10)
> > Comment on attachment 410762 [details]
> > Patch for landing
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=410762&action=review
> > 
> > > LayoutTests/imported/w3c/web-platform-tests/css/css-masking/clip-path/clip-path-path-with-zoom.html:22
> > > +    zoom: 2;
> > 
> > zoom is not a standardized property, and is not supported by Firefox so I
> > don't think it's appropriate to use it in WPT tests.
> 
> there is no current way to represent page zoom in Selenium/WPT. There is an
> open issue for it here: https://github.com/w3c/webdriver/issues/1378
> 
> So the zoom property here is not used as a standard presentation property,
> but rather as a way to simulate a user-initiated page zoom. What would you
> suggest to do instead?

Move the tests out of WPT so they don't fail in Firefox?

[Noam Rosenthal]

(In reply to Simon Fraser (smfr) from comment #12)
> (In reply to Noam Rosenthal from comment #11)
> > (In reply to Simon Fraser (smfr) from comment #10)
> > > Comment on attachment 410762 [details]
> > > Patch for landing
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=410762&action=review
> > > 
> > > > LayoutTests/imported/w3c/web-platform-tests/css/css-masking/clip-path/clip-path-path-with-zoom.html:22
> > > > +    zoom: 2;
> > > 
> > > zoom is not a standardized property, and is not supported by Firefox so I
> > > don't think it's appropriate to use it in WPT tests.
> > 
> > there is no current way to represent page zoom in Selenium/WPT. There is an
> > open issue for it here: https://github.com/w3c/webdriver/issues/1378
> > 
> > So the zoom property here is not used as a standard presentation property,
> > but rather as a way to simulate a user-initiated page zoom. What would you
> > suggest to do instead?
> 
> Move the tests out of WPT so they don't fail in Firefox?

Doable 
Not sure when I can get to it myself though but can try in a few weeks