LibWeb: Support content-visibility css + resolve fixme #267
Conversation
|
I'll try and look at this later today. I think I spotted a couple of small issues but I've only had time to look through it on my phone. 😅 |
| "valid-identifiers": [ | ||
| "visible", | ||
| "auto", | ||
| "hidden" | ||
| ] |
There was a problem hiding this comment.
You already defined an enum for these, so you can use it here:
| "valid-identifiers": [ | |
| "visible", | |
| "auto", | |
| "hidden" | |
| ] | |
| "valid-types": [ | |
| "content-visibility" | |
| ] |
| @@ -338,7 +338,7 @@ void TreeBuilder::create_layout_tree(DOM::Node& dom_node, TreeBuilder::Context& | |||
| VERIFY(!element.needs_style_update()); | |||
| style = element.computed_css_values(); | |||
| display = style->display(); | |||
| if (display.is_none()) | |||
| if (display.is_none() || element.has_parent_with_content_visibility_hidden()) | |||
There was a problem hiding this comment.
This seems like the wrong approach. Instead of recursing into the children, then walking back up to the parent, to decide whether to generate layout boxes for them, we could just not consider the children if the current element has content-visibility: hidden.
Further down in this function there's a check for dom_node.has_children(), and that seems like a good place to add a check for style->content_visibility() and skip the children altogether. You might also need to adjust the checks for adding pseudo-elements, as I'm not sure if they're affected by content-visibility.
(Then the whole has_parent_with_content_visibility_hidden() method can be removed.)
There was a problem hiding this comment.
That is a lot better yes, I was actually looking for something like that but I totally overlooked that part 😅 Let me know if I implemented it correctly now.
And yes, pseudo elements should also be invisible, I've added the check there too.
| padding: 16px; | ||
| content-visibility: hidden; | ||
| } | ||
| </style><div><span>I am not visible |
There was a problem hiding this comment.
I think it's worth also testing that text content directly inside the <div> is also hidden. eg:
| </style><div><span>I am not visible | |
| </style><div>I am not visible <span>and nor am I |
Maybe also have CSS that gives div::before some content just to make sure that doesn't appear in the layout either.
There was a problem hiding this comment.
Yes, good idea!
EdwinHoksberg
force-pushed
the
css-content-visibility
branch
from
3968698 to
35400ee
Compare
EdwinHoksberg
force-pushed
the
css-content-visibility
branch
from
35400ee to
f498be5
Compare
EdwinHoksberg
force-pushed
the
css-content-visibility
branch
from
f498be5 to
360749d
Compare
No description provided.