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

Implement `visibility: hidden` in Layout 2020 #26841

Closed
SimonSapin opened this issue Jun 10, 2020 · 4 comments
Closed

Implement `visibility: hidden` in Layout 2020 #26841

SimonSapin opened this issue Jun 10, 2020 · 4 comments

Comments

@SimonSapin
Copy link
Member

@SimonSapin SimonSapin commented Jun 10, 2020

Relevant spec: https://drafts.csswg.org/css2/visufx.html#propdef-visibility

Relevant code: https://github.com/servo/servo/blob/master/components/layout_2020/display_list/mod.rs

Changes needed: when generating display items for a BoxFragment or TextFragment, check the value visibility property in its style or parent_style field respectively:

use style::properties::ComputedValues;
use style::properties::longhands::visibility::computed_value::T as Visibility;

fn display_items_are_visible(style: &ComputedValues) -> bool {
    let visible = match style.get_inherited_box().visibility {
        Visibility::Visible => true,
        Visibility::Hidden => false,

        // `visibility: collapse` is disabled at compile time
        // in `components/style/properties/longhands/inherited_box.mako.rs`
        // at the moment, so the variant doesn’t exist.
        // When it is enabled, this code should treat it like `hidden`
        // Visibility::Collapse => false,
    }
}

For non-visible fragments, skip emitting display items for backgrounds, borders, or text. Don’t inhibit rendering of descendant fragments: CSS inheritance already propagates the property to descendant elements, and they can override it to be visible again.

See https://github.com/servo/servo/wiki/Layout-2020#using for building with Layout 2020 enabled, to check that your changes compile.

@highfive
Copy link

@highfive highfive commented Jun 10, 2020

Hi! If you have any questions regarding this issue, feel free to make a comment here, or ask it in the #servo channel in Matrix.

If you intend to work on this issue, then add @highfive: assign me to your comment, and I'll assign this to you. 😄

@SimonSapin
Copy link
Member Author

@SimonSapin SimonSapin commented Jun 10, 2020

        // `visibility: collapse` is disabled at compile time
        // in `components/style/properties/longhands/inherited_box.mako.rs`
        // at the moment, so the variant doesn’t exist.

In fact it should be enabled (and treated as hidden) even if we don’t implement flex item collapsing or table cell collapsing yet. So, additional steps needed:

In components/style/properties/longhands/inherited_box.mako.rs find the definition of the visibility property, remove its servo_2020_pref line, remove its extra_gecko_values line, and add collapse to its list of keyword values (the second argument to helpers.single_keyword, currently "visible hidden",).


… and, new tests will likely pass, so expected results will need to be updated. See https://github.com/servo/servo/wiki/Layout-2020#web-platform-tests for running web-platform-tests locally, but this may take a long time as there are very many tests. It may be easier to submit a pull request and do a "try" run on CI servers.

@MDeiml
Copy link
Contributor

@MDeiml MDeiml commented Jun 13, 2020

@highfive: assign me
I'd like to try this. Seems achievable.

@highfive
Copy link

@highfive highfive commented Jun 13, 2020

Hey @MDeiml! Thanks for your interest in working on this issue. It's now assigned to you!

@highfive highfive added the C-assigned label Jun 13, 2020
@MDeiml MDeiml mentioned this issue Jun 13, 2020
4 of 5 tasks complete
bors-servo added a commit that referenced this issue Jun 13, 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. -->
bors-servo added a commit that referenced this issue Jun 14, 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. -->
bors-servo added a commit that referenced this issue Jun 14, 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. -->
bors-servo added a commit that referenced this issue Jun 14, 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. -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

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