render: resolve functional flex-basis against the containing block - #749
Open
alexskinner wants to merge 1 commit into
Open
render: resolve functional flex-basis against the containing block#749alexskinner wants to merge 1 commit into
alexskinner wants to merge 1 commit into
Conversation
`flex-basis: calc(100% - 314px)` was evaluated context-free at parse time through `dimension_value`, so the `100%` became 0 and the basis a negative length that taffy clamped to 0. With a definite zero basis and `flex-grow: 0` the item sat at its min-content width, and any specified `width` was (correctly, per spec) irrelevant — which made the failure look like a dropped width declaration. readymembership.com's header nav stacked vertically because of exactly this rule. `width`/`height`/`min-*`/`max-*` already defer functional expressions in `size_expressions` and resolve them in the layout pass; give `flex-basis` the same treatment via a `flex_basis_expression` slot resolved against the containing-block width. The `flex` shorthand now tokenises at paren depth 0 so `flex: 0 0 calc(...)` yields one basis token instead of three.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed
flex-basis: calc(100% - 314px)(longhand, or as the third token offlex:) was evaluated context-free at parse time throughdimension_value, so the100%became 0 and the basis a negative length that taffy clamped to 0. With a definite zero basis andflex-grow: 0the item sat at its min-content width, and any specifiedwidthwas — correctly, per spec — irrelevant. That made the failure look like a droppedwidthdeclaration (only an inlinestyle.flexBasiscould move the element), which is how it was first reported in #737 alongside a Wikipedia case that turns out to be a separate table-sizing issue.width/height/min-*/max-*already defer functional expressions insize_expressionsand resolve them inlayout_dom's top-down pass; this givesflex-basisthe same treatment:LayoutStyle::flex_basis_expression: Option<String>(lib.rs)flex-basislonghand andflexshorthand store the expression; keyword forms clear it (style.rs)flexshorthand tokenises withsplit_top_level(value, ' ')soflex: 0 0 calc(100% - 314px)is one basis token instead of threeresolve_contextual_lengthagainstinh.cb_widthnext to the existing width/height resolution (dom.rs)Related to #737 (fixes the readymembership.com half of it; the
en.wikipedia.orginfobox there is a table min-content-width problem, not this).Validation
functional_flex_basis_resolves_against_the_containing_blockincrates/obscura-render/tests/layout_test.rs: longhand and shorthandcalc()bases in a 1200px flex row both lay out at 886px (were 26px, the item's min-content width).cargo test -p obscura-render --test layout_test: 97 passed / 11 failed, the same 11 failures asmainbefore this change (font/environment related, unrelated files).https://readymembership.com/at 1400×900,.main-nav-holderwent from 145px (nav stacked vertically over the hero) to 1046px with the nav laid out horizontally.width: calc(100% - 314px)on the same item already produced 886px in the fixture before the change, confirming the deferred path is the right one.Rendering
Layout only. Fixture, viewport 1400×900, DSF 1:
Second item before: 26px wide. After: 886px (Chrome: 886px).
Performance
No expected impact: one extra
Option<String>per style, populated only when aflex-basis/flexvalue contains(, and one extra expression evaluation per such node in the existing per-node resolution pass.Checklist
main.)LayoutStyle, doc-commented.)