serenity icon indicating copy to clipboard operation
serenity copied to clipboard

LibWeb: Marker Position and sizing #23941

Open hayneian000 opened this issue 1 year ago • 3 comments

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.

hayneian000 avatar Apr 18 '24 17:04 hayneian000

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.

AtkinsSJ avatar Apr 18 '24 18:04 AtkinsSJ

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[bot] avatar May 09 '24 19:05 stale[bot]

@hayneian000 Do you need help?

AtkinsSJ avatar May 11 '24 08:05 AtkinsSJ

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[bot] avatar Jun 02 '24 07:06 stale[bot]

This pull request has been closed because it has not had recent activity. Feel free to re-open if you wish to still contribute these changes. Thank you for your contributions!

stale[bot] avatar Jun 19 '24 02:06 stale[bot]