Textualize / textual

The lean application framework for Python. Build sophisticated user interfaces with a simple Python API. Run your apps in the terminal and a web browser.
https://textual.textualize.io/
MIT License
25.07k stars 767 forks source link

`Input` wider than widgets sized the same #3721

Closed davep closed 7 months ago

davep commented 10 months ago

I've not dived into why this is, but I just noticed it while working on something else and wanted to make a note for further checking. If you have a collection of widgets, all with the same width, Input stands out as not being the same width as the others:

from textual.app import App, ComposeResult
from textual.widgets import Input, TextArea, Static, Button

class InputVsTextArea(App[None]):

    CSS = """
    Screen > *, Screen > *:focus {
        width: 50%;
        height: 1fr;
        border: solid red;
    }
    """

    def compose(self) -> ComposeResult:
        yield Input()
        yield TextArea()
        yield Static()
        yield Button()

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

Screenshot 2023-11-21 at 21 19 28

Possible down to something in its DEFAULT_CSS? Whatever the cause, it should probably really line up with the other widgets when at the same width.

rodrigogiraoserrao commented 9 months ago

I wondered whether it was something about the box sizing, but it's not.

Also, this happens with h and w units but not with absolute widths, vh, and vw. fr untested.

TomJGooding commented 9 months ago

You've probably figured this out already, but this seems to be due to the padding: 0 2 in the Input default CSS.

If you add the same padding to the other widgets in your example, they display the same width. This possibly suggests a bug with how the percentage width is calculated, where it isn't properly taking into account padding? I don't quite understand why the width is 2 columns wider than than 4 though...

rodrigogiraoserrao commented 9 months ago

I meant to say that the issue could be a rule box-sizing that had the wrong value and it turns out it's not that (or, at least, not just that).

It may very well just be that the style width doesn't take into account padding, which may be on purpose.

I'll stop speculating and leave this for whomever picks up the issue to solve it.

TomJGooding commented 9 months ago

I think I might have narrowed it down to these lines:

https://github.com/Textualize/textual/blob/a64a0d21d787016df9dde84287cb36ecda50b787/src/textual/widget.py#L991

https://github.com/Textualize/textual/blob/a64a0d21d787016df9dde84287cb36ecda50b787/src/textual/widget.py#L1018-L1019

I confess I'm struggling a bit to follow the code, but here's what seems to be happening if we take my example app below.

  1. content_container = 96 (100 - 4)
  2. content_width = 48 (96 * 0.5)
  3. widget width = 52 (48 + 4)

The content_width doesn't account for the padding as styles_width.excludes_border returns False.

[EDIT: Sorry I've realised this also wouldn't calculate the correct size if gutter.width is 4!]

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

class PercentWidthTestApp(App):
    CSS = """
    Container {
        width: 100;
    }

    Static {
        padding: 0 2;
        width: 50%;
    }
    """

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

if __name__ == "__main__":
    app = PercentWidthTestApp()
    app.run()
TomJGooding commented 9 months ago

Based on my findings above, I'm wondering if the sizing calculation instead should be something like this:

@@ -1007,6 +1007,9 @@ class Widget(DOMNode):
         is_auto_width = styles.width and styles.width.is_auto
         is_auto_height = styles.height and styles.height.is_auto

+        is_percent_width = styles.width and styles.width.unit == Unit.WIDTH
+        is_percent_height = styles.height and styles.height.unit == Unit.HEIGHT
+
         # Container minus padding and border
         content_container = container - gutter.totals
         # The container including the content
@@ -1029,6 +1032,13 @@ class Widget(DOMNode):
                 and self._has_relative_children_width
             ):
                 content_width = Fraction(content_container.width)
+        elif is_percent_width:
+            styles_width = styles.width
+            content_width = styles_width.resolve(
+                container - styles.margin.totals, viewport, width_fraction
+                )
+            if is_border_box:
+                content_width -= gutter.width
         else:
             # An explicit width
             styles_width = styles.width
@@ -1073,6 +1083,13 @@ class Widget(DOMNode):
                 and self._has_relative_children_height
             ):
                 content_height = Fraction(content_container.height)
+        elif is_percent_height:
+            styles_height = styles.height
+            content_height = styles_height.resolve(
+                container - styles.margin.totals, viewport, height_fraction
+                )
+            if is_border_box:
+                content_height -= gutter.height
         else:
             styles_height = styles.height
             # Explicit height set

My regression test passes, but there are 9 mismatched snapshots. But these mostly look like examples of widgets with a percent size and padding.

async def test_widget_percent_size_with_padding():
    """Regression test for https://github.com/Textualize/textual/issues/3721"""

    class PercentSizeTestApp(App):
        CSS = """
        Container {
            height: 100;
            width: 100;
        }

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

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

    app = PercentSizeTestApp()
    async with app.run_test() as pilot:
        widget = pilot.app.query_one(Static)
        assert widget.content_size == Size(46, 46)
        assert widget.outer_size == Size(50, 50)
willmcgugan commented 8 months ago

It does look like percentage widths (and probably height) don't account for padding. It should, 50% with border-box should always be literally half the width.

rodrigogiraoserrao commented 8 months ago

The other widths are off by one. The screenshot below shows a “ruler” that measures 100 cells and the textarea, button, and static, all measure 51 cells. The Input measures 53.

Screenshot 2024-01-10 at 14 52 54

App code ```py from textual.app import App, ComposeResult from textual.containers import Container from textual.widgets import Input, TextArea, Static, Button class InputVsTextArea(App[None]): CSS = """ Screen > Container > *, Screen > Container > *:focus { width: 50%; height: 1fr; border: solid red; box-sizing: border-box; } """ def compose(self) -> ComposeResult: yield Static("1234567890" * 10) with Container(): yield TextArea() yield Input() yield Static() yield Button() if __name__ == "__main__": InputVsTextArea().run() ```
TomJGooding commented 8 months ago

I missed this before, but presumably gutter.width with the borders is 2, and then with the addtional padding the width would be 6. So doing the same calculations from my earlier post shows where this might be going awry:

With border only:

  1. content_container = 98 (100 - 2)
  2. content_width = 49 (98 * 0.5)
  3. widget width = 51 (48 + 2)

With extra padding:

  1. content_container = 94 (100 - 6)
  2. content_width = 47 (94 * 0.5)
  3. widget width = 53 (47 + 6)
rodrigogiraoserrao commented 8 months ago

Yup. Your diff above seems to do pretty much the trick. I'm just checking everything but I'll add you as a co-author.

TomJGooding commented 8 months ago

Thanks Rodrigo but honestly no need! My fix above is a bit rough and ready so no doubt you'll find a better solution!

rodrigogiraoserrao commented 8 months ago

😭 the issue also affects max/min dimensions.

github-actions[bot] commented 7 months ago

Don't forget to star the repository!

Follow @textualizeio for Textual updates.