Skip to content

Conversation

@campaul
Copy link
Contributor

@campaul campaul commented May 29, 2025

Fix for #67

This still needs more tests and documentation before it's ready to go but I wanted to post the PR to start getting feedback as early as possible. This is a big change so I expect it will end up needing many revisions.

Overview

Some changes that were made as prerequisites:

  • style.margin is now a length instead of just a value
  • the style class has marginLeft, marginRight, marginTop, and marginBottom functions for converting back to a value. Percent and auto are treated as 0 by this function.
  • boxOffsetX, boxRestWidth, and boxDiffWidth are moved from style to widget under the names marginBoxOffsetX, marginBoxRestWidth, and marginBoxDiffWidth.
  • a margin field is added to the Widget class.

The main logic is as follows:

  • calcWidth computes margin widths and returns a BoxWidth struct that contains the computed margin sizes instead of just the total size.
  • calcFinalWidth sets widget.margin.left and widget.margin.right based on what it picked as the final size.
  • the marginBoxOffsetX, marginBoxRestWidth, and marginBoxDiffWidth functions use the saved widget.margin values instead of style.margin values.

This works because getAvailableWidth is always called before any of boxOffsetX, boxRestWidth or boxDiffWidth. This means calcFinalWidth is guaranteed to have already run.

Screenshots

The margin auto test passing
Screenshot 2025-05-29 at 2 22 59 PM

The dillo website correctly centered now
Screenshot 2025-05-29 at 2 23 33 PM

@rodarima
Copy link
Member

rodarima commented Jun 2, 2025

Thanks a lot for the effort! I will check the implementation when I have a moment, what you describe sounds reasonable.

So far I see that with fca573f the page https://dillo-browser.github.io/25-years/index.html extends the width ignoring the max-width constraint. Can we add another test case so we prevent future regressions?

@campaul campaul mentioned this pull request Jun 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants