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

Add a formal mapping of Pack->CSS #1778

Merged
merged 16 commits into from
Feb 23, 2023
Merged

Conversation

freakboy3742
Copy link
Member

@freakboy3742 freakboy3742 commented Feb 14, 2023

The Pack layout was somewhat confused around how it was meant to behave with the "root" content of a layout. Using HTML as a model, the "root" content is the <body> element; and that element is always the size of the display window. Previously, Pack would only assume the full size of the window if the root content had no children; as soon as the root content had children, the flex axis (i.e., vertical in a Column box, horizontal in a Row box) would be shrunk to the minimum possible size. This led to interesting situations where a Column layout would be flexible in height, but wouldn't expand to the full height of the window. It would also lead to background colors not being applied over the entire background.

This PR makes an attempt at a policy statement at the intended Pack->HTML/CSS mapping as a source of reference, and then implements some changes to the handling of the root element that reflects those changes.

[Remainder moved to #1794. – @mhsmith]

PR Checklist:

  • All new features have been tested
  • All new features have been documented
  • I have read the CONTRIBUTING.md file
  • I will abide by the code of conduct

@mhsmith
Copy link
Member

mhsmith commented Feb 19, 2023

Using HTML as a model, the "root" content is the <body> element; and that element is always the size of the display window.

Based on my experiments, this is only the case: when quirks mode is active, which happens when the document contains no DOCTYPE, or a very old one. I think this is what was happening with your test document on Thursday.

With <!DOCTYPE html>, the <html> and <body> elements follow the normal rules for block elements, and shrink vertically to fit their content. However, it may not look that way, because there's a special case in the CSS spec which says the background-color and overflow properties on the <body> element are actually applied to the viewport and not the body at all. If you want to see what the <body> dimensions really are, you can add a border, or use the browser's layout inspector.

So it looks like we should add the following rule:

html, body {
    height: 100%;
}

I'll post the rest of my review tomorrow.

@freakboy3742
Copy link
Member Author

Agreed that quirks mode was the problem here; the 100% rule change is definitely required. I'll make a change to that effect.

@freakboy3742
Copy link
Member Author

freakboy3742 commented Feb 20, 2023

I've done some more fiddling with quirks mode, and updated the mapping definition to include a full reference HTML document.

I've also added white-space: nowrap to the body definition, on the basis that we don't do content wrapping anywhere in Pack.

Based on those changes, Tutorial 0 becomes:

<body style="flex-direction: row;">
    <div style="flex-direction: row; margin: 50px; flex: 1 0 0%">Press me</div>
</body>

and Tutorial 1 becomes:

<body style="flex-direction: column; margin: 20px">
    <div style="flex-direction: row; margin: 10px">
        <div style="flex-direction: row; margin-left: 210px; flex:1 0 0%;">Input</div>
        <div style="flex-direction: row; margin-left: 10px; width: 200px;">Celcius</div>
    </div>
    <div style="flex-direction: row; margin: 10px">
        <div style="flex-direction: row; width: 200px; margin-right: 10px">Is equal to</div>
        <div style="flex-direction: row; flex:1 0 0%;">Input</div>
        <div style="flex-direction: row; margin-left: 10px; width: 200px">Fahrenheit</div>
    </div>
    <div style="flex-direction: row; margin: 15px">Convert</div>
</body>

(This tutorial 1 example has slightly different margin values, but the effect is the same).

Interestingly, I think I've found a bug in Chrome (109.0.5414.119) around the handling of margin collapse on the root element. Opening the document in a new tab always gives the correct rendering; however, flex-direction:column containers collapse the root element's top margin if the document is already loaded on the page.

core/tests/style/test_pack.py Outdated Show resolved Hide resolved
docs/reference/style/pack.rst Outdated Show resolved Hide resolved
Comment on lines 581 to 582
if self.flex:
css.append(f"flex: {self.flex} 0 0%;")
Copy link
Member

Choose a reason for hiding this comment

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

This contradicts the definition of flex in pack.rst, which says that an explicit size in the main axis should cause flex to be ignored. Is that the current Pack behavior?

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe I was relying on the fact that a flex definition will be ignored if width/height are defined. However, to avoid confusion, it's probably better to merge this into the width/height constructions above.

Copy link
Member

@mhsmith mhsmith Feb 22, 2023

Choose a reason for hiding this comment

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

Based on my tests, both flex: 1 0 0 and flex: 0 0 0 do have an effect in CSS even if there's an explicit width/height, so we'd better only generate either flex or width/height.

docs/reference/style/pack.rst Show resolved Hide resolved
docs/reference/style/pack.rst Outdated Show resolved Hide resolved
docs/reference/style/pack.rst Outdated Show resolved Hide resolved
docs/reference/style/pack.rst Outdated Show resolved Hide resolved
docs/reference/style/pack.rst Outdated Show resolved Hide resolved
docs/reference/style/pack.rst Outdated Show resolved Hide resolved
docs/reference/style/pack.rst Outdated Show resolved Hide resolved
core/src/toga/style/pack.py Outdated Show resolved Hide resolved
docs/reference/style/pack.rst Outdated Show resolved Hide resolved
freakboy3742 and others added 2 commits February 23, 2023 08:11
@mhsmith
Copy link
Member

mhsmith commented Feb 23, 2023

I think there will be some overlap between the GTK changes and what I'm working on for Android. So I've reduced this PR to the CSS mapping changes, which I'll merge now, and I'll make a separate PR for the GTK changes (#1794).

@mhsmith mhsmith merged commit b851bc0 into beeware:main Feb 23, 2023
mhsmith added a commit to mhsmith/toga that referenced this pull request Feb 23, 2023
@mhsmith mhsmith mentioned this pull request Feb 23, 2023
4 tasks
@mhsmith mhsmith changed the title Modify container handling for GTK Add a formal mapping of Pack->CSS Feb 23, 2023
@freakboy3742 freakboy3742 deleted the containers branch February 23, 2023 22:21
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