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: Implement :modal pseudo class #345

Merged

Conversation

lukewarlow
Copy link
Contributor

Adds the :modal pseudo class which matches dialogs opened with showModal().

Passes all but 1 assertions within the first subtest of https://wpt.live/css/selectors/modal-pseudo-class.html

image

That final assertion seems to be failing due to a spec bug: whatwg/html#10452

I'll do a follow up PR to fix this in Ladybird once the spec is updated.

@lukewarlow lukewarlow requested a review from AtkinsSJ as a code owner July 1, 2024 23:39
Copy link
Member

@AtkinsSJ AtkinsSJ left a comment

Choose a reason for hiding this comment

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

Looks good, but can we get a test for this?

@lukewarlow
Copy link
Contributor Author

So there's a WPT test for it, but ladybirds WPT infrastructure is a bit confusing and doesnt' seem complete. Where would an internal test for this go and what would it look like?

Also any ideas on the leaks in the CI? @AtkinsSJ

@AtkinsSJ
Copy link
Member

AtkinsSJ commented Jul 2, 2024

I wouldn't worry too much about the leak, that seems very unrelated.

We rely a lot on internal tests for now. They go in Tests/LibWeb. This would probably make sense as a layout test, so you'd create an html file in there with two dialogs, one modal and one not, and apply some CSS that affects only one, in a way that shows up in the layout. There's a rebaseline-libweb-test script in there which will generate the output file, which is a dump of the layout tree.

@lukewarlow
Copy link
Contributor Author

@AtkinsSJ is that test the sort of thing you were looking for?

@tcl3
Copy link
Member

tcl3 commented Jul 2, 2024

This change is causing the following test to fail: /home/tim/repos/ladybird/Tests/LibWeb/Layout/input/top-layer.html

on master:

image

This PR:

image

You can use ./Meta/ladybird.sh test LibWeb to run all our tests locally.

Adds the :modal pseudo class which matches dialogs opened with
showModal().
@lukewarlow
Copy link
Contributor Author

This change is causing the following test to fail: /home/tim/repos/ladybird/Tests/LibWeb/Layout/input/top-layer.html

Apologies, I didn't spot that. After some debugging I've found that this change in rendering while a regression visually is actually correct. So I've updated the baseline for it.

The below CSS is used in Default.css and I believe there's a bug in the implementation in some of these CSS properties. Which leads to the "incorrect" rendering of modal dialogs after this change.

dialog:modal {
    position: fixed;
    overflow: auto;
    inset-block: 0;
    max-width: calc(100% - 6px - 2em);
    max-height: calc(100% - 6px - 2em);
}

@awesomekling awesomekling merged commit 63a5ff7 into LadybirdBrowser:master Jul 8, 2024
6 checks passed
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.

4 participants