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

Input wider than widgets sized the same #3721

Closed
davep opened this issue Nov 21, 2023 · 12 comments · Fixed by #4037
Closed

Input wider than widgets sized the same #3721

davep opened this issue Nov 21, 2023 · 12 comments · Fixed by #4037
Assignees
Labels
bug Something isn't working Task

Comments

@davep
Copy link
Contributor

davep commented Nov 21, 2023

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.

@davep davep added bug Something isn't working Task labels Nov 21, 2023
davep added a commit to davep/tinboard that referenced this issue Nov 21, 2023
Work-in-progress of adding an edit screen for bookmarks. This will be used
for adding new bookmarks and editing existing ones. Something that needs to
be added is a tag suggestion facility -- perhaps using the autocomplete or
something; need to look into that.

Textualize/textual#3721 is currently causing the
layout to not be as good as it could be, and there's also the issue of how
TextArea works meaning that:

- There's no simple coloring.
- There's no easy way to handle going into light mode.
- There's no way to have the cursor not be visible when not focused.

Some of these things will just have to be so until changes are made in
Textual itself.
@rodrigogiraoserrao
Copy link
Contributor

rodrigogiraoserrao commented Nov 22, 2023

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
Copy link
Contributor

TomJGooding commented Nov 22, 2023

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
Copy link
Contributor

rodrigogiraoserrao commented Nov 22, 2023

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
Copy link
Contributor

TomJGooding commented Nov 22, 2023

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

content_container = container - gutter.totals

textual/src/textual/widget.py

Lines 1018 to 1019 in a64a0d2

if is_border_box and styles_width.excludes_border:
content_width -= gutter.width

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
Copy link
Contributor

TomJGooding commented Nov 30, 2023

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
Copy link
Collaborator

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 rodrigogiraoserrao self-assigned this Jan 10, 2024
@rodrigogiraoserrao
Copy link
Contributor

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
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
Copy link
Contributor

TomJGooding commented Jan 10, 2024

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

😭 the issue also affects max/min dimensions.

rodrigogiraoserrao added a commit that referenced this issue Jan 15, 2024
ASSUMPTION: The margin of a widget is NOT taken into account when computing its size.
E.g., if the terminal has width 100 and a widget has margin of 10 and a width of 100% or 100 or 100w or 100vw, its height _will_ be 100 and not 80.

The original fix to #3721 introduced a new branch in the 'if elif else' branch that determines how to compute the width/height of widgets but as I kept digging, I realised calculations were 'off by one' in a couple of other places.
Further debugging eventually revealed that the 'fix' actually results in a simplification of the code.
rodrigogiraoserrao added a commit that referenced this issue Jan 15, 2024
ASSUMPTION: The margin of a widget is NOT taken into account when computing its size.
E.g., if the terminal has width 100 and a widget has margin of 10 and a width of 100% or 100 or 100w or 100vw, its height _will_ be 100 and not 80.

The original fix to #3721 introduced a new branch in the 'if elif else' branch that determines how to compute the width/height of widgets but as I kept digging, I realised calculations were 'off by one' in a couple of other places.
Further debugging eventually revealed that the 'fix' actually results in a simplification of the code.

Co-authored-by: TomJGooding <[email protected]>
@rodrigogiraoserrao rodrigogiraoserrao linked a pull request Jan 15, 2024 that will close this issue
Copy link

Don't forget to star the repository!

Follow @textualizeio for Textual updates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Task
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants