LibWeb: Add word spacing to tab size correctly #1999
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Oops! Can we add a test? |
|
Yeah I was surprised that this didn't change the existing layout tests but the extra word spacing didn't actually affect where the next glyph went as the next tab stop didn't change with the extra width 😄 I'll add a test where the extra spacing does push the glyph to the subsequent tab stop! |
|
Shouldn't there have been an unused variable error for this? |
|
erm good point, it did not seem to trigger it that in linting, it is indeed unused before this patch. |
kostyafarber
added a commit
to kostyafarber/ladybird
that referenced
this pull request
Oct 28, 2024
This is the default font size used in Chrome and Firefox and my theory is some WPT tests make the same assumption. As an example, https://wpt.live/css/css-text/tab-size/tab-size-spacing-001.html will pass once PR LadybirdBrowser#1999 is merged if we explicitly set the font-size to be 13px. I figured this out when comparing the size of the space character across Chrome and Ladybird, which we were sizing to large and hence overshooting the red square in the aforementioned WPT test. I'd like to think that this is true across all WPT tests that measure pixels and use monospace as their font-family :^) but this is unsubstantiated.
kostyafarber
force-pushed
the
fix-tab-size-word-spacing-bug
branch
from
83c67ed
to
1b48008
Compare
We should be adding the computed value for word spacing not letter spacing twice.
kostyafarber
force-pushed
the
fix-tab-size-word-spacing-bug
branch
from
1b48008
to
755d938
Compare
|
@awesomekling test added, I yoinked it from wpt, although it's not passing there (see why I think in the discussion on #2020) but it's a good test for this bug. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We should be adding the computed value for word spacing not letter spacing twice.