Content-Length: 455286 | pFad | http://github.com/WebKit/WebKit/pull/13479

20 [image-set] Handle zero resolution and clamping negative resolutions in calc expressions by rreno · Pull Request #13479 · WebKit/WebKit · GitHub
Skip to content

[image-set] Handle zero resolution and clamping negative resolutions in calc expressions #13479

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
May 21, 2023
Merged

[image-set] Handle zero resolution and clamping negative resolutions in calc expressions #13479

merged 1 commit into from
May 21, 2023

Conversation

rreno
Copy link
Member

@rreno rreno commented May 5, 2023 •
edited by webkit-early-warning-system
Loading

f9cc497

[image-set] Handle zero resolution and clamping negative resolutions in calc expressions
https://bugs.webkit.org/show_bug.cgi?id=254388
rdar://107167273

Reviewed by Tim Nguyen.

In a CSSWG resolution it was decided that zero resolutions should be
accepted by image-set but that they should produce an invalid image.
This change accepts zero but will reject negative resolutions. In a
follow-up we should clamp the result of calc expressions to zero.

This change also resyncs the image-set WPT as of upstream commit
c56aec60503481604225fdb7705f67d82ce437b8

w3c/csswg-drafts#8532 (comment)

* LayoutTests/TestExpectations:
* LayoutTests/imported/w3c/web-platform-tests/css/css-images/image-set/image-set-negative-resolution-rendering-2.html:
* LayoutTests/imported/w3c/web-platform-tests/css/css-images/image-set/image-set-negative-resolution-rendering-3-expected.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-images/image-set/image-set-negative-resolution-rendering-3.html: Copied from LayoutTests/imported/w3c/web-platform-tests/css/css-images/image-set/image-set-negative-resolution-rendering.html.
* LayoutTests/imported/w3c/web-platform-tests/css/css-images/image-set/image-set-negative-resolution-rendering-expected.html:
* LayoutTests/imported/w3c/web-platform-tests/css/css-images/image-set/image-set-negative-resolution-rendering.html:
* LayoutTests/imported/w3c/web-platform-tests/css/css-images/image-set/image-set-parsing-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/css-images/image-set/image-set-parsing.html:
* LayoutTests/imported/w3c/web-platform-tests/css/css-images/image-set/image-set-zero-resolution-rendering-2-expected.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-images/image-set/image-set-zero-resolution-rendering-2.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-images/image-set/image-set-zero-resolution-rendering-expected.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-images/image-set/image-set-zero-resolution-rendering.html: Added.

* Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:
(WebCore::CSSPropertyParserHelpers::consumeImageSetOption):
* Source/WebCore/rendering/style/StyleImageSet.cpp:
(WebCore::StyleImageSet::bestImageForScaleFactor):

Canonical link: https://commits.webkit.org/264298@main

aca75bd

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug   🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🛠 gtk
  🧪 ios-wk2-wpt ✅ 🧪 mac-wk1   🧪 gtk-wk2
  🧪 api-ios ✅ 🧪 mac-wk2   🧪 api-gtk
✅ 🛠 tv   🧪 mac-AS-debug-wk2
✅ 🛠 tv-sim ✅ 🧪 mac-wk2-stress
✅ 🛠 🧪 merge ✅ 🛠 watch
✅ 🛠 watch-sim

@rreno rreno self-assigned this May 5, 2023
@rreno rreno added the CSS Cascading Style Sheets implementation label May 5, 2023
@rreno rreno requested a review from nt1m May 5, 2023 04:02
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented May 5, 2023 •
edited
Loading

EWS run on previous version of this PR (hash 8417bd9)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🛠 gtk
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ❌ 🧪 gtk-wk2
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🧪 api-gtk
✅ 🛠 tv ✅ 🧪 mac-AS-debug-wk2
✅ 🛠 tv-sim ✅ 🧪 mac-wk2-stress
✅ 🛠 watch
✅ 🛠 watch-sim

@rreno
Copy link
Member Author

rreno commented May 5, 2023

need to rename the bug this doesn't handle clamping in math functions.

nt1m
nt1m approved these changes May 5, 2023
View reviewed changes
Copy link
Member

@nt1m nt1m left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 things:

  • Can you retitle the issue to be only about zero resolution?
  • Can we add a WPT for rendering zero resolution image set? (similar to the negative resolution one we had before)

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 5, 2023
@rreno
Copy link
Member Author

rreno commented May 5, 2023

Ah right we have zero resolution parsing tests but no zero resolution rendering tests. Yep I'll add a test.

@rreno rreno removed the merging-blocked Applied to prevent a change from being merged label May 20, 2023
@rreno rreno changed the title [image-set] Handle zero resolution and clamping resolutions [image-set] Handle zero resolution May 20, 2023
@rreno rreno force-pushed the eng/image-set-Handle-zero-resolution-and-clamping-resolutions branch from 8417bd9 to ee56df9 Compare May 20, 2023 00:06
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented May 20, 2023 •
edited
Loading

EWS run on previous version of this PR (hash ee56df9)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🛠 gtk
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🧪 gtk-wk2
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🧪 api-gtk
✅ 🛠 tv ✅ 🧪 mac-AS-debug-wk2
✅ 🛠 tv-sim ✅ 🧪 mac-wk2-stress
✅ 🛠 watch
✅ 🛠 watch-sim

@rreno rreno changed the title [image-set] Handle zero resolution [image-set] Handle zero resolution and clamping negative resolutions in calc expressions May 20, 2023
@rreno rreno force-pushed the eng/image-set-Handle-zero-resolution-and-clamping-resolutions branch from ee56df9 to f4c999c Compare May 20, 2023 19:56
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented May 20, 2023 •
edited
Loading

EWS run on previous version of this PR (hash f4c999c)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🛠 gtk
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ❌ 🧪 gtk-wk2
⏳ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🧪 api-gtk
✅ 🛠 tv ✅ 🧪 mac-AS-debug-wk2
✅ 🛠 tv-sim ✅ 🧪 mac-wk2-stress
✅ 🛠 watch
✅ 🛠 watch-sim

@rreno
Copy link
Member Author

rreno commented May 20, 2023

I got clamping to work in calc. We're failing the two new parsing tests because we don't serialize calc() the way WPT expects but we do clamp as we should.

@nt1m can you take another look at this since I've made some changes since the last time you looked.

nt1m
nt1m approved these changes May 20, 2023
View reviewed changes
Copy link
Member

@nt1m nt1m left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks!

Source/WebCore/rendering/style/StyleImageSet.cpp
@@ -96,7 +96,7 @@ ImageWithScale StyleImageSet::bestImageForScaleFactor()
result = image;
}

if (!result.image)
if (!result.image || !result.scaleFactor)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we assert that result.scaleFactor is never negative given calc() should have clamped it earlier?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I'll add a debug assert here for that. Should catch any mistakes in the parser.

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label May 20, 2023
@rreno rreno removed the merging-blocked Applied to prevent a change from being merged label May 21, 2023
@rreno rreno force-pushed the eng/image-set-Handle-zero-resolution-and-clamping-resolutions branch from f4c999c to aca75bd Compare May 21, 2023 00:51
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented May 21, 2023 •
edited
Loading

EWS run on current version of this PR (hash aca75bd)

Misc iOS, tvOS & watchOS macOS Linux Windows
loading 🧪 style loading 🛠 ios loading 🛠 mac loading 🛠 wpe loading 🛠 wincairo
loading 🧪 bindings loading 🛠 ios-sim loading 🛠 mac-AS-debug loading 🧪 wpe-wk2
loading 🧪 webkitperl loading 🧪 ios-wk2 loading 🧪 api-mac loading 🛠 gtk
loading 🧪 ios-wk2-wpt loading 🧪 mac-wk1 loading 🧪 gtk-wk2
loading 🧪 api-ios loading 🧪 mac-wk2 loading 🧪 api-gtk
loading 🛠 tv loading 🧪 mac-AS-debug-wk2
loading 🛠 tv-sim loading 🧪 mac-wk2-stress
loading 🛠 watch
loading 🛠 watch-sim

@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented May 21, 2023 •
edited
Loading

EWS run on current version of this PR (hash aca75bd)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug   🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🛠 gtk
  🧪 ios-wk2-wpt ✅ 🧪 mac-wk1   🧪 gtk-wk2
  🧪 api-ios ✅ 🧪 mac-wk2   🧪 api-gtk
✅ 🛠 tv   🧪 mac-AS-debug-wk2
✅ 🛠 tv-sim ✅ 🧪 mac-wk2-stress
✅ 🛠 🧪 merge ✅ 🛠 watch
✅ 🛠 watch-sim

@rreno rreno added the merge-queue Applied to send a pull request to merge-queue label May 21, 2023
@rreno
[image-set] Handle zero resolution and clamping negative resolutions …
f9cc497 
…in calc expressions

https://bugs.webkit.org/show_bug.cgi?id=254388
rdar://107167273

Reviewed by Tim Nguyen.

In a CSSWG resolution it was decided that zero resolutions should be
accepted by image-set but that they should produce an invalid image.
This change accepts zero but will reject negative resolutions. In a
follow-up we should clamp the result of calc expressions to zero.

This change also resyncs the image-set WPT as of upstream commit
c56aec6

w3c/csswg-drafts#8532 (comment)

* LayoutTests/TestExpectations:
* LayoutTests/imported/w3c/web-platform-tests/css/css-images/image-set/image-set-negative-resolution-rendering-2.html:
* LayoutTests/imported/w3c/web-platform-tests/css/css-images/image-set/image-set-negative-resolution-rendering-3-expected.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-images/image-set/image-set-negative-resolution-rendering-3.html: Copied from LayoutTests/imported/w3c/web-platform-tests/css/css-images/image-set/image-set-negative-resolution-rendering.html.
* LayoutTests/imported/w3c/web-platform-tests/css/css-images/image-set/image-set-negative-resolution-rendering-expected.html:
* LayoutTests/imported/w3c/web-platform-tests/css/css-images/image-set/image-set-negative-resolution-rendering.html:
* LayoutTests/imported/w3c/web-platform-tests/css/css-images/image-set/image-set-parsing-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/css-images/image-set/image-set-parsing.html:
* LayoutTests/imported/w3c/web-platform-tests/css/css-images/image-set/image-set-zero-resolution-rendering-2-expected.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-images/image-set/image-set-zero-resolution-rendering-2.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-images/image-set/image-set-zero-resolution-rendering-expected.html: Added.
* LayoutTests/imported/w3c/web-platform-tests/css/css-images/image-set/image-set-zero-resolution-rendering.html: Added.

* Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:
(WebCore::CSSPropertyParserHelpers::consumeImageSetOption):
* Source/WebCore/rendering/style/StyleImageSet.cpp:
(WebCore::StyleImageSet::bestImageForScaleFactor):

Canonical link: https://commits.webkit.org/264298@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/image-set-Handle-zero-resolution-and-clamping-resolutions branch from aca75bd to f9cc497 Compare May 21, 2023 01:52
@webkit-commit-queue
Copy link
Collaborator

Committed 264298@main (f9cc497): https://commits.webkit.org/264298@main

Reviewed commits have been landed. Closing PR #13479 and removing active labels.

@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label May 21, 2023
@webkit-commit-queue webkit-commit-queue merged commit f9cc497 into WebKit:main May 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nt1m nt1m

Assignees

@rreno rreno

Labels
CSS Cascading Style Sheets implementation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@rreno @webkit-early-warning-system @webkit-commit-queue @nt1m @webkit-ews-buildbot








ApplySandwichStrip

pFad - (p)hone/(F)rame/(a)nonymizer/(d)eclutterfier!      Saves Data!


--- a PPN by Garber Painting Akron. With Image Size Reduction included!

Fetched URL: http://github.com/WebKit/WebKit/pull/13479

Alternative Proxies:

Alternative Proxy

pFad Proxy

pFad v3 Proxy

pFad v4 Proxy