Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upImplement visibility for layout_2020 #26909
Conversation
highfive
commented
Jun 13, 2020
|
Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @SimonSapin (or someone else) soon. |
highfive
commented
Jun 13, 2020
|
Heads up! This PR modifies the following files:
|
highfive
commented
Jun 13, 2020
|
|
|
@bors-servo try=wpt-2020 |
|
@MDeiml: |
|
@bors-servo try=wpt |
Implement visibility for layout_2020 <!-- Please describe your changes on the following line: --> Implement the 'visibility: hidden' (and 'visibility: collapse') css properties. --- <!-- 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 #26841 (GitHub issue number if applicable) <!-- Either: --> - [X] There are tests for these changes OR - [ ] These changes do not require tests because ___ <!-- 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 retry try=wpt-2020 |
Implement visibility for layout_2020 <!-- Please describe your changes on the following line: --> Implement the 'visibility: hidden' (and 'visibility: collapse') css properties. --- <!-- 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 #26841 (GitHub issue number if applicable) <!-- Either: --> - [X] There are tests for these changes OR - [ ] These changes do not require tests because ___ <!-- 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. -->
|
|
|
These tests need to be updated (using './mach update-wpt'), but I'm not sure how I'm supposed to do that. If I just use the log from the tests it also disables the intermittents which have nothing to do with this change. That can't be right? |
|
Hi @MDeiml. Your patch looks good. It also disables hit testing (which answers questions like "what elements are under this mouse cursor?") for hidden elements and I wasn’t sure that it should. Unfortunately we don’t have a specification for hit testing, so the next best thing is looking at other engines. And indeed, For updating expected results, For tests already marked as intermittent in a way it doesn’t matter because CI ignores their result for the purpose of deciding wether to merge a PR. Still, to make a PR cleaner we can look at … and by the time I typed this up it looks like you’ve already managed :) @bors-servo r+ |
|
|
Implement visibility for layout_2020 <!-- Please describe your changes on the following line: --> Implement the 'visibility: hidden' (and 'visibility: collapse') css properties. --- <!-- 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 #26841 (GitHub issue number if applicable) <!-- Either: --> - [X] There are tests for these changes OR - [ ] These changes do not require tests because ___ <!-- 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. -->
|
Thanks for explaining it anyways |
|
|
|
Implement visibility for layout_2020 <!-- Please describe your changes on the following line: --> Implement the 'visibility: hidden' (and 'visibility: collapse') css properties. --- <!-- 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 #26841 (GitHub issue number if applicable) <!-- Either: --> - [X] There are tests for these changes OR - [ ] These changes do not require tests because ___ <!-- 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. -->
|
Uh it looks like editing my comment that contains the "r+" command made Homu/Bors interpret that as a new command, which is arguably a bug, and then despite commenting "no need to approve again" it made and pushed a second merge commit triggering a second CI run, which is another bug. |
|
|
|
Ah this patch also adds |
|
@bors-servo r+ |
|
|
|
|
MDeiml commentedJun 13, 2020
Implement the 'visibility: hidden' (and 'visibility: collapse') css properties.
./mach build -ddoes not report any errors./mach test-tidydoes not report any errors