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

fix for percentage dimensions #4037

Merged
merged 6 commits into from
Jan 31, 2024
Merged

fix for percentage dimensions #4037

merged 6 commits into from
Jan 31, 2024

Conversation

willmcgugan
Copy link
Collaborator

@willmcgugan willmcgugan commented Jan 17, 2024

Calculating percentage dimensions didn't take in to account padding / border.

Fixes #3721

Supersedes #4023

@rodrigogiraoserrao
Copy link
Contributor

There's something wrong with this PR.

If you look at dimensions_width.py test in the superseded PR, you'll see that your PR doesn't pass that test.
Barring any misunderstanding on my part of how max-/min- units are supposed to work, the apps from dimensions_width.py, dimensions_max_width.py, and dimensions_min_width.py, all should look the same but they don't (I suggest looking at them on a terminal of width 80 because that's the length of the ruler).

@willmcgugan willmcgugan marked this pull request as draft January 17, 2024 15:24
@willmcgugan
Copy link
Collaborator Author

@rodrigogiraoserrao Could you please isolate a single case that isn't working. i.e. an MRE with as few widgets as possible which displays incorrect behaviour.

@rodrigogiraoserrao
Copy link
Contributor

Sure, here is one: % isn't respected when margin is set.
In this case, the red frame should take up all of the horizontal width but it's off by 4

from textual.app import App, ComposeResult
from textual.widgets import Label


class NestedCSSTokenErrorApp(App[None]):
    CSS = """
    Label.target {
        width: 100%;
        margin: 2;
        border: solid red;
    }
    """

    def compose(self) -> ComposeResult:
        # yield Label("1234567890" * 80)
        yield Label("", classes="target")


if __name__ == "__main__":
    NestedCSSTokenErrorApp().run()

Another manifestation of the same problem can be seen in the app below. The bottom label is correctly set to 50% width (via max-width) but the top one isn't:

from textual.app import App, ComposeResult
from textual.widgets import Label, Static


class NestedCSSTokenErrorApp(App[None]):
    CSS = """
    Label {
        border: solid red;
        margin: 0 4;
    }
    #first { width: 50%; }
    #second {
        width: 100%;
        max-width: 50%;
    }
    """

    def compose(self) -> ComposeResult:
        yield Static("1234567890" * 8)
        yield Label("", id="first")
        yield Label("", id="second")


if __name__ == "__main__":
    NestedCSSTokenErrorApp().run()

@rodrigogiraoserrao
Copy link
Contributor

This may be why you feel my PR changed the semantics of %, but we talked and we agreed that the margin is not a part of the size of the widget.

When we talked, you mentioned there was some special case in the code to do something cheeky with 100%, widgets with a single children, and margin, but we also realised that you didn't need that special case (which impacted other situations where there are % units and margin, like the one with the 50% above).

@willmcgugan
Copy link
Collaborator Author

It's behaving as I'd expect.

The percentage applies to the available width, which doesn't include margin.

If it didn't behave like this, and you set width 100% with a margin, it would be pushed off screen. It would be simpler (for us) if the percentage was an exact percentage of the container, but this would make it very hard for the dev to achieve sensible layouts.

The behaviour should be identical if you have a width of 1fr with a single item.

@rodrigogiraoserrao
Copy link
Contributor

rodrigogiraoserrao commented Jan 17, 2024

So, you're telling me that if I set width: 100% and then margin: 1000, my widget collapses to have zero width?
I was under the impression that the percent unit % was relative to the total size of the container and not “available” size of the container, and that using margins that are too big with sizes that are too big does result in things getting pushed off.

I know we don't blindly copy browser CSS but it appears that's what it does as well:

Screenshot 2024-01-17 at 17 40 05

When we talked, we understood that the effect you're describing could be achieved by setting padding in the container and child dimensions to fr units, for example, precisely because 100% + margin pushes things off screen whereas padding + 1fr won't, given that 1fr will fill the available container space (which will be everything minus the padding you set there).

@TomJGooding
Copy link
Contributor

TomJGooding commented Jan 17, 2024

Hope you don't mind me sticking my nose in as usual, but this behaviour with margin seems counter-intuitive to me?

Percent size with padding ✅

Here's a simple example with a 10x10 container with a widget of 50% height/width. The widget takes up a quarter of the container size as expected, so this PR fixes the issue with how size was calculated with padding/border.

Code
from textual.app import App, ComposeResult
from textual.containers import Container
from textual.widgets import Static


class ExampleApp(App):
    CSS = """
    Screen {
        align: center middle;
    }

    Container {
        height: 10;
        width: 10;
        background: blue;
    }

    Static {
        height: 50%;
        width: 50%;
        background: red;
        padding: 2 1;
    }
    """

    def compose(self) -> ComposeResult:
        with Container():
            yield Static("5x5")


if __name__ == "__main__":
    app = ExampleApp()
    app.run()

image

Percent size with padding and margin ❌

But now let's add a margin of 5 (i.e. 50% of the container) to the top and left of the widget. You would expect the widget to now take up the bottom-right quarter of the container, but instead you get this:

Code
from textual.app import App, ComposeResult
from textual.containers import Container
from textual.widgets import Static


class ExampleApp(App):
    CSS = """
    Screen {
        align: center middle;
    }

    Container {
        height: 10;
        width: 10;
        background: blue;
    }

    Static {
        height: 50%;
        width: 50%;
        background: red;
        padding: 2 1;
        margin-top: 5;
        margin-left: 5;
    }
    """

    def compose(self) -> ComposeResult:
        with Container():
            yield Static("5x5")


if __name__ == "__main__":
    app = ExampleApp()
    app.run()

image

@willmcgugan
Copy link
Collaborator Author

It's a pragmatic design choice for Textual. If I have a widget with width and height 100% and apply a margin, it currently produces something useful. i.e. the widget is centered in the screen. If the margin was additive, then the widget would overflow the screen boundary. That is very unlikely to be useful for the developer.

If you do want a widget to be an exact percentage, but offset from the top left then you can use offset and achieve the same results.

The interaction with margin is orthogonal to the original issue. The original issue was that percentage dimensions excluded the gutter, which this PR should fix. Nothing re margins has changed. We can always change that if needed, but AFAIK nobody as found this to be a problem, and I think we should err on the side of not breaking things.

@willmcgugan willmcgugan marked this pull request as ready for review January 18, 2024 10:07
Copy link
Contributor

@rodrigogiraoserrao rodrigogiraoserrao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If margin + % is to work like so, shouldn't margin be taken into account for max/min units, then?

E.g., regardless of the way we decide to resolve % units with margin, I'd expect the two placeholders below to have the same width because of their TCSS:

Screenshot 2024-01-18 at 14 22 09

from textual.app import App, ComposeResult
from textual.widgets import Label, Placeholder


class NestedCSSTokenErrorApp(App[None]):
    CSS = """
    Placeholder {
        margin: 0 4;
        height: 3;
    }
    #first { 
        width: 50%;  # <--
    }
    #second {
        width: 999;    # <-- this is huge...
        max-width: 50%;  # <-- ... so, this one kicks in.
    }
    """

    def compose(self) -> ComposeResult:
        yield Label("1234567890" * 8)
        yield Placeholder("", id="first")
        yield Placeholder("", id="second")


if __name__ == "__main__":
    NestedCSSTokenErrorApp().run()

Comment on lines +29 to +31
box_margins: list[Spacing] = [
styles.margin for styles in child_styles if styles.overlay != "screen"
]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see you changed this because you probably thought of an edge case that wasn't being covered; wouldn't this warrant a test?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It probably should. Going to leave it for now, because overlay isn't yet documented.

Comment on lines -39 to +41
max(
min(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment re: test

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is covered by a snapshot, but it probably does deserve a more specific test. Going to leave it for now though.

max(
min(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment re: test

src/textual/widget.py Show resolved Hide resolved
@willmcgugan willmcgugan merged commit 343893d into main Jan 31, 2024
20 checks passed
@willmcgugan willmcgugan deleted the fix-percentage-dimensions branch January 31, 2024 13:53
@TomJGooding TomJGooding mentioned this pull request Feb 1, 2024
@rodrigogiraoserrao
Copy link
Contributor

@willmcgugan you didn't say anything regarding this comment so I'm not sure if this slipped past you or if it's the intended behaviour.

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.

Input wider than widgets sized the same
3 participants