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/HTML: Skip layout for a an empty label that controls nothing #3079

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

manuel-za
Copy link
Contributor

Fixes #2864

@Psychpsyo
Copy link
Contributor

Psychpsyo commented Dec 29, 2024

Removing the label from layouting isn't the solution to this and I doubt it's what the spec requires.
The label still needs to participate in layout and have a box, otherwise things like this break:

<label style="padding:100px"></label>Hello!

Those 100px of padding still need to exist and need to shove the "Hello!" over to the right.

Screenshots for the example above:
In Firefox:
image
In Chrome:
image
In Ladybird, with your patch:
image

It seems to me that the actual solution here is to just remove the display: inline-block; from the label style in Default.css.
I can't find a spec for that and it seems to not exist in other browsers either.

@manuel-za
Copy link
Contributor Author

manuel-za commented Dec 29, 2024

The label still needs to participate in layout and have a box

Makes sense. I'll try to adjust. Maybe just zero the height? Spec for label seems to cover mostly the happy path.

just remove the display: inline-block; from the label style in Default.css

Brings up a whole other can of worms. Lots of stuff seems to be relying on that.

FIXME: InlineFormattingContext::dimension_box_on_line got unexpected box in inline context:

Thanks @Psychpsyo!

@Psychpsyo
Copy link
Contributor

The label still needs to participate in layout and have a box

Makes sense. I'll try to adjust. Maybe just zero the height? Spec for label seems to cover mostly the happy path.

just remove the display: inline-block; from the label style in Default.css

Brings up a whole other can of worms. Lots of stuff seems to be relying on that.

I think the whole other can of worms might be what needs dealing with here.
Introducing non-spec workarounds feels like it'd just lead to more broken behavior in other situations without fixing the underlying problem.

Just zeroing the height for example doesn't work since any label can be given display: block and height: 100px, in which case it needs to be 100 pixels tall.
And I'd assume there's probably a myriad of other CSS properties one can use to make the label have any width, height or layout, while not having any actual content.
Your current solution also only checks for empty text content, so you'd be treating a label with non-text content, like an image, as empty as well and I'm not sure what the implications of that even are.

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.

Empty label element's height is not 0
2 participants