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: Specify input box width when parent does not #24490

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

Conversation

Arben-Sear
Copy link

@Arben-Sear Arben-Sear commented May 30, 2024

Text input boxes inside a parent with no defined width, such as the following, appear too short in Ladybird.

<div style = "float:left">
    <input style = "width:auto">
</div>

See #23915, which this fixes.

The fix was two parts: remove some hacky code from HTMLInputElement::adjust_computed_style() that set the width of any input box container with <... width:auto> and instead use set_natural_width() and set_natural_height() in the Layout::BlockContainer constructor.

NOTES/Pending issues

  • Despite setting natural height to 1lh, some text boxes (such as the one on the ladybird home screen) are now too short. I believe natural width and height might ignore padding/margins/borders, but I'm not sure.
  • Apologies, but this probably won't pass the code submission policies -- this is my first ever PR.
    • EDIT: all pending changes done -- we could use feedback on the failing checks, see previous note

@Arben-Sear Arben-Sear changed the title LibWeb: Specify input box width when parent does not #23915 LibWeb: Specify input box width when parent does not May 30, 2024
@github-actions github-actions bot added the 👀 pr-needs-review PR needs review from a maintainer or community member label May 30, 2024
@BuggieBot
Copy link
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.

@Arben-Sear Arben-Sear force-pushed the input-box-width branch 2 times, most recently from 6cd2552 to 1771116 Compare May 31, 2024 17:25
Copy link
Member

@awesomekling awesomekling left a comment

Choose a reason for hiding this comment

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

These changes are spread across three separate commits even though they're just iterating on the same places again and again. I think it would be better to just squash them all into one commit that gets to the end state immediately. :)

Userland/Libraries/LibWeb/HTML/HTMLInputElement.cpp Outdated Show resolved Hide resolved
Userland/Libraries/LibWeb/Layout/BlockContainer.cpp Outdated Show resolved Hide resolved
Comment on lines 16 to 18
StringBuilder builder;
builder.append(dom_node()->class_name());
if (MUST(builder.to_string()) == "HTMLInputElement") {
Copy link
Member

Choose a reason for hiding this comment

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

This can be:

if (dom_node()->is_html_input_element()) {

Copy link
Author

Choose a reason for hiding this comment

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

I did something similar elsewhere, but it took the form if (foo.dom_node() && foo.dom_node()->is_html_input_element()). Is this okay?

Userland/Libraries/LibWeb/Layout/BlockContainer.cpp Outdated Show resolved Hide resolved
Userland/Libraries/LibWeb/Layout/BlockContainer.cpp Outdated Show resolved Hide resolved
Userland/Libraries/LibWeb/Layout/FormattingContext.cpp Outdated Show resolved Hide resolved
Userland/Libraries/LibWeb/Layout/BlockContainer.cpp Outdated Show resolved Hide resolved
Userland/Libraries/LibWeb/Layout/BlockContainer.cpp Outdated Show resolved Hide resolved
@Arben-Sear
Copy link
Author

@awesomekling I performed the changes as you requested. There's still the issue with input box height:
image

It appears as though using natural_height doesn't account for borders/padding. Any recommendations?

Copy link

stale bot commented Jun 26, 2024

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions!

@stale stale bot added the stale label Jun 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
👀 pr-needs-review PR needs review from a maintainer or community member stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants