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 uplayout_2020: Implement support for backface-visibility #26537
Conversation
| // TODO(mrobinson): We should pass the hit test information here and not create | ||
| // so many hit test display items. |
This comment has been minimized.
This comment has been minimized.
SimonSapin
May 15, 2020
Member
The problem is that we want hit testing to be always present for each fragment, and always based on the border edge of the fragment. Some fragments do not have a border or background and thus do not generate a display item other than the hit-testing one. Some have backgrounds, but with the background-clip property such that the rectangle is not the one appropriate for hit testing.
We can still reduce the number of display items dedicated to hit testing when there happens to be another display item with the appropriate rectangle, but it’s not as straightforward as this comment suggests. Please reword it to indicate that.
This comment has been minimized.
This comment has been minimized.
| if self.get_box().backface_visibility == BackfaceVisiblity::Visible { | ||
| wr::PrimitiveFlags::default() | ||
| } else { | ||
| wr::PrimitiveFlags::empty() | ||
| } |
This comment has been minimized.
This comment has been minimized.
SimonSapin
May 15, 2020
Member
Nit: please use match instead of == so that the compiler reminds us to update this code in case a new variant is ever added to the enum. (I suspect it is very unlikely that the backface-visibility property will ever get a new value, but making more common a pattern that can help elsewhere seems good.)
This comment has been minimized.
This comment has been minimized.
|
Thanks for the review. I've pushed a new version of the branch that addresses the issue you've identified. |
|
Looks good, thanks! @bors-servo r+ |
|
|
layout_2020: Implement support for backface-visibility <!-- Please describe your changes on the following line: --> --- <!-- 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] 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. -->
|
|
I'll fix the expectation and submit this again. |
|
@bors-servo r=SimonSapin |
|
|
|
|
|
@bors-servo r=SimonSapin |
|
|
layout_2020: Implement support for backface-visibility <!-- Please describe your changes on the following line: --> --- <!-- 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] 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 r=SimonSapin |
|
|
|
|
mrobinson commentedMay 15, 2020
./mach build -ddoes not report any errors./mach test-tidydoes not report any errors