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: Missing layout updates causes failed subtest in css-masking/animations/clip-interpolation #3087

Open
shannonbooth opened this issue Dec 31, 2024 · 2 comments

Comments

@shannonbooth
Copy link
Contributor

shannonbooth commented Dec 31, 2024

To reproduce, add a very updatey call to update_layout() inthe HTMLElement attribute changed steps:

diff --git a/Libraries/LibWeb/HTML/HTMLElement.cpp b/Libraries/LibWeb/HTML/HTMLElement.cpp
index 7aaaf60f76c..892dc141f65 100644
--- a/Libraries/LibWeb/HTML/HTMLElement.cpp
+++ b/Libraries/LibWeb/HTML/HTMLElement.cpp
@@ -598,6 +598,8 @@ void HTMLElement::attribute_changed(FlyString const& name, Optional<String> cons
     Base::attribute_changed(name, old_value, value, namespace_);
     HTMLOrSVGElement::attribute_changed(name, old_value, value, namespace_);

+    document().update_layout();
+
     if (name == HTML::AttributeNames::contenteditable) {
         if (!value.has_value()) {
             // No value maps to the "inherit" state.

And observe that one extra test passes in:

--- Tests/LibWeb/Text/expected/./wpt-import/css/css-masking/animations/clip-interpolation.txt
+++ Tests/LibWeb/Text/expected/./wpt-import/css/css-masking/animations/clip-interpolation.txt
@@ -2,8 +2,8 @@

 Found 324 tests

-248 Pass
-76 Fail
+249 Pass
+75 Fail
 Fail   CSS Transitions: property <clip> from neutral to [rect(20px, 20px, 20px, 20px)] at (-1) should be [rect(-20px 180px -20px 180px)]
 Fail   CSS Transitions: property <clip> from neutral to [rect(20px, 20px, 20px, 20px)] at (0) should be [rect(0px 100px 0px 100px)]
 Fail   CSS Transitions: property <clip> from neutral to [rect(20px, 20px, 20px, 20px)] at (0.25) should be [rect(5px 80px 5px 80px)]
@@ -21,7 +21,7 @@
 Fail   CSS Animations: property <clip> from neutral to [rect(20px, 20px, 20px, 20px)] at (0.25) should be [rect(5px 80px 5px 80px)]
 Fail   CSS Animations: property <clip> from neutral to [rect(20px, 20px, 20px, 20px)] at (0.75) should be [rect(15px 40px 15px 40px)]
 Pass   CSS Animations: property <clip> from neutral to [rect(20px, 20px, 20px, 20px)] at (1) should be [rect(20px 20px 20px 20px)]
-Fail   CSS Animations: property <clip> from neutral to [rect(20px, 20px, 20px, 20px)] at (2) should be [rect(40px -60px 40px -60px)]
+Pass   CSS Animations: property <clip> from neutral to [rect(20px, 20px, 20px, 20px)] at (2) should be [rect(40px -60px 40px -60px)]
 Fail   Web Animations: property <clip> from neutral to [rect(20px, 20px, 20px, 20px)] at (-1) should be [rect(-20px 180px -20px 180px)]
 Fail   Web Animations: property <clip> from neutral to [rect(20px, 20px, 20px, 20px)] at (0) should be [rect(0px 100px 0px 100px)]
 Fail   Web Animations: property <clip> from neutral to [rect(20px, 20px, 20px, 20px)] at (0.25) should be [rect(5px 80px 5px 80px)]
@awesomekling
Copy link
Member

Sounds like whichever API is producing the incorrect result needs to do a bit of update_layout(). Doing it on every attribute change is obviously excessive :)

@shannonbooth
Copy link
Contributor Author

Yeah, that's why I reported the issue, I haven't spent the time digging into what is missing it 😆 I just happened to notice it from that.

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

No branches or pull requests

2 participants