Skip to content
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.

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

Make CanvasRenderingContext2D.createPattern() return null for incomplete images #25710

Merged
merged 4 commits into from Feb 11, 2020

Conversation

@pylbrecht
Copy link
Contributor

pylbrecht commented Feb 7, 2020

createPattern() should return null if the passed image argument is not usable.

References:


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix part of #25331
  • There are tests for these changes
@highfive
Copy link

highfive commented Feb 7, 2020

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/canvas_state.rs, components/script/dom/offscreencanvasrenderingcontext2d.rs, components/script/dom/htmlimageelement.rs, components/script/dom/paintrenderingcontext2d.rs, components/script/dom/offscreencanvas.rs and 2 more
  • @KiChjang: components/script/canvas_state.rs, components/script/dom/offscreencanvasrenderingcontext2d.rs, components/script/dom/htmlimageelement.rs, components/script/dom/paintrenderingcontext2d.rs, components/script/dom/offscreencanvas.rs and 2 more
@pylbrecht
Copy link
Contributor Author

pylbrecht commented Feb 7, 2020

These changes are based on #25675.

@pylbrecht
Copy link
Contributor Author

pylbrecht commented Feb 7, 2020

@bors-servo try=wpt

@bors-servo
Copy link
Contributor

bors-servo commented Feb 7, 2020

Trying commit 52e1a73 with merge eebd043...

bors-servo added a commit that referenced this pull request Feb 7, 2020
Make CanvasRenderingContext2D.createPattern() return null for incomplete images

<!-- Please describe your changes on the following line: -->
`createPattern()` should return `null` if the passed `image` argument is not usable.

References:
- https://html.spec.whatwg.org/multipage/canvas.html#dom-context-2d-createpattern
- https://html.spec.whatwg.org/multipage/canvas.html#check-the-usability-of-the-image-argument
---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix part of #25331

<!-- Either: -->
- [x] There are tests for these changes

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
@pylbrecht
Copy link
Contributor Author

pylbrecht commented Feb 7, 2020

r? @jdm

@highfive highfive assigned jdm and unassigned asajeffrey Feb 7, 2020
@bors-servo
Copy link
Contributor

bors-servo commented Feb 7, 2020

💔 Test failed - status-taskcluster

@pshaughn
Copy link
Member

pshaughn commented Feb 7, 2020

  ▶ Unexpected subtest result in /2dcontext/fill-and-stroke-styles/2d.pattern.image.broken.html:
  │ FAIL [expected PASS] Canvas test: 2d.pattern.image.broken
  │   → assert_throws_dom: function "function() { ctx.createPattern(img, 'repeat'); }" did not throw
  │ 
  │ @http://web-platform.test:8000/2dcontext/fill-and-stroke-styles/2d.pattern.image.broken.html:23:1
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:2024:25
  │ _addTest/</<@http://web-platform.test:8000/2dcontext/resources/canvas-tests.js:66:15
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:2024:25
  └ _addTest/<@http://web-platform.test:8000/2dcontext/resources/canvas-tests.js:63:11

Looks like this test is expecting a throw on an image it thinks is broken, and possibly getting the new null instead?
No one else is passing either, so probably the spec is right and the test is wrong https://wpt.fyi/results/2dcontext/fill-and-stroke-styles/2d.pattern.image.broken.html?label=experimental&label=master&aligned

@pylbrecht
Copy link
Contributor Author

pylbrecht commented Feb 8, 2020

is_usable() somehow returns bad, when it shouldn't, because create_pattern() returns before actually trying to fetch the image data, which should fail for broken.png and therefore lead to the desired result. Maybe we need to incorporate fetching the image data into is_usable() somehow.

It would be very helpful to know how broken.png is broken, so we can figure out why our checks fail.
I guess today is about looking at the image's bytes and learning how PNG works.

@pylbrecht
Copy link
Contributor Author

pylbrecht commented Feb 8, 2020

No one else is passing either, so probably the spec is right and the test is wrong https://wpt.fyi/results/2dcontext/fill-and-stroke-styles/2d.pattern.image.broken.html?label=experimental&label=master&aligned

I've never considered this an option. It would spare me the debugging though.

@pylbrecht pylbrecht force-pushed the pylbrecht:pattern.incomplete branch from 52e1a73 to 0d83e54 Feb 8, 2020
@highfive highfive removed the S-tests-failed label Feb 8, 2020
@pylbrecht
Copy link
Contributor Author

pylbrecht commented Feb 8, 2020

@bors-servo retry

@pylbrecht
Copy link
Contributor Author

pylbrecht commented Feb 8, 2020

@bors-servo try=wpt

@bors-servo
Copy link
Contributor

bors-servo commented Feb 8, 2020

Trying commit 0d83e54 with merge 566053e...

bors-servo added a commit that referenced this pull request Feb 8, 2020
Make CanvasRenderingContext2D.createPattern() return null for incomplete images

<!-- Please describe your changes on the following line: -->
`createPattern()` should return `null` if the passed `image` argument is not usable.

References:
- https://html.spec.whatwg.org/multipage/canvas.html#dom-context-2d-createpattern
- https://html.spec.whatwg.org/multipage/canvas.html#check-the-usability-of-the-image-argument
---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix part of #25331

<!-- Either: -->
- [x] There are tests for these changes

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
@pylbrecht
Copy link
Contributor Author

pylbrecht commented Feb 8, 2020

I wasn't checking for a broken state in case the current_request's image attribute is None. Hopefully the rewrite will still pass all the tests. 🤞

@bors-servo
Copy link
Contributor

bors-servo commented Feb 8, 2020

💔 Test failed - status-taskcluster

@pylbrecht
Copy link
Contributor Author

pylbrecht commented Feb 8, 2020

1 unexpected results that are NOT known-intermittents:
  ▶ Unexpected subtest result in /2dcontext/drawing-images-to-the-canvas/2d.drawImage.broken.html:
  │ FAIL [expected PASS] Canvas test: 2d.drawImage.broken
  └   → The object is in an invalid state.

This is weird. It seems createPattern() and drawImage() need to be handled differently, although the spec links to the same usability checks.

From the spec of drawImage():

  1. If any of the arguments are infinite or NaN, then return
  2. Let usability be the result of checking the usability of image.
  3. If usability is bad, then return (without drawing anything).

From the spec of createPattern():

  1. Let usability be the result of checking the usability of image.
  2. If result is bad, then return null.

From the spec of checking the usability of the image argument:

If image's current request's state is broken, then throw an "InvalidStateError" DOMException.
If image is not fully decodable, then return bad.
If image has an intrinsic width or intrinsic height (or both) equal to zero, then return bad.

To me it looks like /2dcontext/drawing-images-to-the-canvas/2d.drawImage.broken.html should raise an InvalidStateError instead of drawing nothing. I filed an issue in the wpt repo: web-platform-tests/wpt#21683

@pylbrecht
Copy link
Contributor Author

pylbrecht commented Feb 9, 2020

Since looking at other browsers' behavior for reference seems the common thing to do, I'm a little lost on how to proceed.
Should we fail this test (like most other browsers) in favor of passing the drawImage() equivalent? Or is implementing two different usability checks for createPattern() and drawImage() the way to go?

@pylbrecht
Copy link
Contributor Author

pylbrecht commented Feb 10, 2020

It seems 2d.pattern.broken.image.html is wrong, so I will roll back to the previous changes that fail this test, but pass 2d.drawImage.broken.html.
See https://github.com/guest271314/wpt/pull/10 for reference.

@pylbrecht
Copy link
Contributor Author

pylbrecht commented Feb 10, 2020

@bors-servo try=wpt

@bors-servo
Copy link
Contributor

bors-servo commented Feb 10, 2020

Trying commit 70b2d31 with merge a567f6c...

bors-servo added a commit that referenced this pull request Feb 10, 2020
Make CanvasRenderingContext2D.createPattern() return null for incomplete images

<!-- Please describe your changes on the following line: -->
`createPattern()` should return `null` if the passed `image` argument is not usable.

References:
- https://html.spec.whatwg.org/multipage/canvas.html#dom-context-2d-createpattern
- https://html.spec.whatwg.org/multipage/canvas.html#check-the-usability-of-the-image-argument
---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix part of #25331

<!-- Either: -->
- [x] There are tests for these changes

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
@bors-servo
Copy link
Contributor

bors-servo commented Feb 10, 2020

☀️ Test successful - status-taskcluster
State: approved= try=True

@jdm
Copy link
Member

jdm commented Feb 10, 2020

@bors-servo r+
Looks like a good improvement!

@bors-servo
Copy link
Contributor

bors-servo commented Feb 10, 2020

📌 Commit 70b2d31 has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Feb 10, 2020

Testing commit 70b2d31 with merge 9c9dc35...

bors-servo added a commit that referenced this pull request Feb 10, 2020
Make CanvasRenderingContext2D.createPattern() return null for incomplete images

<!-- Please describe your changes on the following line: -->
`createPattern()` should return `null` if the passed `image` argument is not usable.

References:
- https://html.spec.whatwg.org/multipage/canvas.html#dom-context-2d-createpattern
- https://html.spec.whatwg.org/multipage/canvas.html#check-the-usability-of-the-image-argument
---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix part of #25331

<!-- Either: -->
- [x] There are tests for these changes

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
@bors-servo
Copy link
Contributor

bors-servo commented Feb 11, 2020

💔 Test failed - status-taskcluster

@jdm
Copy link
Member

jdm commented Feb 11, 2020

@bors-servo
Copy link
Contributor

bors-servo commented Feb 11, 2020

Testing commit 70b2d31 with merge 886edc6...

bors-servo added a commit that referenced this pull request Feb 11, 2020
Make CanvasRenderingContext2D.createPattern() return null for incomplete images

<!-- Please describe your changes on the following line: -->
`createPattern()` should return `null` if the passed `image` argument is not usable.

References:
- https://html.spec.whatwg.org/multipage/canvas.html#dom-context-2d-createpattern
- https://html.spec.whatwg.org/multipage/canvas.html#check-the-usability-of-the-image-argument
---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix part of #25331

<!-- Either: -->
- [x] There are tests for these changes

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
@bors-servo
Copy link
Contributor

bors-servo commented Feb 11, 2020

💔 Test failed - status-taskcluster

@jdm
Copy link
Member

jdm commented Feb 11, 2020

@bors-servo
Copy link
Contributor

bors-servo commented Feb 11, 2020

Testing commit 70b2d31 with merge 55058b2...

bors-servo added a commit that referenced this pull request Feb 11, 2020
Make CanvasRenderingContext2D.createPattern() return null for incomplete images

<!-- Please describe your changes on the following line: -->
`createPattern()` should return `null` if the passed `image` argument is not usable.

References:
- https://html.spec.whatwg.org/multipage/canvas.html#dom-context-2d-createpattern
- https://html.spec.whatwg.org/multipage/canvas.html#check-the-usability-of-the-image-argument
---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix part of #25331

<!-- Either: -->
- [x] There are tests for these changes

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
@bors-servo
Copy link
Contributor

bors-servo commented Feb 11, 2020

☀️ Test successful - status-taskcluster
Approved by: jdm
Pushing 55058b2 to master...

@bors-servo bors-servo merged commit 70b2d31 into servo:master Feb 11, 2020
2 checks passed
2 checks passed
Community-TC (pull_request) TaskGroup: success
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.