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

LibWeb: Invalidate an element with CSS custom props on attribute changes #3853

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kmruiz
Copy link

@kmruiz kmruiz commented Mar 7, 2025

This change forces an element to render again when it depends on custom CSS properties on any attribute change. Essentially, CSS custom properties could be attached on an element by using any selector, based on any attribute, so ideally if the current element is using CSS Custom Properties they need to flow down the tree.

It attempts to fix #3838 and there is a test that should validate the new behaviour.

Behaviour with the fix:

output

@ladybird-bot
Copy link
Collaborator

Hello!

One or more of the commit messages in this PR do not match the Ladybird code submission policy, please check the lint_commits CI job for more details on which commits were flagged and why.
Please do not close this PR and open another, instead modify your commit message(s) with git commit --amend and force push those changes to update this PR.

@kmruiz kmruiz force-pushed the fix/render-document-after-parent-changes-class-with-css-vars branch 5 times, most recently from 8985c79 to 01f9c95 Compare March 7, 2025 22:04
@AtkinsSJ
Copy link
Member

AtkinsSJ commented Mar 8, 2025

Your new test is timing out on all the CI actions that run it. Something is up.

@kmruiz
Copy link
Author

kmruiz commented Mar 8, 2025

Yes, apologies! Just found out that the test was broken because it was testing for the colour name instead of the rgb(...) syntax. There was also a typo that was making the test throw an exception, which I guess made the CI timeout.

This commit should fix it.

@InvalidUsernameException
Copy link
Contributor

Please squash the changes from the second commit into the first. PRs in this repository are merged by rebasing the individual commits on master. And each commit that ends up on master should be fully buildable have tests passing. Since your second commit fixes a problem with code introduced in the first commit, this is currently not the case.

@kmruiz kmruiz force-pushed the fix/render-document-after-parent-changes-class-with-css-vars branch from 07a2080 to 38138cf Compare March 8, 2025 12:42
@kmruiz
Copy link
Author

kmruiz commented Mar 8, 2025

Done. Squashed everything into a single commit.

Copy link
Member

@kalenikaliaksandr kalenikaliaksandr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you add explanation why the change is needed from PR description to commit message. currently commit message is duplicate of commit title text.

Element styles should be invalidated when they depend on CSS variables
defined in one of their ancesors or when one of their ancestors change
their class, due to the possibility of adding new CSS variables in
scope.
@kmruiz kmruiz force-pushed the fix/render-document-after-parent-changes-class-with-css-vars branch from 38138cf to 3437c81 Compare March 9, 2025 19:18
@kmruiz
Copy link
Author

kmruiz commented Mar 9, 2025

@kalenikaliaksandr Done! If this message is not clear enough I believe I can tweak it a little bit more.

@kalenikaliaksandr kalenikaliaksandr enabled auto-merge (rebase) March 10, 2025 00:22
@kmruiz
Copy link
Author

kmruiz commented Mar 11, 2025

@kalenikaliaksandr it seems that CI is failing with the new commit, could it be some flaky tests? The code didn't change, only the commit message.

@sideshowbarker
Copy link
Contributor

it seems that CI is failing with the new commit, could it be some flaky tests?

Yeah the CI failure at https://github.com/LadybirdBrowser/ladybird/actions/runs/13751716386/job/38599825023?pr=3853 isn’t unique to this branch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CSS/JS: CSS variables don't propagate properly when dynamically assigning/removing classes
6 participants