Skip to content

LibWeb: ignore repeat(auto-fit/auto-fill, auto) as it is not allowed - #23155

Merged
kalenikaliaksandr merged 1 commit into
SerenityOS:masterfrom
pixelspark:fix/23154-repeat-auto-grid
Feb 11, 2024
Merged

kalenikaliaksandr merged 1 commit into
SerenityOS:masterfrom
pixelspark:fix/23154-repeat-auto-grid

Conversation

@pixelspark

Copy link
Copy Markdown
Contributor

Fixes #23154

@pixelspark
pixelspark requested a review from AtkinsSJ as a code owner February 11, 2024 13:46
@github-actions github-actions Bot added the 👀 pr-needs-review PR needs review from a maintainer or community member label Feb 11, 2024
@BuggieBot

Copy link
Copy Markdown
Member

Hello!

One or more of the commit messages in this PR do not match the SerenityOS 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.

@pixelspark
pixelspark force-pushed the fix/23154-repeat-auto-grid branch 2 times, most recently from 9d170d7 to 326c0f2 Compare February 11, 2024 13:49
@kalenikaliaksandr

Copy link
Copy Markdown
Contributor

please, add a layout test for this change so we can be sure it won't regress in the future :)

layout tests live here https://github.com/SerenityOS/serenity/tree/master/Tests/LibWeb/Layout

command that could be used to run tests ./Meta/serenity.sh test lagom LibWeb

@pixelspark
pixelspark force-pushed the fix/23154-repeat-auto-grid branch 2 times, most recently from d131eea to 46cd14f Compare February 11, 2024 14:05
@pixelspark

Copy link
Copy Markdown
Contributor Author

please, add a layout test for this change so we can be sure it won't regress in the future :)

layout tests live here https://github.com/SerenityOS/serenity/tree/master/Tests/LibWeb/Layout

command that could be used to run tests ./Meta/serenity.sh test lagom LibWeb

OK, something like this then?

@pixelspark
pixelspark force-pushed the fix/23154-repeat-auto-grid branch from 46cd14f to bd42841 Compare February 11, 2024 14:11
@kalenikaliaksandr

Copy link
Copy Markdown
Contributor

OK, something like this then?

I think we have an undeclared rule to use layout-tests when it is possible, as in this case. ref-tests are usually used for testing paint-related things that can't be caught in layout dumps.

@AtkinsSJ AtkinsSJ left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks good! Thank you. :^)

@AtkinsSJ AtkinsSJ added ✅ pr-maintainer-approved-but-awaiting-ci PR has been approved by a maintainer and can be merged after CI has passed and removed 👀 pr-needs-review PR needs review from a maintainer or community member ✅ pr-maintainer-approved-but-awaiting-ci PR has been approved by a maintainer and can be merged after CI has passed labels Feb 11, 2024
@AtkinsSJ

Copy link
Copy Markdown
Member

Oh whoops, Aliaksandr is right. A layout test would be better.

@pixelspark
pixelspark force-pushed the fix/23154-repeat-auto-grid branch from bd42841 to 4028dcd Compare February 11, 2024 15:24
@github-actions github-actions Bot added the 👀 pr-needs-review PR needs review from a maintainer or community member label Feb 11, 2024

@kalenikaliaksandr kalenikaliaksandr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

great, thank you!

@pixelspark

Copy link
Copy Markdown
Contributor Author

Related, this took way longer than expected. Turns out the headless-browser dumps the reference output too early, i.e. after loading "about:blank". The below patch fixed that for me (it would print "Loaded about:blank" and then the expected URL):

index 9a2a449f..1d4b3659 100644
--- a/Userland/Utilities/headless-browser.cpp
+++ b/Userland/Utilities/headless-browser.cpp
@@ -282,8 +282,8 @@ static ErrorOr<TestResult> run_dump_test(HeadlessWebContentView& view, StringVie
 
     if (mode == TestMode::Layout) {
         view.on_load_finish = [&](auto const& loaded_url) {
-            VERIFY(url.equals(loaded_url, URL::ExcludeFragment::Yes));
-
+            dbgln("Loaded: {}, need {}", loaded_url, url);
+            if(loaded_url == url) {
             // NOTE: We take a screenshot here to force the lazy layout of SVG-as-image documents to happen.
             //       It also causes a lot more code to run, which is good for finding bugs. :^)
             (void)view.take_screenshot();
@@ -293,7 +293,10 @@ static ErrorOr<TestResult> run_dump_test(HeadlessWebContentView& view, StringVie
             builder.append("\n"sv);
             builder.append(view.dump_paint_tree().release_value_but_fixme_should_propagate_errors());
             result = builder.to_string().release_value_but_fixme_should_propagate_errors();
+
+            
             loop.quit(0);
+            }
         };
         view.on_text_test_finish = {};
     } else if (mode == TestMode::Text) {

My invocation:

./Tests/LibWeb/rebaseline-libweb-test "/Users/tommy/Repos/serenity/Tests/LibWeb/Layout/input/grid/repeat-auto-fit-fill-cannot-have-auto.html"

@pixelspark

Copy link
Copy Markdown
Contributor Author

Just let me know if you want me to include that in the PR :-)

@nico

nico commented Feb 11, 2024

Copy link
Copy Markdown
Contributor

It's good to do one thing per PR. That seems unrelated to this one? (I don't know if it's something desirable for a different PR.)

@pixelspark

Copy link
Copy Markdown
Contributor Author

Sure thing, #23156

@pixelspark pixelspark changed the title fix: ignore repeat(auto-fit/auto-fill, auto) as it is not allowed LibWeb: ignore repeat(auto-fit/auto-fill, auto) as it is not allowed Feb 11, 2024
@kalenikaliaksandr
kalenikaliaksandr merged commit 1c7ec9c into SerenityOS:master Feb 11, 2024
@github-actions github-actions Bot removed the 👀 pr-needs-review PR needs review from a maintainer or community member label Feb 11, 2024
@pixelspark
pixelspark deleted the fix/23154-repeat-auto-grid branch February 11, 2024 16:57
@AtkinsSJ

Copy link
Copy Markdown
Member

@pixelspark You ended up being our 1000th contributor, congrats! 🥳
I don't think it actually means anything, but it's cool regardless.

@pixelspark

Copy link
Copy Markdown
Contributor Author

Cool, impressive milestone for the project 🎉

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.

LibWeb: specific auto grid layout causes crash (verification failed: grid_size.is_definite())

5 participants