-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
LibWeb: dont inject natural sizes from prepare_for_replaced_layout #7028
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
base: master
Are you sure you want to change the base?
LibWeb: dont inject natural sizes from prepare_for_replaced_layout #7028
Conversation
|
Hello! One or more of the commit messages in this PR do not match the Ladybird code submission policy, please check the |
720f705 to
84abc88
Compare
c25e0ee to
f36a1fc
Compare
|
Your pull request has conflicts that need to be resolved before it can be reviewed and merged. Make sure to rebase your branch on top of the latest |
5520ff6 to
2e9c8cb
Compare
43f93ae to
0f8b840
Compare
|
I can't comment on the implementation itself, but this patch fixes the page I opened #6040 for |
|
Hi! Please check our contribution guidelines, and particularly:
Basically, whenever your commit message starts to describe more than one thing, it's very likely that we'd like to see separate commits. You should split them up to discern between non-functional changes involving code cleanup, refactors to prepare for changes, bugfixes and functional changes. |
a4be658 to
76cefaa
Compare
7b28372 to
572d31e
Compare
Ditch prepare_for_replaced_layout size value injection in favor of
virtual accessors that compute the same. This affects ReplacedBox
and all its subclasses.
Distinguish between:
- Natural size - always from media sources
- Intrinsic content box size (i.e. replaced-like) - could be natural
size or could be calculated from attributes like size, rows, cols
Don't overwrite them with aspect ratio calculations.
The is_textarea_box() flag in Node.h is unfortunate, but it's marginally better than an include & cast. textarea has specific baseline handling compared to other inline-block elements.
572d31e to
16f5830
Compare
|
I will say this about your commit message linter - I have had more enjoyable interactions with the rust borrow checker. |
Did you use pre-commit locally to lint your commits before pushing? If there's anything in particular that we should improve with the commit linter, feel free to raise an issue or submit a PR. |
I did not, and thanks for the heads up about it. I am just now remembering to run lint-clang-format.py
Oh, it was just frustration from trying to force push a bunch of pseudo-historical commits carefully constructed from manual edits of I'm miles away from an actionable suggestion. |
|
I wonder if it makes sense to require Category in the PR only. The commits themselves are ostensibly be related to this owning Category, and if they diverge then the true scope of each is readily available from the paths of their changed files. It could claw back 8-12 precious characters per commit for more context that may be less redundant. But maybe there is automated reporting going on where commit titles are readily available and files are not. I did notice the contributing.md mentions joining categories with + which sounds rather reporty. |
Instead, compute them in accessors.
Add TextAreaBox and TextInputBox to Layout so that textarea and text inputs can be stretched when auto.
Fix #634, fix #6040, fix #7070, and possibly related issues involving baseline, cross axis sizing, aspect ratio, canvas, size containment, and textarea