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: Marker Position and sizing #23941 #24013

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

Conversation

hayneian000
Copy link

@hayneian000 hayneian000 commented Apr 18, 2024

Student Project.

Made changes to both BlockFormattingContext and MarkerPaintable to adjust the size and positioning of bullet points.

BlockFormattingContext:
Removed the marker_state.content_width() from the final marker width. This made the markers width adjust to the font size more naturally and have spacing similar to other browsers.

MarkerPaintable:
Made the marker height scaled down by a factor of 4 instead of 2. This made bullet size more similar to Chrome and Firefox.

@github-actions github-actions bot added the 👀 pr-needs-review PR needs review from a maintainer or community member label Apr 18, 2024
@AtkinsSJ
Copy link
Member

AtkinsSJ commented Apr 18, 2024

Hi there!

This isn't really a review of the code, I haven't tested it out myself. But here are some pointers.

From looking at the logs, these tests are failing:

  • Tests/LibWeb/Layout/input/ordered-list.html
  • Tests/LibWeb/Layout/input/block-and-inline/list-markers-intruded-by-float.html
  • Tests/LibWeb/Layout/input/details-closed.html
  • Tests/LibWeb/Layout/input/details-open.html

All of those are expected failures, because their results depend on the position and size of the list-item marker. You'll need to update the test output for those by running the Tests/LibWeb/rebaseline-libweb-test script and passing the test path as the input, one at a time. Then add those changes to the commit you already made, and force-push your branch.

As for the commit itself, the title should be an imperative, and say what you're changing. So I'd suggest something more like LibWeb: Make list-item markers more consistent with other browsers, and then put that nice explanation from your PR description above, into the commit message. There's no need to put the issue number in the title. However, writing "Fixes #..." either in the commit or the PR description will make GitHub automatically close the issue which is nice. :^)

If you need help with modifying an existing commit, this video is very helpful. You can also just ask on Discord and people will help you out.

Copy link

stale bot commented May 9, 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 May 9, 2024
@AtkinsSJ
Copy link
Member

@hayneian000 Do you need help?

@stale stale bot removed the stale label May 11, 2024
Copy link

stale bot commented Jun 2, 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 2, 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 student project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants