Setting current color to black if canvas is not rendered in document

[craftytrickster]

Fixes #10601

The change seems deceptively easy, I hope I am not missing anything...

---

This change is

[highfive]

Heads up! This PR modifies the following files:

• @KiChjang: components/script/dom/canvasrenderingcontext2d.rs

[highfive]

Warning

• These commits modify script code, but no tests are modified. Please consider adding a test!

[emilio]

The change looks good, but the 2d context spec defines being rendered quite precisely, and it's not what we're doing here:

An element is being rendered if it has any associated CSS layout boxes, SVG layout boxes, or some equivalent in other styling languages.

That is, if the node has an associated layout fragment.

I think testing for presence in the document and the computed value of the display property (checking it's not none) could be enough, over all knowing we already use GetComputedStyle (you can make that change and I'll trigger a try run).

I'm not sure if there's another way for an element not to generate a layout box (and if that's well specified enough). If the condition is more complex, we might need to query layout, but...

I'll check the CSS spec and other browsers' implementation, meanwhile that change is probably saner :)

-S-awaiting-review +S-needs-code-changes

---

Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion, some commit checks broke.

---

components/script/dom/canvasrenderingcontext2d.rs, line 497 [r1] (raw file):
nit: } else {

---

Comments from Reviewable

[craftytrickster]

Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion.

---

components/script/dom/canvasrenderingcontext2d.rs, line 497 [r1] (raw file):
Done.

---

Comments from Reviewable

[craftytrickster]

When I have time later I will try to add the computed value of the display as well.

---

Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion.

---

Comments from Reviewable

[jdm]

display: none is one way to make an element not have a layout box.

[emilio]

@jdm: Yeah my concern is if it's the only one or not (appart from not being in a document) :)

[dzbarsky]

I remember looking at this when I was implementing, and the spec doesn't match what existing browsers do. Don't remember the exact details, but please tread carefully! (and file a spec bug after investigation, like I should have done before)

[craftytrickster]

Assuming that the element_not_rendered implementation is correct, should the logic to get the boolean value be placed somewhere else? If it were placed as an impl method somewhere (not sure which place would be the most appropriate, maybe window?), then the logic could be reused.

[craftytrickster]

Review status: 0 of 1 files reviewed at latest revision, 2 unresolved discussions.

---

components/script/dom/canvasrenderingcontext2d.rs, line 498 [r3] (raw file):
If not_in_doc is true, then there is no need to perform the display_is_none check. I can make the change once I know if this logic will be extracted into its own method or not.

---

Comments from Reviewable

[emilio]

I don't think it's actually needed to extract it in its own method, probably using short-circuiting or would suffice :)

Let's check if this matches what the 2dcontext test suite expects :)

@bors-servo: try

[bors-servo]

⌛ Trying commit 7d08bee with merge d745dd1...

[bors-servo]

💔 Test failed - linux-rel

[craftytrickster]

The test says it fails, yet if I run the test, it do see the canvas getting set to black. Is this an implementation error or does the test itself need to be changed?

[KiChjang]

@craftytrickster The test says it PASSES, doesn't it? It's just expected to FAIL instead.

Ran 4676 tests finished in 359.0 seconds.
  • 4675 ran as expected. 1363 tests skipped.
  • 1 tests had unexpected subtest results

Tests with unexpected results:
  ▶ Unexpected subtest result in /2dcontext/fill-and-stroke-styles/2d.fillStyle.parse.current.removed.html:
  └ PASS [expected FAIL] currentColor is solid black when the canvas element is not in a document

[craftytrickster]

@KiChjang I see that too, but it says that the expected output is a black rectangle. Prior to the change the output was a red rectangle, but now it is indeed black, so why would it be considered a failure? Is it possible it was just set to expect a failure to pass CI?

[KiChjang]

@craftytrickster Nonononono, the test is currently passing, but our test suite expects it to fail. What you need to do in this case is to change the test expectations, i.e. remove this file.

[craftytrickster]

@KiChjang thanks for the pointer on the .ini file, I have removed it as suggested.

[Ms2ger]

Right now, we only have a test for the is_in_doc check, so I'd like you to add a test where:

1. the canvas element is in the document but it is display:none
2. the canvas element is in the document but its parent is display:none

and check what other browsers do there.

[craftytrickster]

whatwg/html#1099 affects this PR.

[Ms2ger]

Let's make this

Ok(RGBA {
    red: 0.0,
    green: 0.0,
    blue: 0.0,
    alpha: 0.0,
})

[craftytrickster]

Done, although I made alpha 1 to correspond to "black"

[Ms2ger]

@bors-servo r+

[bors-servo]

📌 Commit b5159dc has been approved by Ms2ger

[bors-servo]

⌛ Testing commit b5159dc with merge 3c8e401...

[bors-servo]

💔 Test failed - mac-rel-wpt

[highfive]

  ▶ FAIL [expected PASS] /_mozilla/mozilla/sslfail.html
  └   → /_mozilla/mozilla/sslfail.html 9d627658adfcf6a768983b0fbae728aa4edab3e0
/_mozilla/mozilla/sslfail-ref.html 0a7088f19a748be896762e4a5cbb67467342f6e8
Testing 9d627658adfcf6a768983b0fbae728aa4edab3e0 == 0a7088f19a748be896762e4a5cbb67467342f6e8

[wafflespeanut]

@bors-servo retry #10760

[bors-servo]

⌛ Testing commit b5159dc with merge 4da6855...

[bors-servo]

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt