css inset should work on Layout 2020
#29810
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mrobinson
changed the title
inset should work on servo-2020inset should work on Layout 2020
|
@bors-servo try=wpt-2020 |
bors-servo
added a commit
that referenced
this pull request
May 29, 2023
css `inset` should work on Layout 2020 <!-- Please describe your changes on the following line: --> `top`, `bottom`, `left`, `right`, are already implemented in layout-2020, so adding the shorthand can be enabled. --- <!-- 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 #29705 (GitHub issue number if applicable) <!-- Either: --> - [ ] There are tests for these changes OR - [ ] These changes do not require tests because ___ I'm don't know if I need tests for this, how do I determine that? <!-- 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. -->
|
☀️ Test successful - checks-github |
|
It's very odd that this didn't change any test results. 🤔 |
|
There are two other inset shorthands in that file. Should they be updated as well? |
|
@jdm don't think they're implemented in layout-2020, but not sure |
|
@MendyBerger The logical @mrobinson Enabling the 3 shorthands should improve |
|
Thanks @Loirooriol, I've updated the PR. Just out of curiosity, how did you know that it's implemented? Doing a global search didn't return any results, so I first assumed it's not implemented. But then a manual test confirmed that you're right. |
|
I saw servo/components/style/properties/longhands/position.mako.rs Lines 25 to 37 in 13e1aeb and tried a testcase. |
Loirooriol
reviewed
Jun 1, 2023
There was a problem hiding this comment.
Please update the test expectations, css-logical is no longer skipped.
These are improving:
- /css/css-logical/logical-box-inset.html
- /css/css-logical/parsing/inset-block-inline-valid.html
- /css/css-logical/parsing/inset-block-inline-shorthand.html
- /css/css-logical/parsing/inset-shorthand.html
- /css/css-logical/parsing/inset-valid.html
And this crashes :(
- /css/css-logical/animation-002.html
It's hitting
Maybe there is some problem with pending-substitution values:
<!DOCTYPE html>
<style>
:root {--200px: 200px; --300px: 300px}
@keyframes anim{
from { inset-block: var(--200px) }
to { inset-block: var(--300px) }
}
</style>
<div style="animation: anim 10s -5s paused linear; width: 0px; height: 0px;"></div>Can you investigate if there is an easy fix? Or otherwise go back to just enabling inset I guess, since it doesn't seem to have this problem.
|
Note: the The problem seems to be that Any thoughts appreciated. This is tough but I'll continue to investigate. |
|
@Loirooriol does it make sense to merge like this even with this issue since it's not specific to inset-inline/block? It also happens to padding-inline/block and margin-inline/block. |
|
I've opened a separate issue for the inline/block crash issue #29891 |
|
Adding more crashes isn't great, but since it's already crashing for the other logical shorthands then I guess landing is fine. |
|
@bors-servo try=wpt-2020 So I guess let's just run the bot and then please update test expectations. |
bors-servo
added a commit
that referenced
this pull request
Jun 20, 2023
css `inset` should work on Layout 2020 <!-- Please describe your changes on the following line: --> `top`, `bottom`, `left`, `right`, are already implemented in layout-2020, so adding the shorthand can be enabled. --- <!-- 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 #29705 (GitHub issue number if applicable) <!-- Either: --> - [ ] There are tests for these changes OR - [ ] These changes do not require tests because ___ I'm don't know if I need tests for this, how do I determine that? <!-- 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. -->
|
Test results for linux-wpt-layout-2020 from try job (#5324125609): Flaky unexpected result (1)
Stable unexpected results that are known to be intermittent (1)
Stable unexpected results (7)
|
|
💔 Test failed - checks-github |
|
@bors-servo try=wpt-2020 |
bors-servo
added a commit
that referenced
this pull request
Jun 26, 2023
css `inset` should work on Layout 2020 <!-- Please describe your changes on the following line: --> `top`, `bottom`, `left`, `right`, are already implemented in layout-2020, so adding the shorthand can be enabled. --- <!-- 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 #29705 (GitHub issue number if applicable) <!-- Either: --> - [ ] There are tests for these changes OR - [ ] These changes do not require tests because ___ I'm don't know if I need tests for this, how do I determine that? <!-- 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. -->
|
Test results for linux-wpt-layout-2020 from try job (#5382519710): Flaky unexpected result (2)
|
|
☀️ Test successful - checks-github |
|
@bors-servo r+ |
|
📌 Commit 458df8c has been approved by |
|
Test results for linux-wpt-layout-2013 from try job (#5382906740): Flaky unexpected result (15)
Stable unexpected results that are known to be intermittent (6)
|
|
Test results for linux-wpt-layout-2020 from try job (#5382906740): |
|
☀️ Test successful - checks-github |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
top,bottom,left,right, are already implemented in layout-2020, so adding the shorthand can be enabled../mach build -ddoes not report any errors./mach test-tidydoes not report any errorsI'm don't know if I need tests for this, how do I determine that?