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

Correctly paint the CSS canvas’ background #26414

Merged
merged 8 commits into from May 15, 2020
Merged

Correctly paint the CSS canvas’ background #26414

merged 8 commits into from May 15, 2020

Conversation

@SimonSapin
Copy link
Member

SimonSapin commented May 4, 2020

https://drafts.csswg.org/css-backgrounds/#special-backgrounds

Fixes #25559
Closes #26121, as it is an alternative.
Fixes #26444.

@highfive
Copy link

highfive commented May 4, 2020

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/node.rs
  • @jgraham: tests/wpt/reftests-report/gen.py, tests/wpt/reftests-report/prism.css, tests/wpt/reftests-report/prism.js
  • @KiChjang: components/script/dom/node.rs
  • @emilio: components/style/values/specified/box.rs
@SimonSapin
Copy link
Member Author

SimonSapin commented May 4, 2020

At the moment this PR has the same test expectation changes as #26121. It passes a couple more tests, but has some remaining regressions on my machine.

@bors-servo try=wpt-2020

  • These two tests dynamically set display: none to the root element. They hang without output, I haven’t investigated them yet.

    ▶ TIMEOUT [expected PASS] /css/css-backgrounds/background-color-root-propagation-002.html
    ▶ TIMEOUT [expected PASS] /css/css-backgrounds/background-color-body-propagation-006.html
    
  • A transform on the root element is not applied to the positioning area of the canvas’s background images. @mrobinson Would calling build_reference_frame_if_necessary on the root StackingContext be appropriate here?

    Also, the painting area is a rectangle in the top-level (un-transformed) coordinate space, but the display item’s bounds rect is in local (transformed) coordinates. So we may need to take the axis-aligned bounding box of the inverse-transformed rect.

    ▶ FAIL [expected PASS] /css/css-transforms/transform-translate-background-002.html
    ▶ FAIL [expected PASS] /css/css-transforms/transform-translate-background-001.html
    
@bors-servo
Copy link
Contributor

bors-servo commented May 4, 2020

Trying commit 70f2ead with merge 5454e9b...

bors-servo added a commit that referenced this pull request May 4, 2020
Correctly paint the CSS canvas’ background

https://drafts.csswg.org/css-backgrounds/#special-backgrounds

Fixes #25559
Closes #26121, as it is an alternative.
@atouchet atouchet added this to Architecture in Layout 2020 May 4, 2020
@SimonSapin SimonSapin mentioned this pull request May 4, 2020
4 of 5 tasks complete
@bors-servo
Copy link
Contributor

bors-servo commented May 4, 2020

💔 Test failed - status-taskcluster

@mrobinson
Copy link
Member

mrobinson commented May 5, 2020

A transform on the root element is not applied to the positioning area of the canvas’s background images. @mrobinson Would calling build_reference_frame_if_necessary on the root StackingContext be appropriate here?

This depends: Is the other content on the page transformed? If it is, the correct solution would likely be to assign the background display item the correct reference frame as its spatial node. If the rest of the page content is not transformed (but should be), then I think appropriate thing would be to create a new reference frame at the the top level.

@jdm jdm mentioned this pull request May 6, 2020
@bors-servo
Copy link
Contributor

bors-servo commented May 6, 2020

The latest upstream changes (presumably #26407) made this pull request unmergeable. Please resolve the merge conflicts.

@SimonSapin SimonSapin force-pushed the canvas-background2 branch from 70f2ead to ed27ba1 May 7, 2020
@highfive highfive removed the S-tests-failed label May 7, 2020
@SimonSapin SimonSapin force-pushed the canvas-background2 branch from ed27ba1 to 0a39c6a May 7, 2020
@SimonSapin
Copy link
Member Author

SimonSapin commented May 7, 2020

@bors-servo try=wpt-2020

@bors-servo
Copy link
Contributor

bors-servo commented May 7, 2020

Trying commit 0a39c6a with merge ce9c20f...

bors-servo added a commit that referenced this pull request May 7, 2020
Correctly paint the CSS canvas’ background

https://drafts.csswg.org/css-backgrounds/#special-backgrounds

Fixes #25559
Closes #26121, as it is an alternative.
Fixes #26444.
@SimonSapin SimonSapin force-pushed the canvas-background2 branch from 0a39c6a to 4bddac9 May 7, 2020
@SimonSapin SimonSapin marked this pull request as ready for review May 7, 2020
@bors-servo
Copy link
Contributor

bors-servo commented May 11, 2020

The latest upstream changes (presumably #26477) made this pull request unmergeable. Please resolve the merge conflicts.

@jdm
Copy link
Member

jdm commented May 11, 2020

@mrobinson Do you want to review Simon's change here?

@SimonSapin SimonSapin force-pushed the canvas-background2 branch from 761318a to a024ecc May 15, 2020
@SimonSapin
Copy link
Member Author

SimonSapin commented May 15, 2020

However in doing so I seem to have broken transforms (many WPT failures) so that still needs investigating.

Alright, you gave me the correct lead on Matrix to fix this bug. This uncovered another pre-existing bug, for which I added a separate commit. And I also introduced another bug in passing, now fixed as well. I think this should be in pretty good shape now.

@bors-servo try=wpt-2020

@bors-servo
Copy link
Contributor

bors-servo commented May 15, 2020

Trying commit a024ecc with merge 13011ee...

bors-servo added a commit that referenced this pull request May 15, 2020
Correctly paint the CSS canvas’ background

https://drafts.csswg.org/css-backgrounds/#special-backgrounds

Fixes #25559
Closes #26121, as it is an alternative.
Fixes #26444.
@bors-servo
Copy link
Contributor

bors-servo commented May 15, 2020

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

Copy link
Member

mrobinson left a comment

Thanks! These changes look good.

@SimonSapin
Copy link
Member Author

SimonSapin commented May 15, 2020

@bors-servo r=mrobinson

@bors-servo
Copy link
Contributor

bors-servo commented May 15, 2020

📌 Commit a024ecc has been approved by mrobinson

@bors-servo
Copy link
Contributor

bors-servo commented May 15, 2020

Testing commit a024ecc with merge 7d7e987...

@bors-servo
Copy link
Contributor

bors-servo commented May 15, 2020

☀️ Test successful - status-taskcluster
Approved by: mrobinson
Pushing 7d7e987 to master...

@bors-servo bors-servo merged commit 7d7e987 into master May 15, 2020
2 checks passed
2 checks passed
Community-TC (pull_request) TaskGroup: success
Details
homu Test successful
Details
Layout 2020 automation moved this from Architecture to Merged / resolved May 15, 2020
@bors-servo bors-servo deleted the canvas-background2 branch May 15, 2020
bors-servo added a commit that referenced this pull request Jun 18, 2020
Adapt layout 2020 viewer to new BoxTree and FragmentTree structs

- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors

#26414 introduced some changes to how we represent the box and fragment trees, so we need to adapt the layout viewer to support them.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Layout 2020
  
Merged / resolved
Linked issues

Successfully merging this pull request may close these issues.

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