improve(HTML): add full flex support#1672
Open
kokekanon wants to merge 16 commits intoopentibiabr:mainfrom
Open
improve(HTML): add full flex support#1672kokekanon wants to merge 16 commits intoopentibiabr:mainfrom
kokekanon wants to merge 16 commits intoopentibiabr:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds 21 standalone inline-styled HTML Flexbox examples and implements a large flex-layout engine overhaul with related widget API changes, HTML batching, style parsing, text layout, color parsing, and game-module CSS/HTML sample updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Container as Flex Container
participant Engine as layoutFlex()
participant Measure as Content Measurement
participant Distributor as Free-Space Distributor
participant Items as Flex Items
participant Nested as Nested Flex / Reflow
Container->>Engine: layoutFlex(container)
Engine->>Engine: set in-flex guard / enqueue depth
Engine->>Measure: collect child bounds & compute content extent
Measure->>Items: measure contentMainSize / fit-content where required
Engine->>Distributor: resolve auto-margins, distribute positive/negative space
Distributor->>Items: assign final main sizes
Engine->>Nested: trigger nested layout/text reflow when cross sizes change
Nested->>Items: re-measure cross sizes (step-7 remeasure)
Engine->>Engine: compute line cross sizes, clamp/stretch, position items (handle reverse)
Engine->>Container: update versions / flush queued descendant resets
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/framework/ui/uilayoutflexbox.cpp (1)
465-474:⚠️ Potential issue | 🟠 MajorSnapshot children before calling
updateSize().This loop walks the live child list and then calls
child->updateSize(). If that cascades into sibling insert/remove paths, the iteration can dangle or skip items. Iterating overgetChildren()here is much safer.Suggested fix
- for (const auto& childPtr : container.getChildrenRef()) { + const auto children = container.getChildren(); + for (const auto& childPtr : children) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/ui/uilayoutflexbox.cpp` around lines 465 - 474, The loop currently iterates the live list via container.getChildrenRef() and then calls child->updateSize(), which can mutate siblings and invalidate the iterator; change it to first snapshot the children (e.g., auto children = container.getChildren() or otherwise copy the child pointers into a local vector) and then iterate that snapshot, preserving the existing checks (getDisplay, getPositionType) and calling child->updateSize() on the copied list so mutations during updateSize() cannot dangle or skip items.src/framework/ui/uiwidgetbasestyle.cpp (1)
214-225:⚠️ Potential issue | 🟠 MajorDefault omitted
flex-basisto0%, not0px.In CSS Flexbox, the shorthand
flex: 1expands toflex-grow: 1; flex-shrink: 1; flex-basis: 0%per the CSS specification. The current code defaults to0px, which diverges when the container's main size is indefinite—now exercised by this PR's nested auto-sized flex containers.Suggested fix
- FlexBasis basis = { FlexBasis::Type::Px, 0.f }; + FlexBasis basis = { FlexBasis::Type::Percent, 0.f };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/ui/uiwidgetbasestyle.cpp` around lines 214 - 225, The flex shorthand default uses 0px currently; change it to 0% to match CSS (flex:1 -> flex-basis:0%). Specifically, in the flex parsing block where you declare FlexBasis basis = { FlexBasis::Type::Px, 0.f }; (and later call parseFlexBasis(filtered[idx]) and widget->setFlexBasis(basis)), initialize basis to a percent type with value 0 (e.g., FlexBasis::Type::Percent, 0.f) or ensure the fallback when no token is present sets percent 0 rather than px 0, then continue to call widget->setFlexBasis(basis).
🟡 Minor comments (18)
docs/exampleHTML_flex/14-responsive-sidebar.html-50-51 (1)
50-51:⚠️ Potential issue | 🟡 MinorIncomplete responsive implementation.
The comment at line 50 and description (lines 63-64) claim the sidebar "stacks above the main content on small screens using flex-direction change," but no CSS rule implements this behavior. The responsive stacking rule is missing.
If OTClient's UI system doesn't support media queries, consider updating the description to clarify this is a demonstration of the sidebar pattern rather than actual responsive behavior, or add a note about how responsiveness would be achieved in this system.
Also applies to: 63-64
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/exampleHTML_flex/14-responsive-sidebar.html` around lines 50 - 51, The comment claims the sidebar stacks above main on small screens but no responsive rule is present; either add a media-query-based rule that switches the layout container (selecting .container) from row to column so .sidebar appears above .main on narrow viewports, or, if OTClient UI doesn't support media queries, update the explanatory text referring to the pattern (lines describing responsive behavior) to state this is a non-responsive demonstration and explain how responsiveness would be achieved (e.g., change .container flex-direction to column via media query) instead of asserting it already happens.docs/exampleHTML_flex/04-flex-wrap-gap.html-77-82 (1)
77-82:⚠️ Potential issue | 🟡 MinorMissing border-style in shorthand.
Same issue:
border: 1px#888;needs a style value.🔧 Proposed fix
img { width: 28px; height: 28px; border-radius: 5px; - border: 1px `#888`; + border: 1px solid `#888`; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/exampleHTML_flex/04-flex-wrap-gap.html` around lines 77 - 82, The CSS rule for the img selector uses a shorthand border without a style ("border: 1px `#888`;"), which is invalid; update the img rule (selector "img") to include a border-style (for example use "border: 1px solid `#888`;") so the border width, style and color are all specified.modules/game_forge/game_forge.css-399-407 (1)
399-407:⚠️ Potential issue | 🟡 MinorRemove unnecessary empty line before declaration.
Stylelint flagged an unexpected empty line before the
display: flexdeclaration. This affects consistency with other selectors in this file.🔧 Proposed fix
.history-footer { - display: flex; flex-direction: row;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/game_forge/game_forge.css` around lines 399 - 407, There is an extra blank line before the first declaration in the .history-footer rule; remove the empty line so the block starts immediately with "display: flex" to satisfy stylelint and match the file's selector formatting (look for the .history-footer CSS rule in the stylesheet and delete the lone blank line preceding its declarations).docs/exampleHTML_flex/04-flex-wrap-gap.html-31-37 (1)
31-37:⚠️ Potential issue | 🟡 MinorMissing border-style in shorthand.
Same issue:
border: 2px#96a19b;needs a style value.🔧 Proposed fix
.container { display: flex; width: 420px; - border: 2px `#96a19b`; + border: 2px dashed `#96a19b`; padding: 10px;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/exampleHTML_flex/04-flex-wrap-gap.html` around lines 31 - 37, The CSS .container rule uses a border shorthand missing the border-style (currently `border: 2px `#96a19b`;`); update the .container declaration (in the selector `.container`) to include a valid border-style (e.g., `solid`, `dashed`, etc.) so the shorthand becomes `border: 2px solid `#96a19b`;` (or another chosen style) to ensure the border renders correctly.docs/exampleHTML_flex/04-flex-wrap-gap.html-22-29 (1)
22-29:⚠️ Potential issue | 🟡 MinorAdd missing border-style to border shorthand declarations.
Lines 23, 34, and 81 specify
borderwithout a style value (e.g.,solid). Without the style component, the border defaults tononeand will not render visually. Change toborder: 2px solid#2e7d32;(line 23),border: 2px solid#96a19b;(line 34), andborder: 1px solid#888;(line 81).🔧 Proposed fix
.example { - border: 2px `#2e7d32`; + border: 2px solid `#2e7d32`; border-radius: 10px;.container { display: flex; width: 420px; - border: 2px `#96a19b`; + border: 2px solid `#96a19b`; padding: 10px;img { width: 28px; height: 28px; border-radius: 5px; - border: 1px `#888`; + border: 1px solid `#888`; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/exampleHTML_flex/04-flex-wrap-gap.html` around lines 22 - 29, The border shorthand declarations are missing the border-style so they render as none; update the three CSS rules that set border to include a style: change the .example rule to use "border: 2px solid `#2e7d32`;", update the second selector that currently uses "border: 2px `#96a19b`;" to "border: 2px solid `#96a19b`;", and update the third selector that currently uses "border: 1px `#888`;" to "border: 1px solid `#888`;".docs/exampleHTML_flex/03-align-items.html-116-117 (1)
116-117:⚠️ Potential issue | 🟡 MinorFix the intro text typo.
“Viewtical” should be “Vertical”.
📝 Suggested copy fix
- <p>Viewtical alignment inside the flex container (row direction).</p> + <p>Vertical alignment inside the flex container (row direction).</p>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/exampleHTML_flex/03-align-items.html` around lines 116 - 117, Replace the typo in the paragraph text inside the example HTML (the <p> element following the <h1>Flexbox: align-items</h1>) by changing "Viewtical alignment inside the flex container (row direction)." to "Vertical alignment inside the flex container (row direction)."docs/exampleHTML_flex/07-flex-flow-shorthand.html-142-143 (1)
142-143:⚠️ Potential issue | 🟡 MinorClean up the example copy.
The title repeats “flex shorthand”, and the intro is missing the space in “combines flex-direction”, so the page reads unfinished.
📝 Suggested copy fix
- <h1>flex-flow shorthand flex shorthand</h1> - <p>flex-flow combinesflex-direction + flex-wrap. flex combines grow + shrink + basis.</p> + <h1>Flexbox: flex-flow and flex shorthand</h1> + <p>flex-flow combines flex-direction + flex-wrap. flex combines flex-grow + flex-shrink + flex-basis.</p>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/exampleHTML_flex/07-flex-flow-shorthand.html` around lines 142 - 143, Update the example copy to remove repetition and fix spacing: change the H1 text from "flex-flow shorthand flex shorthand" to a concise title like "flex-flow shorthand" (referencing the <h1> element) and correct the paragraph text in the <p> element to read "flex-flow combines flex-direction + flex-wrap. flex combines grow + shrink + basis." ensuring there is a space between "combines" and "flex-direction" and the sentence reads clearly.docs/exampleHTML_flex/13-card-grid-wrap.html-73-77 (1)
73-77:⚠️ Potential issue | 🟡 MinorUse valid CSS syntax in the browser-parity example.
text-wrap: trueis not valid CSS; the property accepts keyword values only (wrap,nowrap,balance,pretty,stable). Similarly,style="height: 32; width: 32;"uses unitless lengths, which are invalid for size properties—they must include units likepx. The same invalid inline sizing pattern repeats on the later cards.♻️ Suggested cleanup
.card-text { color: `#555`; flex: 1; - text-wrap: true; } ... - <img src="/data/images/clienticon.png" style="height: 32; width: 32;" /> + <img src="/data/images/clienticon.png" style="height: 32px; width: 32px;" />Also applies to: 107-107
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/exampleHTML_flex/13-card-grid-wrap.html` around lines 73 - 77, Update the invalid CSS and inline sizing: in the .card-text rule replace the nonstandard "text-wrap: true" with a valid keyword (e.g., "text-wrap: wrap" or another appropriate keyword) and update all inline style attributes like style="height: 32; width: 32;" on the card elements to include units (e.g., "height: 32px; width: 32px;") and fix the same unitless sizes repeated later so all size properties use valid CSS units.docs/exampleHTML_flex/10-nested-flex.html-168-168 (1)
168-168:⚠️ Potential issue | 🟡 MinorFix typo in demo description text.
Line 168 has “Viewy common”; this should be “Very common”.
Suggested fix
- <p>Flex inside flex. Viewy common in card and list layouts.</p> + <p>Flex inside flex. Very common in card and list layouts.</p>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/exampleHTML_flex/10-nested-flex.html` at line 168, Fix the typo in the demo paragraph content: locate the <p> element that currently reads "Flex inside flex. Viewy common in card and list layouts." and change "Viewy common" to "Very common" so the text reads "Flex inside flex. Very common in card and list layouts."docs/exampleHTML_flex/08-margin-auto.html-1-1 (1)
1-1:⚠️ Potential issue | 🟡 MinorAdd the missing
<!DOCTYPE html>declaration.The file currently starts with
<html>on line 1, missing the required DOCTYPE declaration for valid HTML5 documents. Add<!DOCTYPE html>as the first line.Suggested fix
+<!DOCTYPE html> <html>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/exampleHTML_flex/08-margin-auto.html` at line 1, The document starts with the <html> tag but lacks the HTML5 DOCTYPE declaration; add the declaration <!DOCTYPE html> as the very first line of the file so the browser treats the document as valid HTML5 before the existing <html> element.docs/exampleHTML_flex/12-navbar-patterns.html-1-1 (1)
1-1:⚠️ Potential issue | 🟡 MinorAdd the DOCTYPE declaration at the top of the document.
The file starts with
<html>but is missing the<!DOCTYPE html>declaration. HTML5 documents should begin with a DOCTYPE declaration for standards-compliant rendering and validation.Suggested fix
+<!DOCTYPE html> <html>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/exampleHTML_flex/12-navbar-patterns.html` at line 1, Add the HTML5 DOCTYPE declaration by inserting <!DOCTYPE html> as the very first line before the existing <html> tag so the document starts with the standard HTML5 doctype; ensure the <!DOCTYPE html> appears above the <html> element in the file.docs/exampleHTML_flex/01-flex-direction.html-1-1 (1)
1-1:⚠️ Potential issue | 🟡 MinorAdd HTML doctype declaration.
The file is missing
<!DOCTYPE html>at the beginning, which causes linting failures for thedoctype-firstrule. HTML documents should declare the doctype before the<html>tag.Suggested fix
+<!DOCTYPE html> <html>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/exampleHTML_flex/01-flex-direction.html` at line 1, Add the HTML5 doctype declaration before the opening <html> tag to satisfy the doctype-first rule; update the file so the very first line is the doctype declaration (<!DOCTYPE html>) followed by the existing <html> element.docs/exampleHTML_flex/18-kanban-advanced.html-1-1 (1)
1-1:⚠️ Potential issue | 🟡 MinorAdd the missing HTML doctype.
Line 1 starts with
<html>without<!DOCTYPE html>, which is required for proper HTML document validation.Suggested fix
+<!DOCTYPE html> <html>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/exampleHTML_flex/18-kanban-advanced.html` at line 1, Add the HTML5 doctype declaration before the opening <html> tag: insert <!DOCTYPE html> immediately above the existing <html> element so the document begins with the standard doctype declaration (<!DOCTYPE html>) followed by the existing <html> tag.docs/exampleHTML_flex/10-nested-flex.html-1-1 (1)
1-1:⚠️ Potential issue | 🟡 MinorAdd
<!DOCTYPE html>before<html>.The file is missing the DOCTYPE declaration at the beginning. HTML files should declare the document type to ensure proper parsing.
Suggested fix
+<!DOCTYPE html> <html>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/exampleHTML_flex/10-nested-flex.html` at line 1, Add the HTML5 doctype declaration by inserting <!DOCTYPE html> immediately before the existing <html> tag so the document begins with the proper DOCTYPE declaration to ensure correct parsing by browsers; update the top of the file where the <html> element is defined.docs/exampleHTML_flex/20-chat-layout-flex.html-1-1 (1)
1-1:⚠️ Potential issue | 🟡 MinorAdd
<!DOCTYPE html>at the top of the file.The file is missing the DOCTYPE declaration. The
doctype-firstrule is enabled in.htmlhintrcand requires this declaration to be the first line of the HTML file.Suggested fix
+<!DOCTYPE html> <html>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/exampleHTML_flex/20-chat-layout-flex.html` at line 1, Add the HTML5 doctype declaration as the very first line by inserting <!DOCTYPE html> immediately before the existing <html> tag so the page satisfies the `doctype-first` rule and the document starts with a proper doctype.docs/exampleHTML_flex/11-holy-grail-layout.html-1-1 (1)
1-1:⚠️ Potential issue | 🟡 MinorAdd
<!DOCTYPE html>before<html>.The file starts directly with
<html>, which violates thedoctype-firstlinting rule. Note: this issue affects all files in thedocs/exampleHTML_flex/directory and may indicate these OTML template files should be excluded from HTML linting validation.Suggested fix
+<!DOCTYPE html> <html>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/exampleHTML_flex/11-holy-grail-layout.html` at line 1, Add the HTML doctype declaration before the opening <html> tag in the affected OTML template files (those starting with the <html> tag) so the document begins with <!DOCTYPE html>; update each template in the exampleHTML_flex set to prepend the doctype or exclude these OTML templates from HTML linting if they should not be validated, ensuring the opening <html> element remains unchanged.modules/game_htmlsample/htmlsample.html-49-50 (1)
49-50:⚠️ Potential issue | 🟡 MinorUse
vertical-align: middleinstead ofcenter.
vertical-align: centeris not valid CSS for table cells. The correct keyword ismiddle. Using an invalid value means browsers ignore it, making this sample unreliable as a reference.Suggested fix
- <td style="text-align: center; vertical-align: center"> + <td style="text-align: center; vertical-align: middle">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/game_htmlsample/htmlsample.html` around lines 49 - 50, The table cell uses an invalid CSS value "vertical-align: center"; update the td style on the <td> element (the cell rendering {{index}}) to use "vertical-align: middle" instead of "center" so browsers apply vertical centering correctly.docs/exampleHTML_flex/05-flex-item-props.html-1-1 (1)
1-1:⚠️ Potential issue | 🟡 MinorAdd
<!DOCTYPE html>before this document.While omitting the doctype does trigger quirks mode in modern browsers, flexbox is specifically exempted from the percentage sizing quirks, so this demo will still behave consistently as a flex reference. However, including the doctype is a best practice to ensure standards mode for all HTML documents.
Suggested fix
+<!DOCTYPE html> <html>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/exampleHTML_flex/05-flex-item-props.html` at line 1, Add the HTML5 doctype declaration before the document root so the file that currently starts with the "<html>" element is preceded by "<!DOCTYPE html>"; update the top of the document (the part containing the "<html>" tag) to begin with the doctype to ensure standards mode.
🧹 Nitpick comments (4)
docs/exampleHTML_flex/02-justify-content.html (1)
12-14: Duplicatedisplaydeclaration.
display: block !important;appears twice (lines 12 and 14). The second declaration overrides the first, making the first one redundant.🧹 Proposed fix
display: block !important; --image-source: none; - display: block !important; width: 100% !important;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/exampleHTML_flex/02-justify-content.html` around lines 12 - 14, Remove the duplicate CSS declaration for display: block !important; in the ruleset shown (the repeated "display" property appears twice); keep a single display: block !important; and delete the redundant occurrence so the stylesheet contains only one display declaration for that selector.docs/exampleHTML_flex/15-equal-height-cards.html (1)
12-14: Duplicatedisplaydeclaration.Same pattern as other example files -
display: block !important;appears twice.🧹 Proposed fix
display: block !important; --image-source: none; - display: block !important; width: 100% !important;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/exampleHTML_flex/15-equal-height-cards.html` around lines 12 - 14, Remove the duplicate CSS declaration by keeping a single "display: block !important;" in the rule that also defines "--image-source: none" (the block containing the two identical display lines); edit the CSS so only one display declaration remains to avoid redundancy and potential confusion.docs/exampleHTML_flex/14-responsive-sidebar.html (1)
12-14: Duplicatedisplaydeclaration.
display: block !important;appears on both lines 12 and 14.🧹 Proposed fix
display: block !important; --image-source: none; - display: block !important; width: 100% !important;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/exampleHTML_flex/14-responsive-sidebar.html` around lines 12 - 14, Remove the duplicated CSS declaration "display: block !important;" so it only appears once in the rule that also contains "--image-source: none;"; locate the rule containing these declarations (the block with "--image-source: none;") and delete the redundant "display: block !important;" occurrence, leaving a single display declaration to avoid repetition.docs/exampleHTML_flex/17-sticky-footer.html (1)
11-13: Duplicatedisplaydeclaration.
display: block !important;appears on both lines 11 and 13.🧹 Proposed fix
display: block !important; --image-source: none; - display: block !important; width: 99% !important;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/exampleHTML_flex/17-sticky-footer.html` around lines 11 - 13, Remove the duplicated CSS declaration so only a single "display: block !important;" remains in the same rule block (leave the "--image-source: none;" line intact); locate the rule containing the repeated "display: block !important;" entry and delete the redundant occurrence to avoid duplicate properties.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/exampleHTML_flex/20-chat-layout-flex.html`:
- Around line 191-193: The .input CSS rule sets both background-color and color
to `#f1f5f9`, making text unreadable; update the .input selector to use a
contrasting text color (e.g., change color to a dark value like `#0f172a` or
`#111827`) or change the background to a darker color so foreground and background
meet accessible contrast—modify the .input rule in the stylesheet (the `.input`
class) to ensure readable contrast between background-color and color.
In `@modules/corelib/ui/uihtml.lua`:
- Around line 77-87: computeContentExtent currently measures span as
bounds.right - bounds.left which loses the offset from the scroll origin; change
the measurement to compute width as the distance from the padding/scroll origin
to the far edge (e.g. bounds.right - paddingRect.left) and height as
bounds.bottom - paddingRect.top, then clamp each with paddingRect.width/height
(use math.max(bounds.right - paddingRect.left, paddingRect.width) and
math.max(bounds.bottom - paddingRect.top, paddingRect.height)). Update the
computeContentExtent function (references: computeContentExtent, paddingRect,
bounds, collectContentBounds) so scrollWidth/scrollHeight reflect off-screen
content positioned away from the origin.
- Around line 53-55: The loop over widget:getChildren() currently only checks
child:isDestroyed() and child:isExplicitlyVisible(), which lets descendants with
CSS display:none contribute to bounds; update the guard to also skip children
whose computed display is "none" (the equivalent of DisplayType::None used in
C++ draw path) — e.g. query the child's display via its API (for example
child:getDisplayType() or child:getComputedStyle():display) and continue when
it's "none", so child:getMarginRect() is only used for visible, non-display-none
descendants.
In `@src/framework/ui/uilayoutflexbox.cpp`:
- Around line 1050-1055: The current clamping uses >= 0 for max-size checks
causing unset/default max values to act as hard clamps; change the checks that
use container.getMaxWidth(), container.getMaxHeight(), and any other max*
comparisons from ">= 0" to "> 0" so the max is treated as present only when
positive, leaving min-size checks as-is and ensuring desired remains clamped
only when an explicit positive max is provided (apply the same fix to the
analogous blocks around the container.getMaxWidth()/getMaxHeight() uses noted at
the other occurrences).
In `@src/framework/ui/uiwidget.cpp`:
- Around line 1155-1159: The current guard around calling updateLayout() skips
the child's own layout whenever m_parent->m_inFlexLayout is true; instead always
perform the child's local layout but suppress only the parent-relayout path.
Change the call site so it always invokes the child's layout logic (remove the
conditional skip), and modify updateLayout() (or add a new helper like
updateLocalLayout()) to accept a flag or check that prevents running the
parent/anchor relayout when m_parent && m_parent->m_inFlexLayout; reference
updateLayout(), m_parent and m_inFlexLayout to locate the changes.
In `@src/framework/ui/uiwidgetbasestyle.cpp`:
- Around line 425-429: The parser currently skips style keywords like
"none"/"hidden" in the block that checks `lower == "solid" || ...` but later
still requires `hasBorderWidth` and `hasBorderColor`, causing an exception
instead of treating `border: none`/`border: hidden` as a valid reset. Before the
detailed parsing of border tokens, detect the single-token case (token count ==
1) where the lowercased token is "none" or "hidden" and handle it as a reset:
clear/zero the border state (e.g. set width to 0 or an explicit "no border"
sentinel, set color to transparent/clear, and set `hasBorderWidth` and
`hasBorderColor` appropriately to avoid the later exception) and then
continue/return; this change should be applied around the block that currently
checks `lower == "solid" || ...` and the later validation that throws when
`!hasBorderWidth && !hasBorderColor`.
---
Outside diff comments:
In `@src/framework/ui/uilayoutflexbox.cpp`:
- Around line 465-474: The loop currently iterates the live list via
container.getChildrenRef() and then calls child->updateSize(), which can mutate
siblings and invalidate the iterator; change it to first snapshot the children
(e.g., auto children = container.getChildren() or otherwise copy the child
pointers into a local vector) and then iterate that snapshot, preserving the
existing checks (getDisplay, getPositionType) and calling child->updateSize() on
the copied list so mutations during updateSize() cannot dangle or skip items.
In `@src/framework/ui/uiwidgetbasestyle.cpp`:
- Around line 214-225: The flex shorthand default uses 0px currently; change it
to 0% to match CSS (flex:1 -> flex-basis:0%). Specifically, in the flex parsing
block where you declare FlexBasis basis = { FlexBasis::Type::Px, 0.f }; (and
later call parseFlexBasis(filtered[idx]) and widget->setFlexBasis(basis)),
initialize basis to a percent type with value 0 (e.g., FlexBasis::Type::Percent,
0.f) or ensure the fallback when no token is present sets percent 0 rather than
px 0, then continue to call widget->setFlexBasis(basis).
---
Minor comments:
In `@docs/exampleHTML_flex/01-flex-direction.html`:
- Line 1: Add the HTML5 doctype declaration before the opening <html> tag to
satisfy the doctype-first rule; update the file so the very first line is the
doctype declaration (<!DOCTYPE html>) followed by the existing <html> element.
In `@docs/exampleHTML_flex/03-align-items.html`:
- Around line 116-117: Replace the typo in the paragraph text inside the example
HTML (the <p> element following the <h1>Flexbox: align-items</h1>) by changing
"Viewtical alignment inside the flex container (row direction)." to "Vertical
alignment inside the flex container (row direction)."
In `@docs/exampleHTML_flex/04-flex-wrap-gap.html`:
- Around line 77-82: The CSS rule for the img selector uses a shorthand border
without a style ("border: 1px `#888`;"), which is invalid; update the img rule
(selector "img") to include a border-style (for example use "border: 1px solid
`#888`;") so the border width, style and color are all specified.
- Around line 31-37: The CSS .container rule uses a border shorthand missing the
border-style (currently `border: 2px `#96a19b`;`); update the .container
declaration (in the selector `.container`) to include a valid border-style
(e.g., `solid`, `dashed`, etc.) so the shorthand becomes `border: 2px solid
`#96a19b`;` (or another chosen style) to ensure the border renders correctly.
- Around line 22-29: The border shorthand declarations are missing the
border-style so they render as none; update the three CSS rules that set border
to include a style: change the .example rule to use "border: 2px solid
`#2e7d32`;", update the second selector that currently uses "border: 2px `#96a19b`;"
to "border: 2px solid `#96a19b`;", and update the third selector that currently
uses "border: 1px `#888`;" to "border: 1px solid `#888`;".
In `@docs/exampleHTML_flex/05-flex-item-props.html`:
- Line 1: Add the HTML5 doctype declaration before the document root so the file
that currently starts with the "<html>" element is preceded by "<!DOCTYPE
html>"; update the top of the document (the part containing the "<html>" tag) to
begin with the doctype to ensure standards mode.
In `@docs/exampleHTML_flex/07-flex-flow-shorthand.html`:
- Around line 142-143: Update the example copy to remove repetition and fix
spacing: change the H1 text from "flex-flow shorthand flex shorthand" to a
concise title like "flex-flow shorthand" (referencing the <h1> element) and
correct the paragraph text in the <p> element to read "flex-flow combines
flex-direction + flex-wrap. flex combines grow + shrink + basis." ensuring there
is a space between "combines" and "flex-direction" and the sentence reads
clearly.
In `@docs/exampleHTML_flex/08-margin-auto.html`:
- Line 1: The document starts with the <html> tag but lacks the HTML5 DOCTYPE
declaration; add the declaration <!DOCTYPE html> as the very first line of the
file so the browser treats the document as valid HTML5 before the existing
<html> element.
In `@docs/exampleHTML_flex/10-nested-flex.html`:
- Line 168: Fix the typo in the demo paragraph content: locate the <p> element
that currently reads "Flex inside flex. Viewy common in card and list layouts."
and change "Viewy common" to "Very common" so the text reads "Flex inside flex.
Very common in card and list layouts."
- Line 1: Add the HTML5 doctype declaration by inserting <!DOCTYPE html>
immediately before the existing <html> tag so the document begins with the
proper DOCTYPE declaration to ensure correct parsing by browsers; update the top
of the file where the <html> element is defined.
In `@docs/exampleHTML_flex/11-holy-grail-layout.html`:
- Line 1: Add the HTML doctype declaration before the opening <html> tag in the
affected OTML template files (those starting with the <html> tag) so the
document begins with <!DOCTYPE html>; update each template in the
exampleHTML_flex set to prepend the doctype or exclude these OTML templates from
HTML linting if they should not be validated, ensuring the opening <html>
element remains unchanged.
In `@docs/exampleHTML_flex/12-navbar-patterns.html`:
- Line 1: Add the HTML5 DOCTYPE declaration by inserting <!DOCTYPE html> as the
very first line before the existing <html> tag so the document starts with the
standard HTML5 doctype; ensure the <!DOCTYPE html> appears above the <html>
element in the file.
In `@docs/exampleHTML_flex/13-card-grid-wrap.html`:
- Around line 73-77: Update the invalid CSS and inline sizing: in the .card-text
rule replace the nonstandard "text-wrap: true" with a valid keyword (e.g.,
"text-wrap: wrap" or another appropriate keyword) and update all inline style
attributes like style="height: 32; width: 32;" on the card elements to include
units (e.g., "height: 32px; width: 32px;") and fix the same unitless sizes
repeated later so all size properties use valid CSS units.
In `@docs/exampleHTML_flex/14-responsive-sidebar.html`:
- Around line 50-51: The comment claims the sidebar stacks above main on small
screens but no responsive rule is present; either add a media-query-based rule
that switches the layout container (selecting .container) from row to column so
.sidebar appears above .main on narrow viewports, or, if OTClient UI doesn't
support media queries, update the explanatory text referring to the pattern
(lines describing responsive behavior) to state this is a non-responsive
demonstration and explain how responsiveness would be achieved (e.g., change
.container flex-direction to column via media query) instead of asserting it
already happens.
In `@docs/exampleHTML_flex/18-kanban-advanced.html`:
- Line 1: Add the HTML5 doctype declaration before the opening <html> tag:
insert <!DOCTYPE html> immediately above the existing <html> element so the
document begins with the standard doctype declaration (<!DOCTYPE html>) followed
by the existing <html> tag.
In `@docs/exampleHTML_flex/20-chat-layout-flex.html`:
- Line 1: Add the HTML5 doctype declaration as the very first line by inserting
<!DOCTYPE html> immediately before the existing <html> tag so the page satisfies
the `doctype-first` rule and the document starts with a proper doctype.
In `@modules/game_forge/game_forge.css`:
- Around line 399-407: There is an extra blank line before the first declaration
in the .history-footer rule; remove the empty line so the block starts
immediately with "display: flex" to satisfy stylelint and match the file's
selector formatting (look for the .history-footer CSS rule in the stylesheet and
delete the lone blank line preceding its declarations).
In `@modules/game_htmlsample/htmlsample.html`:
- Around line 49-50: The table cell uses an invalid CSS value "vertical-align:
center"; update the td style on the <td> element (the cell rendering {{index}})
to use "vertical-align: middle" instead of "center" so browsers apply vertical
centering correctly.
---
Nitpick comments:
In `@docs/exampleHTML_flex/02-justify-content.html`:
- Around line 12-14: Remove the duplicate CSS declaration for display: block
!important; in the ruleset shown (the repeated "display" property appears
twice); keep a single display: block !important; and delete the redundant
occurrence so the stylesheet contains only one display declaration for that
selector.
In `@docs/exampleHTML_flex/14-responsive-sidebar.html`:
- Around line 12-14: Remove the duplicated CSS declaration "display: block
!important;" so it only appears once in the rule that also contains
"--image-source: none;"; locate the rule containing these declarations (the
block with "--image-source: none;") and delete the redundant "display: block
!important;" occurrence, leaving a single display declaration to avoid
repetition.
In `@docs/exampleHTML_flex/15-equal-height-cards.html`:
- Around line 12-14: Remove the duplicate CSS declaration by keeping a single
"display: block !important;" in the rule that also defines "--image-source:
none" (the block containing the two identical display lines); edit the CSS so
only one display declaration remains to avoid redundancy and potential
confusion.
In `@docs/exampleHTML_flex/17-sticky-footer.html`:
- Around line 11-13: Remove the duplicated CSS declaration so only a single
"display: block !important;" remains in the same rule block (leave the
"--image-source: none;" line intact); locate the rule containing the repeated
"display: block !important;" entry and delete the redundant occurrence to avoid
duplicate properties.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 709d6815-67cb-4ba6-ad42-78ce914082af
📒 Files selected for processing (33)
docs/exampleHTML_flex/01-flex-direction.htmldocs/exampleHTML_flex/02-justify-content.htmldocs/exampleHTML_flex/03-align-items.htmldocs/exampleHTML_flex/04-flex-wrap-gap.htmldocs/exampleHTML_flex/05-flex-item-props.htmldocs/exampleHTML_flex/06-align-content.htmldocs/exampleHTML_flex/07-flex-flow-shorthand.htmldocs/exampleHTML_flex/08-margin-auto.htmldocs/exampleHTML_flex/09-row-gap-column-gap.htmldocs/exampleHTML_flex/10-nested-flex.htmldocs/exampleHTML_flex/11-holy-grail-layout.htmldocs/exampleHTML_flex/12-navbar-patterns.htmldocs/exampleHTML_flex/13-card-grid-wrap.htmldocs/exampleHTML_flex/14-responsive-sidebar.htmldocs/exampleHTML_flex/15-equal-height-cards.htmldocs/exampleHTML_flex/16-center-both-axes.htmldocs/exampleHTML_flex/17-sticky-footer.htmldocs/exampleHTML_flex/18-kanban-advanced.htmldocs/exampleHTML_flex/19-dashboard-shell.htmldocs/exampleHTML_flex/20-chat-layout-flex.htmldocs/exampleHTML_flex/21-master-detail-flex.htmlmodules/corelib/ui/uihtml.luamodules/game_forge/game_forge.cssmodules/game_htmlsample/htmlsample.cssmodules/game_htmlsample/htmlsample.htmlmodules/game_htmlsample/htmlsample.luasrc/framework/ui/uilayoutflexbox.cppsrc/framework/ui/uiwidget.cppsrc/framework/ui/uiwidget.hsrc/framework/ui/uiwidgetbasestyle.cppsrc/framework/ui/uiwidgethtml.cppsrc/framework/ui/uiwidgettext.cppsrc/framework/util/color.cpp
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/framework/ui/uiwidgetbasestyle.cpp (1)
716-727: Minor inconsistency with left/right margin handling.Lines 716-717 unconditionally call
setMarginTop(top)andsetMarginBottom(bottom), then lines 719-727 override for auto cases. This differs from the left/right handling (lines 729-741) which conditionally calls the setter only in the non-auto branch.Both approaches work correctly, but the top/bottom path does redundant work when auto is specified. Consider aligning with the left/right pattern for consistency.
Align with left/right pattern
- setMarginTop(top); - setMarginBottom(bottom); - if (topAuto) { m_margin.top = top; setMarginTopAuto(true); + } else { + setMarginTop(top); } if (bottomAuto) { m_margin.bottom = bottom; setMarginBottomAuto(true); + } else { + setMarginBottom(bottom); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/ui/uiwidgetbasestyle.cpp` around lines 716 - 727, The top/bottom margin code unconditionally calls setMarginTop(top) and setMarginBottom(bottom) before handling auto, causing redundant work and inconsistent style with the left/right branch; update the logic in the block that currently uses setMarginTop/setMarginBottom and the auto checks so that you only call setMarginTop(top) when topAuto is false and only call setMarginBottom(bottom) when bottomAuto is false, and keep the auto branch behavior that sets m_margin.top/m_margin.bottom and calls setMarginTopAuto(true)/setMarginBottomAuto(true) when the corresponding auto flag is true, mirroring the left/right pattern.src/framework/ui/uiwidget.cpp (1)
1156-1176: Confusing variable name:movedOnlydoesn't mean "only moved".The variable
movedOnlyat line 1156 is actuallypositionChanged— it's true whenever the position changed, regardless of whether the size also changed. The current name implies exclusivity ("moved AND nothing else changed") but that's not what's computed.This causes confusion in the condition at line 1176 where
movedOnlygates entry, but line 1177 then checkssizeChangedseparately. The code works correctly, but the naming is misleading.Suggested rename for clarity
- const bool movedOnly = oldRect.topLeft() != clampedRect.topLeft(); + const bool positionChanged = oldRect.topLeft() != clampedRect.topLeft(); const bool sizeChanged = oldRect.size() != clampedRect.size(); ... - movedOnly) { + positionChanged) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/ui/uiwidget.cpp` around lines 1156 - 1176, Rename the misleading boolean movedOnly to positionChanged and update all references in uiwidget.cpp (where movedOnly is computed from oldRect.topLeft() != clampedRect.topLeft() and used in the flex-entry if) so the name accurately reflects "position changed" semantics; ensure the declaration and the condition that currently reads movedOnly (near the if that also checks sizeChanged, isOnHtml(), m_inFlexLayout, m_parent, and m_displayType/DisplayType::Flex|InlineFlex) are updated to positionChanged to improve readability without changing logic, and run a quick compile/search to update any other uses (e.g., comments, tests) that mention movedOnly.src/framework/ui/uilayoutflexbox.cpp (1)
157-176: Performance consideration: recursive version reset on all descendants.
resetDescendantVersionsrecursively traverses all descendants and resets their size versions, forcing recalculation on the next layout pass. While correct, this is called multiple times during a flex pass (lines 534, 857, 1127), potentially causing redundant traversals for deeply nested trees.For heavily nested layouts, consider batching or deferring these resets to reduce traversal overhead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/ui/uilayoutflexbox.cpp` around lines 157 - 176, The recursive resetDescendantVersions(UIWidget*) causes repeated full-tree traversals; change to a batched/deferred approach by (1) adding a marker or queue (e.g., a global/scope vector or a bool flag like pendingVersionReset on UIWidget) to collect root widgets that need descendant resets instead of calling resetDescendantVersions immediately from the flex pass callers, (2) modify the places that currently call resetDescendantVersions (the flex-pass call sites) to instead mark or enqueue the root widget, and (3) run a single traversal pass at the end of the flex layout phase that walks each queued root and performs the same logic currently in resetDescendantVersions (using getChildrenRef(), getWidthHtml(), getHeightHtml(), isDestroyed(), getDisplay(), Unit checks) and clears the marker/queue; this avoids redundant traversals for deeply nested trees while preserving the exact version/reset semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/framework/ui/uilayoutflexbox.cpp`:
- Around line 157-176: The recursive resetDescendantVersions(UIWidget*) causes
repeated full-tree traversals; change to a batched/deferred approach by (1)
adding a marker or queue (e.g., a global/scope vector or a bool flag like
pendingVersionReset on UIWidget) to collect root widgets that need descendant
resets instead of calling resetDescendantVersions immediately from the flex pass
callers, (2) modify the places that currently call resetDescendantVersions (the
flex-pass call sites) to instead mark or enqueue the root widget, and (3) run a
single traversal pass at the end of the flex layout phase that walks each queued
root and performs the same logic currently in resetDescendantVersions (using
getChildrenRef(), getWidthHtml(), getHeightHtml(), isDestroyed(), getDisplay(),
Unit checks) and clears the marker/queue; this avoids redundant traversals for
deeply nested trees while preserving the exact version/reset semantics.
In `@src/framework/ui/uiwidget.cpp`:
- Around line 1156-1176: Rename the misleading boolean movedOnly to
positionChanged and update all references in uiwidget.cpp (where movedOnly is
computed from oldRect.topLeft() != clampedRect.topLeft() and used in the
flex-entry if) so the name accurately reflects "position changed" semantics;
ensure the declaration and the condition that currently reads movedOnly (near
the if that also checks sizeChanged, isOnHtml(), m_inFlexLayout, m_parent, and
m_displayType/DisplayType::Flex|InlineFlex) are updated to positionChanged to
improve readability without changing logic, and run a quick compile/search to
update any other uses (e.g., comments, tests) that mention movedOnly.
In `@src/framework/ui/uiwidgetbasestyle.cpp`:
- Around line 716-727: The top/bottom margin code unconditionally calls
setMarginTop(top) and setMarginBottom(bottom) before handling auto, causing
redundant work and inconsistent style with the left/right branch; update the
logic in the block that currently uses setMarginTop/setMarginBottom and the auto
checks so that you only call setMarginTop(top) when topAuto is false and only
call setMarginBottom(bottom) when bottomAuto is false, and keep the auto branch
behavior that sets m_margin.top/m_margin.bottom and calls
setMarginTopAuto(true)/setMarginBottomAuto(true) when the corresponding auto
flag is true, mirroring the left/right pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1e09397a-dcd5-4322-9e5f-02350043c114
📒 Files selected for processing (4)
modules/corelib/ui/uihtml.luasrc/framework/ui/uilayoutflexbox.cppsrc/framework/ui/uiwidget.cppsrc/framework/ui/uiwidgetbasestyle.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- modules/corelib/ui/uihtml.lua
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/framework/ui/uilayoutflexbox.cpp (2)
963-968: Manual save/restore ofpendingUpdatecould use RAII.Lines 963-968 manually save and restore
crossUnit.pendingUpdate, but this isn't exception-safe. IflayoutFlex(*item.widget)throws,pendingUpdatewon't be restored.Consider using a small RAII helper for consistency with
SizeUnitGuard:♻️ Suggested fix
if (widthChanged || item.widget->getHeight() <= 0) { - const bool origPending = crossUnit.pendingUpdate; - crossUnit.pendingUpdate = false; - layoutFlex(*item.widget); - crossUnit.pendingUpdate = origPending; + struct PendingGuard { + bool& ref; + bool orig; + PendingGuard(bool& r) : ref(r), orig(r) { ref = false; } + ~PendingGuard() { ref = orig; } + } guard(crossUnit.pendingUpdate); + layoutFlex(*item.widget); }Alternatively, rely on the assumption that
layoutFlexdoes not throw.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/ui/uilayoutflexbox.cpp` around lines 963 - 968, The code manually saves and restores crossUnit.pendingUpdate around layoutFlex(*item.widget), which is not exception-safe; create and use a small RAII helper (similar to SizeUnitGuard) that takes a reference to crossUnit.pendingUpdate, stores the original value in its constructor, sets pendingUpdate to false, and restores the original value in its destructor, then replace the manual save/restore around layoutFlex in the block that currently references crossUnit.pendingUpdate so the flag is always restored even if layoutFlex throws.
587-594: State modification persists after restoring unit.After the fit-content measurement,
preferredCrossUnit.unitis restored, butpendingUpdateandversionremain modified (trueand0). This intentionally triggers subsequent re-measurement, but could cause unexpected behavior if other code paths assume these fields are synchronized withunit.The implementation appears intentional based on the comment at line 626-627 ("leave pendingUpdate=true and version=0 so subsequent passes pick up the measured size"), but ensure this doesn't conflict with caching logic elsewhere.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/ui/uilayoutflexbox.cpp` around lines 587 - 594, preferredCrossUnit.pendingUpdate and preferredCrossUnit.version are being left modified after temporarily switching preferredCrossUnit.unit to Unit::FitContent for measurement, which can desync state; save the original pendingUpdate and version values before setting pendingUpdate=true and version=0, call child->updateSize(), then restore the original pendingUpdate and version after restoring preferredCrossUnit.unit (while still keeping the temporary unit change around the update call), i.e., capture originalPendingUpdate and originalVersion, set pendingUpdate=true/version=0, call child->updateSize(), restore preferredCrossUnit.unit, then restore pendingUpdate and version to the saved originals to keep state consistent with unit.src/framework/ui/uiwidgetbasestyle.cpp (2)
707-728: RedundantisAutoKeywordcalls.Lines 711-712 call
isAutoKeyword(topStr)andisAutoKeyword(bottomStr)again despite already havingtopAutoandbottomAutostored on lines 707 and 709.♻️ Suggested fix
const bool topAuto = isAutoKeyword(topStr); const bool rightAuto = isAutoKeyword(rightStr); const bool bottomAuto = isAutoKeyword(bottomStr); const bool leftAuto = isAutoKeyword(leftStr); - const int top = isAutoKeyword(topStr) ? 0 : stdext::to_number(topStr); - const int bottom = isAutoKeyword(bottomStr) ? 0 : stdext::to_number(bottomStr); + const int top = topAuto ? 0 : stdext::to_number(topStr); + const int bottom = bottomAuto ? 0 : stdext::to_number(bottomStr); const int right = rightAuto ? 0 : stdext::to_number(rightStr); const int left = leftAuto ? 0 : stdext::to_number(leftStr);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/ui/uiwidgetbasestyle.cpp` around lines 707 - 728, The code redundantly calls isAutoKeyword(topStr) and isAutoKeyword(bottomStr) when topAuto and bottomAuto are already computed; update the computation of top and bottom to use the stored booleans (topAuto and bottomAuto) instead of calling isAutoKeyword again (keep using rightAuto/leftAuto as-is), so replace the expressions that compute top and bottom with ternaries that reference topAuto and bottomAuto respectively while leaving the later uses of m_margin, setMarginTopAuto/setMarginTop and setMarginBottomAuto/setMarginBottom unchanged.
642-650: Minor inconsistency in margin assignment pattern.When
autois detected, the code directly assignsm_margin.top = 0(orm_margin.bottom = 0), but when non-auto, it callssetMarginTop()/setMarginBottom(). If the setter methods have side effects (like clearing the auto flag, triggering validation, or updating layout), bypassing them could cause subtle issues.Consider using the setter consistently:
} else if (node->tag() == "margin-top") { auto value = node->value<std::string>(); if (isAutoKeyword(value)) { - m_margin.top = 0; + setMarginTop(0); setMarginTopAuto(true); } else { setMarginTop(stdext::to_number(value)); }However, since
setMarginTopAuto(true)already callsupdateParentLayout(), the current approach may be intentional to avoid redundant layout updates. If so, this is acceptable.Also applies to: 659-667
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/ui/uiwidgetbasestyle.cpp` around lines 642 - 650, The code currently writes m_margin.top = 0 (and m_margin.bottom = 0) directly when encountering the "auto" keyword, bypassing the setter methods; replace those direct assignments with calls to the setters so side effects and validation run consistently — e.g. call setMarginTop(0) then setMarginTopAuto(true) for the "margin-top" branch and setMarginBottom(0) then setMarginBottomAuto(true) for the "margin-bottom" branch (use the setMarginTop, setMarginTopAuto, setMarginBottom and setMarginBottomAuto symbols to locate the logic), or if you intentionally want to avoid duplicate layout updates keep only setMarginTopAuto(true)/setMarginBottomAuto(true) and remove the direct m_margin.* = 0 assignments for consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/framework/ui/uilayoutflexbox.cpp`:
- Around line 195-229: queueDescendantVersionReset currently stores raw
UIWidget* in s_pendingDescendantVersionResetRootsByDepth which can dangle if
widgets are destroyed during layout; update the logic to avoid UB by either (A)
changing the container types s_pendingDescendantVersionResetRootsByDepth and
related s_pendingDescendantVersionResetLookupByDepth to store UIWidgetPtr (or
the project’s owning/shared widget smart pointer) so queued widgets are kept
alive, or (B) if ownership changes are unacceptable, ensure every use path
(queueDescendantVersionReset and
flushQueuedDescendantVersionResetsForCurrentDepth) validates pointers before
dereference by checking for nullptr or a widget.isDestroyed()/isAlive() API and
skipping/erasing destroyed entries; modify queueDescendantVersionReset to insert
UIWidgetPtr (or perform a defensive conversion) and adjust lookup operations to
use the same key type so lookup/erase behavior remains consistent.
- Around line 28-33: The three inline globals (s_flexDepth,
s_pendingDescendantVersionResetRootsByDepth,
s_pendingDescendantVersionResetLookupByDepth) are shared across threads and can
race; change them to thread-local storage so each thread has independent layout
state: replace "inline int s_flexDepth = 0;" with "thread_local int s_flexDepth
= 0;" and similarly make s_pendingDescendantVersionResetRootsByDepth and
s_pendingDescendantVersionResetLookupByDepth thread_local, ensuring they are
sized/initialized to MAX_FLEX_DEPTH where previously expected; keep
MAX_FLEX_DEPTH unchanged and update any code that assumes a single global (e.g.,
functions referencing these symbols during layoutFlex or re-entrance checks such
as m_inFlexLayout) to use the new thread_local variables.
---
Nitpick comments:
In `@src/framework/ui/uilayoutflexbox.cpp`:
- Around line 963-968: The code manually saves and restores
crossUnit.pendingUpdate around layoutFlex(*item.widget), which is not
exception-safe; create and use a small RAII helper (similar to SizeUnitGuard)
that takes a reference to crossUnit.pendingUpdate, stores the original value in
its constructor, sets pendingUpdate to false, and restores the original value in
its destructor, then replace the manual save/restore around layoutFlex in the
block that currently references crossUnit.pendingUpdate so the flag is always
restored even if layoutFlex throws.
- Around line 587-594: preferredCrossUnit.pendingUpdate and
preferredCrossUnit.version are being left modified after temporarily switching
preferredCrossUnit.unit to Unit::FitContent for measurement, which can desync
state; save the original pendingUpdate and version values before setting
pendingUpdate=true and version=0, call child->updateSize(), then restore the
original pendingUpdate and version after restoring preferredCrossUnit.unit
(while still keeping the temporary unit change around the update call), i.e.,
capture originalPendingUpdate and originalVersion, set
pendingUpdate=true/version=0, call child->updateSize(), restore
preferredCrossUnit.unit, then restore pendingUpdate and version to the saved
originals to keep state consistent with unit.
In `@src/framework/ui/uiwidgetbasestyle.cpp`:
- Around line 707-728: The code redundantly calls isAutoKeyword(topStr) and
isAutoKeyword(bottomStr) when topAuto and bottomAuto are already computed;
update the computation of top and bottom to use the stored booleans (topAuto and
bottomAuto) instead of calling isAutoKeyword again (keep using
rightAuto/leftAuto as-is), so replace the expressions that compute top and
bottom with ternaries that reference topAuto and bottomAuto respectively while
leaving the later uses of m_margin, setMarginTopAuto/setMarginTop and
setMarginBottomAuto/setMarginBottom unchanged.
- Around line 642-650: The code currently writes m_margin.top = 0 (and
m_margin.bottom = 0) directly when encountering the "auto" keyword, bypassing
the setter methods; replace those direct assignments with calls to the setters
so side effects and validation run consistently — e.g. call setMarginTop(0) then
setMarginTopAuto(true) for the "margin-top" branch and setMarginBottom(0) then
setMarginBottomAuto(true) for the "margin-bottom" branch (use the setMarginTop,
setMarginTopAuto, setMarginBottom and setMarginBottomAuto symbols to locate the
logic), or if you intentionally want to avoid duplicate layout updates keep only
setMarginTopAuto(true)/setMarginBottomAuto(true) and remove the direct
m_margin.* = 0 assignments for consistency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 66028282-f531-4851-9f16-33b5fd753549
📒 Files selected for processing (2)
src/framework/ui/uilayoutflexbox.cppsrc/framework/ui/uiwidgetbasestyle.cpp
Contributor
Author
|
@coderabbitai help |
|
The PR description image shows the OTClient HTML/CSS module running a "Holy Grail Layout & Flexbox" demo — a three-column layout with a blue header, left sidebar navigation, main article content area, right pink sidebar ("Widgets"), and a footer with "Made with Flexbox." The tab strip shows "Current Demo" and "Flex Examples" tabs, with the examples dropdown set to
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (1)
docs/exampleHTML_flex/20-chat-layout-flex.html (1)
191-193:⚠️ Potential issue | 🟠 MajorComposer input text is still invisible.
background-colorandcolorare both#f1f5f9, so typed text disappears against the input fill. This was already flagged on the previous revision and is still present.Suggested fix
.input { flex: 1 0 360px; width: 360px; min-width: 300px; background-color: `#f1f5f9`; - color: `#f1f5f9`; + color: `#0f172a`; border: 1px solid `#cbd5e1`; border-radius: 8px;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/exampleHTML_flex/20-chat-layout-flex.html` around lines 191 - 193, The composer input's text is invisible because the CSS rule sets both background-color and color to `#f1f5f9`; update the rule that currently contains "background-color: `#f1f5f9`; color: `#f1f5f9`;" (the composer input style in 20-chat-layout-flex.html) so the text color contrasts with the background (for example change color to a dark value like `#0f172a` or `#111827`, or remove the duplicate color declaration), ensuring the composer input text is readable.
🧹 Nitpick comments (10)
src/framework/util/color.cpp (1)
248-311: Consider reusing existing helper functions.The lambdas
clamp255,strip_spaces,split_commas,parse_byte_or_percent,parse_alpha_any, andhsl_to_rgbdefined here duplicate the namespace-scope static functions at lines 160-223. Consider removing these lambdas and calling the existing helpers directly to reduce duplication.This is a pre-existing issue and can be addressed separately from this PR.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/util/color.cpp` around lines 248 - 311, The block defines local lambdas clamp255, strip_spaces, split_commas, parse_byte_or_percent, parse_alpha_any, and hsl_to_rgb which duplicate existing namespace-scope helper functions; remove these local lambdas and replace their uses with calls to the existing helper functions of the same logical purpose (use the existing clamp255/strip_spaces/split_commas/parse_byte_or_percent/parse_alpha_any and the namespace hsl_to_rgb helper), ensuring any captured variables or references are adjusted (pass r,g,b by reference to the existing hsl_to_rgb) and that includes/using directives make the helpers visible in this translation unit.docs/exampleHTML_flex/15-equal-height-cards.html (1)
12-14: Remove duplicatedisplay: block !important;declaration.Same issue as other example files – lines 12 and 14 both declare
display: block !important;.🧹 Proposed fix
display: block !important; --image-source: none; - display: block !important; width: 100% !important;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/exampleHTML_flex/15-equal-height-cards.html` around lines 12 - 14, Remove the duplicate CSS declaration by keeping only one instance of "display: block !important;" and deleting the redundant line so the block contains a single "display: block !important;" alongside the "--image-source: none;" declaration; locate the duplicate by searching for the "display: block !important;" lines in the same rule (the ones surrounding the "--image-source: none;" declaration) and remove the extra occurrence.docs/exampleHTML_flex/08-margin-auto.html (1)
12-14: Remove duplicatedisplay: block !important;declaration.Lines 12 and 14 duplicate the same declaration.
🧹 Proposed fix
display: block !important; --image-source: none; - display: block !important; width: 100% !important;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/exampleHTML_flex/08-margin-auto.html` around lines 12 - 14, The CSS contains a duplicated declaration "display: block !important;" (appearing twice in the same rule); remove the redundant occurrence so only a single "display: block !important;" remains and keep the other declarations like "--image-source: none;" untouched to avoid duplicate property definitions.docs/exampleHTML_flex/14-responsive-sidebar.html (1)
12-14: Remove duplicatedisplay: block !important;declaration.Lines 12 and 14 both declare
display: block !important;. Remove the duplicate.🧹 Proposed fix
display: block !important; --image-source: none; - display: block !important; width: 100% !important;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/exampleHTML_flex/14-responsive-sidebar.html` around lines 12 - 14, Remove the duplicate CSS declaration "display: block !important;" so the block only appears once; locate the repeated lines around the "--image-source: none;" declaration in the responsive sidebar snippet and delete one of the two identical "display: block !important;" entries to avoid redundancy.docs/exampleHTML_flex/18-kanban-advanced.html (1)
12-14: Remove duplicatedisplay: block !important;declaration.Lines 12 and 14 both declare
display: block !important;.🧹 Proposed fix
display: block !important; --image-source: none; - display: block !important; width: 100% !important;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/exampleHTML_flex/18-kanban-advanced.html` around lines 12 - 14, Remove the duplicated CSS declaration: in the CSS block that currently contains two identical "display: block !important;" lines (the block also sets "--image-source: none;"), keep a single "display: block !important;" and delete the redundant one so the rule contains only one display declaration.modules/game_forge/game_forge.css (1)
400-407: Remove empty line before declaration.Stylelint flags an unexpected empty line before
display: flex. Remove the blank line at line 400 for consistent formatting.🧹 Proposed fix
.history-footer { - display: flex; flex-direction: row;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/game_forge/game_forge.css` around lines 400 - 407, Remove the unexpected blank line immediately before the "display: flex;" declaration in the CSS rule block (the block that contains display: flex, flex-direction: row, flex-wrap: nowrap, align-items: stretch, justify-content: flex-start, gap: 0) so the declarations start directly after the opening brace; simply delete the empty line preceding "display: flex" to satisfy stylelint and restore consistent formatting.docs/exampleHTML_flex/17-sticky-footer.html (1)
11-13: Remove duplicatedisplay: block !important;declaration.Lines 11 and 13 both declare
display: block !important;.🧹 Proposed fix
display: block !important; --image-source: none; - display: block !important; width: 99% !important;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/exampleHTML_flex/17-sticky-footer.html` around lines 11 - 13, Remove the duplicate CSS declaration "display: block !important;" so it only appears once; in the block containing "--image-source: none" (the snippet showing two identical "display: block !important;" lines), keep a single "display: block !important;" (preferably the first) and delete the redundant one to avoid repetition.docs/exampleHTML_flex/10-nested-flex.html (1)
40-46: Keep structural layout rules out of the theme selector.
.example.purplecurrently owns the first demo's width/box-sizing/flex-column behavior, so changing only the color variant would also change layout semantics. This would be safer on.exampleitself or on a dedicated helper class for the card-row example.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/exampleHTML_flex/10-nested-flex.html` around lines 40 - 46, The CSS in .example.purple contains structural layout rules (width, box-sizing, display, flex-direction, align-items) and should not live on the color variant; move those structural rules from .example.purple into the base .example (or into a dedicated helper class like .card-row) and leave .example.purple to only define color-related styles. Update references to .example.purple in markup if you choose a helper class so the layout classes and color variant are applied separately.docs/exampleHTML_flex/11-holy-grail-layout.html (1)
23-28: Consider sizing this demo to the preview area instead of a fixed 500px box.For a holy-grail example, the interesting part is the fill-available-height behavior. Hard-coding
height: 500pxmakes the demo less representative and can clip or leave dead space depending on the sample pane size.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/exampleHTML_flex/11-holy-grail-layout.html` around lines 23 - 28, The .page rule currently hard-codes height: 500px which prevents the demo from sizing to the preview area; remove the fixed height and replace it with a fill-available rule (e.g., use min-height: 100vh or min-height: 100%/height: 100% with the root/preview container given 100% height) so the .page element will stretch to the available preview height and demonstrate the holy-grail fill-available behavior. Reference: .page selector and its height property.docs/exampleHTML_flex/03-align-items.html (1)
155-162: Make the baseline example visually stronger.Both text items use the same font metrics, so this ends up looking close to bottom alignment. Giving one child a noticeably larger font size or line-height would make the
baselinebehavior much clearer.Possible tweak
- <div class="item1"><span>Small text</span></div> - <div class="item1"><span>Large text</span></div> + <div class="item1"><span style="font-size: 12px;">Small text</span></div> + <div class="item1"><span style="font-size: 22px;">Large text</span></div> <div class="item1"><img src="/data/images/clienticon.png" alt="img" style="border:1px solid `#777`; border-radius:4px;" /></div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/exampleHTML_flex/03-align-items.html` around lines 155 - 162, The baseline example's alignment isn't clear because both children share the same font metrics; update the .container baseline example so one of its children (e.g., the second .item1 or the span inside it) has a noticeably larger font-size and/or increased line-height (and keep the image item unchanged) to make the baseline behavior visually obvious; locate the HTML block with the container class "baseline" and modify the second .item1 (or its inner span) to use larger typography so the baseline shift is visible.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/exampleHTML_flex/07-flex-flow-shorthand.html`:
- Around line 84-115: Update the inline comments and visible heading text so the
shorthand labels and descriptions match the actual flex values exactly: correct
the comment annotations for .f-auto, .f-none, .f-1, .f-2, .f-3, and .f-0-200 to
reflect their true computed flex shorthand (e.g., "flex: auto", "flex: none",
"flex: 1" etc.), and fix the heading/description spacing/separators in the
example HTML so titles/readme text are properly spaced and punctuated; apply the
same corrections to the duplicate section indicated around lines 142-143.
In `@docs/exampleHTML_flex/09-row-gap-column-gap.html`:
- Around line 88-106: The stylesheet gap properties (gap, row-gap, column-gap)
are currently ignored by the UI style parser; update
src/framework/ui/uiwidgetbasestyle.cpp to parse and apply these properties by
adding handling in the style parsing path (e.g., extend parseStyleProperty or
the function that handles CSS property tokens) to set widget style members
rowGap, columnGap and gap, and ensure applyLayoutSpacing or the layout-measure
step consumes those members to compute spacing; add fallback logic so gap sets
both rowGap and columnGap when present and ensure numeric values (e.g., "16px")
are converted consistently with existing spacing units.
In `@docs/exampleHTML_flex/12-navbar-patterns.html`:
- Around line 40-68: The middle cluster drifts because .nav1 .links uses
margin-left:auto and margin-right:auto while .nav1 .cta adds a third auto margin
(margin-left:auto); remove the asymmetric auto margin on .nav1 .cta and either
(a) center .nav1 .links with margin: 0 auto and let .cta sit without auto
margins, or (b) convert the layout to a true flex spacer pattern by having .nav1
as display:flex with explicit left and right side wrappers (or spacer elements
with flex:1) so the centered .links stays centered; update the selectors .nav1
.links and .nav1 .cta accordingly to implement one of these fixes.
In `@docs/exampleHTML_flex/14-responsive-sidebar.html`:
- Around line 50-51: The comment claims a responsive "flex-direction change"
that isn't implemented; either implement it or correct the comment: add a media
query (e.g., `@media` (max-width: 600px)) that switches the layout by setting
.container { flex-direction: column; } and adjusting .sidebar and .main sizing
so the sidebar stacks above the main, or update the comments on the
.container/.sidebar/.main sections to state that the sidebar stays left and the
main grows (removing references to stacking/flex-direction change). Ensure edits
touch the .container, .sidebar, and .main selectors to keep behavior and docs
consistent.
In `@src/framework/ui/uilayoutflexbox.cpp`:
- Around line 804-809: Re-apply the single-line definite-cross-size override
after the step 7 recompute that recalculates line cross-sizes but before the
align-content logic runs: after the block that recomputes line heights from item
dimensions (the step 7 recompute) and immediately before the align-content
application, re-check the exact condition (!crossAuto && lines.size() == 1) and
set lines[0].crossSize to std::max(0.0, innerCrossSize) so the single-line
override is not later overwritten by the align-content handling.
In `@src/framework/ui/uiwidget.cpp`:
- Around line 1195-1203: The direct m_rect.translate(delta) fast path leaves
text hitbox cache m_rectToWord stale, breaking getTextByPos(); update the same
code path that currently calls w->m_rect.translate(delta) to also adjust cached
rects: either translate every Rect in w->m_rectToWord by the same delta or
clear/invalidate the cache so cacheRectToWord() will rebuild on next access;
target the block where w->m_rect is translated and modify m_rectToWord
accordingly to keep cache consistent with the moved widget.
In `@src/framework/ui/uiwidget.h`:
- Around line 821-829: The margin setters (setMargin, setMarginHorizontal,
setMarginVertical, setMarginTop/Right/Bottom/Left and the auto setters
setMarginTopAuto/setMarginBottomAuto) currently only call updateParentLayout();
they must also trigger the HTML flex invalidation path used for flex-item
placement. After the existing updateParentLayout() call in each of these
methods, call scheduleHtmlTask() (the same function used in layout invalidation
for HTML flex containers) so that layoutFlex() will be queued and flex
positioning is recomputed for margin-top/ bottom/ numeric changes on flex items.
In `@src/framework/ui/uiwidgethtml.cpp`:
- Around line 660-665: applyFitContentRecursive() currently special-cases only
row flex via parentIsRowFlex and uses breakLine(c->getDisplay()), which
mis-measures column flex containers and ignores gap; update the logic around
parentIsRowFlex/shouldBreak so that you detect any flex parent (use
isFlexContainer(w->getDisplay()) and w->getFlexDirection()) and: for
Row/RowReverse treat children as non-breaking flex items (shouldBreak = false),
for Column/ColumnReverse treat each child as stacked (shouldBreak = true)
instead of deferring to breakLine(c->getDisplay()); additionally, when
accumulating runWidth and totalHeight add the container’s gap spacing between
items (add gap for every item after the first) so fit-content measurement
includes gaps before layoutFlex() runs. Ensure references to
applyFitContentRecursive(), parentIsRowFlex (or renamed parentIsFlex),
shouldBreak, breakLine(c->getDisplay()), runWidth, totalHeight, and layoutFlex()
are updated accordingly.
In `@src/framework/ui/uiwidgettext.cpp`:
- Around line 646-649: The reset logic only updates this widget's
m_textWrapOptions so inherited parent options can persist; change these
assignments to modify the effective owner options (use getTextWrapOptions() or
the parent m_textWrapOptions reference) so removal of "overflow-wrap" or
"word-break" on the parent resets the child's effective options; specifically,
replace assignments to this->m_textWrapOptions.overflowWrapMode =
OverflowWrapMode::Normal and this->m_textWrapOptions.wordBreakMode =
WordBreakMode::Normal with updates to the owner returned by getTextWrapOptions()
(or m_parent->m_textWrapOptions) so updateText() sees the cleared defaults.
---
Duplicate comments:
In `@docs/exampleHTML_flex/20-chat-layout-flex.html`:
- Around line 191-193: The composer input's text is invisible because the CSS
rule sets both background-color and color to `#f1f5f9`; update the rule that
currently contains "background-color: `#f1f5f9`; color: `#f1f5f9`;" (the composer
input style in 20-chat-layout-flex.html) so the text color contrasts with the
background (for example change color to a dark value like `#0f172a` or `#111827`, or
remove the duplicate color declaration), ensuring the composer input text is
readable.
---
Nitpick comments:
In `@docs/exampleHTML_flex/03-align-items.html`:
- Around line 155-162: The baseline example's alignment isn't clear because both
children share the same font metrics; update the .container baseline example so
one of its children (e.g., the second .item1 or the span inside it) has a
noticeably larger font-size and/or increased line-height (and keep the image
item unchanged) to make the baseline behavior visually obvious; locate the HTML
block with the container class "baseline" and modify the second .item1 (or its
inner span) to use larger typography so the baseline shift is visible.
In `@docs/exampleHTML_flex/08-margin-auto.html`:
- Around line 12-14: The CSS contains a duplicated declaration "display: block
!important;" (appearing twice in the same rule); remove the redundant occurrence
so only a single "display: block !important;" remains and keep the other
declarations like "--image-source: none;" untouched to avoid duplicate property
definitions.
In `@docs/exampleHTML_flex/10-nested-flex.html`:
- Around line 40-46: The CSS in .example.purple contains structural layout rules
(width, box-sizing, display, flex-direction, align-items) and should not live on
the color variant; move those structural rules from .example.purple into the
base .example (or into a dedicated helper class like .card-row) and leave
.example.purple to only define color-related styles. Update references to
.example.purple in markup if you choose a helper class so the layout classes and
color variant are applied separately.
In `@docs/exampleHTML_flex/11-holy-grail-layout.html`:
- Around line 23-28: The .page rule currently hard-codes height: 500px which
prevents the demo from sizing to the preview area; remove the fixed height and
replace it with a fill-available rule (e.g., use min-height: 100vh or
min-height: 100%/height: 100% with the root/preview container given 100% height)
so the .page element will stretch to the available preview height and
demonstrate the holy-grail fill-available behavior. Reference: .page selector
and its height property.
In `@docs/exampleHTML_flex/14-responsive-sidebar.html`:
- Around line 12-14: Remove the duplicate CSS declaration "display: block
!important;" so the block only appears once; locate the repeated lines around
the "--image-source: none;" declaration in the responsive sidebar snippet and
delete one of the two identical "display: block !important;" entries to avoid
redundancy.
In `@docs/exampleHTML_flex/15-equal-height-cards.html`:
- Around line 12-14: Remove the duplicate CSS declaration by keeping only one
instance of "display: block !important;" and deleting the redundant line so the
block contains a single "display: block !important;" alongside the
"--image-source: none;" declaration; locate the duplicate by searching for the
"display: block !important;" lines in the same rule (the ones surrounding the
"--image-source: none;" declaration) and remove the extra occurrence.
In `@docs/exampleHTML_flex/17-sticky-footer.html`:
- Around line 11-13: Remove the duplicate CSS declaration "display: block
!important;" so it only appears once; in the block containing "--image-source:
none" (the snippet showing two identical "display: block !important;" lines),
keep a single "display: block !important;" (preferably the first) and delete the
redundant one to avoid repetition.
In `@docs/exampleHTML_flex/18-kanban-advanced.html`:
- Around line 12-14: Remove the duplicated CSS declaration: in the CSS block
that currently contains two identical "display: block !important;" lines (the
block also sets "--image-source: none;"), keep a single "display: block
!important;" and delete the redundant one so the rule contains only one display
declaration.
In `@modules/game_forge/game_forge.css`:
- Around line 400-407: Remove the unexpected blank line immediately before the
"display: flex;" declaration in the CSS rule block (the block that contains
display: flex, flex-direction: row, flex-wrap: nowrap, align-items: stretch,
justify-content: flex-start, gap: 0) so the declarations start directly after
the opening brace; simply delete the empty line preceding "display: flex" to
satisfy stylelint and restore consistent formatting.
In `@src/framework/util/color.cpp`:
- Around line 248-311: The block defines local lambdas clamp255, strip_spaces,
split_commas, parse_byte_or_percent, parse_alpha_any, and hsl_to_rgb which
duplicate existing namespace-scope helper functions; remove these local lambdas
and replace their uses with calls to the existing helper functions of the same
logical purpose (use the existing
clamp255/strip_spaces/split_commas/parse_byte_or_percent/parse_alpha_any and the
namespace hsl_to_rgb helper), ensuring any captured variables or references are
adjusted (pass r,g,b by reference to the existing hsl_to_rgb) and that
includes/using directives make the helpers visible in this translation unit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 00e479a2-4a08-43d0-83d9-52fa754bf5cf
📒 Files selected for processing (33)
docs/exampleHTML_flex/01-flex-direction.htmldocs/exampleHTML_flex/02-justify-content.htmldocs/exampleHTML_flex/03-align-items.htmldocs/exampleHTML_flex/04-flex-wrap-gap.htmldocs/exampleHTML_flex/05-flex-item-props.htmldocs/exampleHTML_flex/06-align-content.htmldocs/exampleHTML_flex/07-flex-flow-shorthand.htmldocs/exampleHTML_flex/08-margin-auto.htmldocs/exampleHTML_flex/09-row-gap-column-gap.htmldocs/exampleHTML_flex/10-nested-flex.htmldocs/exampleHTML_flex/11-holy-grail-layout.htmldocs/exampleHTML_flex/12-navbar-patterns.htmldocs/exampleHTML_flex/13-card-grid-wrap.htmldocs/exampleHTML_flex/14-responsive-sidebar.htmldocs/exampleHTML_flex/15-equal-height-cards.htmldocs/exampleHTML_flex/16-center-both-axes.htmldocs/exampleHTML_flex/17-sticky-footer.htmldocs/exampleHTML_flex/18-kanban-advanced.htmldocs/exampleHTML_flex/19-dashboard-shell.htmldocs/exampleHTML_flex/20-chat-layout-flex.htmldocs/exampleHTML_flex/21-master-detail-flex.htmlmodules/corelib/ui/uihtml.luamodules/game_forge/game_forge.cssmodules/game_htmlsample/htmlsample.cssmodules/game_htmlsample/htmlsample.htmlmodules/game_htmlsample/htmlsample.luasrc/framework/ui/uilayoutflexbox.cppsrc/framework/ui/uiwidget.cppsrc/framework/ui/uiwidget.hsrc/framework/ui/uiwidgetbasestyle.cppsrc/framework/ui/uiwidgethtml.cppsrc/framework/ui/uiwidgettext.cppsrc/framework/util/color.cpp
src/framework/ui/uiwidget.h
Outdated
Comment on lines
+821
to
+829
| void setMargin(const int margin) { m_margin.set(margin); m_marginTopAuto = m_marginBottomAuto = m_marginLeftAuto = m_marginRightAuto = false; updateParentLayout(); } | ||
| void setMarginHorizontal(const int margin) { m_margin.right = m_margin.left = margin; m_marginLeftAuto = m_marginRightAuto = false; updateParentLayout(); } | ||
| void setMarginVertical(const int margin) { m_margin.bottom = m_margin.top = margin; updateParentLayout(); } | ||
| void setMarginTop(const int margin) { m_margin.top = margin; updateParentLayout(); } | ||
| void setMarginVertical(const int margin) { m_margin.bottom = m_margin.top = margin; m_marginTopAuto = m_marginBottomAuto = false; updateParentLayout(); } | ||
| void setMarginTop(const int margin) { m_margin.top = margin; m_marginTopAuto = false; updateParentLayout(); } | ||
| void setMarginRight(const int margin) { m_margin.right = margin; m_marginRightAuto = false; updateParentLayout(); } | ||
| void setMarginBottom(const int margin) { m_margin.bottom = margin; updateParentLayout(); } | ||
| void setMarginBottom(const int margin) { m_margin.bottom = margin; m_marginBottomAuto = false; updateParentLayout(); } | ||
| void setMarginLeft(const int margin) { m_margin.left = margin; m_marginLeftAuto = false; updateParentLayout(); } | ||
| void setMarginTopAuto(bool v = true) { m_marginTopAuto = v; updateParentLayout(); } | ||
| void setMarginBottomAuto(bool v = true) { m_marginBottomAuto = v; updateParentLayout(); } |
There was a problem hiding this comment.
Margin setters still miss the HTML flex invalidation path.
These setters only call updateParentLayout(). In src/framework/ui/uiwidget.cpp, Lines 829-833 only queue the parent UILayout, but HTML flex containers are recomputed through scheduleHtmlTask() / layoutFlex(). A runtime margin-top:auto, margin-bottom:auto, or numeric margin change on a flex item can therefore keep stale placement until some unrelated HTML task happens to rerun layout.
🔧 Minimal fix pattern
void setMarginTopAuto(bool v = true) {
m_marginTopAuto = v;
updateParentLayout();
+ if (isOnHtml()) {
+ scheduleHtmlTask(PropUpdateSize);
+ scheduleHtmlTask(PropApplyAnchorAlignment);
+ }
}Apply the same invalidation pattern to the other margin setters in this block.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/framework/ui/uiwidget.h` around lines 821 - 829, The margin setters
(setMargin, setMarginHorizontal, setMarginVertical,
setMarginTop/Right/Bottom/Left and the auto setters
setMarginTopAuto/setMarginBottomAuto) currently only call updateParentLayout();
they must also trigger the HTML flex invalidation path used for flex-item
placement. After the existing updateParentLayout() call in each of these
methods, call scheduleHtmlTask() (the same function used in layout invalidation
for HTML flex containers) so that layoutFlex() will be queued and flex
positioning is recomputed for margin-top/ bottom/ numeric changes on flex items.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/framework/ui/uiwidget.h (1)
821-831:⚠️ Potential issue | 🟠 Major
margin-left/right:autostill misses the HTML flex invalidation path.This block fixes the numeric and vertical auto-margin setters, but
setMarginLeftAutoandsetMarginRightAutoon Lines 830-831 still only callupdateParentLayout(). For HTML flex parents that does not queue the flex HTML task, so toggling main-axis auto margins at runtime can leave the item in a stale position until some unrelated HTML update happens.Suggested fix
- void setMarginLeftAuto(bool v = true) { m_marginLeftAuto = v; updateParentLayout(); } - void setMarginRightAuto(bool v = true) { m_marginRightAuto = v; updateParentLayout(); } + void setMarginLeftAuto(bool v = true) { m_marginLeftAuto = v; updateParentLayout(); scheduleParentHtmlFlexLayout(); } + void setMarginRightAuto(bool v = true) { m_marginRightAuto = v; updateParentLayout(); scheduleParentHtmlFlexLayout(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/ui/uiwidget.h` around lines 821 - 831, setMarginLeftAuto and setMarginRightAuto currently only call updateParentLayout(), missing the HTML flex invalidation path; update both methods (setMarginLeftAuto(bool) and setMarginRightAuto(bool)) to also call scheduleParentHtmlFlexLayout() after updateParentLayout() so that toggling left/right auto margins triggers the HTML flex re-layout for flex parents.
🧹 Nitpick comments (6)
modules/game_htmlsample/htmlsample.lua (4)
180-201: Inconsistent indentation in palette array.The first element
"red"has different indentation than the rest of the array elements, which affects readability.✏️ Fix indentation
local palette = { - "red", - "blue", + "red", + "blue",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/game_htmlsample/htmlsample.lua` around lines 180 - 201, The palette table has inconsistent indentation for its first element ("red") which harms readability; open the palette declaration (variable name "palette") and align all string entries to the same indentation level (indent the "red" entry to match the other lines) so every element in the table uses consistent spacing and column alignment.
8-10: Defensive check could fail silently ifmodules.game_thingsis nil.The function returns
nil(falsy) whenmodules.game_thingsisnil, which works but is inconsistent with the explicitfalsepattern used elsewhere in this file (e.g., cycle event returns).💡 Suggested improvement
function HtmlSample:isThingsLoaded() - return modules.game_things and modules.game_things.isLoaded() + return modules.game_things ~= nil and modules.game_things.isLoaded() end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/game_htmlsample/htmlsample.lua` around lines 8 - 10, HtmlSample:isThingsLoaded currently returns nil when modules.game_things is missing; change it to explicitly return false in that case by checking modules.game_things first and only calling modules.game_things.isLoaded() when present, otherwise return false so the function consistently yields a boolean (refer to HtmlSample:isThingsLoaded and modules.game_things.isLoaded).
49-61: Magic numbers for schedule delays reduce readability.The delays
111,1, and later30,80,90are unexplained. Consider extracting these as named constants or adding brief comments explaining why these specific values were chosen.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/game_htmlsample/htmlsample.lua` around lines 49 - 61, Replace magic numeric delays used in scheduleEvent calls with named constants and/or brief comments explaining their purpose; e.g., define descriptive constants like EXAMPLES_RENDER_DELAY, RESPONSIVE_INIT_DELAY, RESPONSIVE_STEP_DELAY, RESPONSIVE_START_DELAY and use them in the calls to self:scheduleEvent around renderSelectedExample, updateResponsiveViewport, and startResponsiveDemo so the intent of 111, 1, 30, 80, 90 is clear and maintainable.
144-169: Multiple deferred layout refreshes may be unnecessary overhead.Scheduling
refreshPreviewLayoutthree times at 1ms, 30ms, and 80ms suggests timing uncertainty. While this works, it indicates a potential underlying issue with layout timing. Consider whether a single deferred call with a longer delay would suffice, or investigate why multiple refreshes are needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/game_htmlsample/htmlsample.lua` around lines 144 - 169, The code currently schedules refreshPreviewLayout three times (via self:scheduleEvent) at 1, 30 and 80 ms which is likely redundant; replace these multiple calls with a single deferred refresh (choose a single reasonable delay, e.g. 30ms) or implement a simple debounce/once-only scheduling around refreshPreviewLayout so it only queues one future execution (check/track a boolean like refreshScheduled or use existing scheduler token) and call self:scheduleEvent(refreshPreviewLayout, <delay>) only when not already scheduled; target the refreshPreviewLayout closure and the three self:scheduleEvent(...) calls in this block to make the change.modules/game_htmlsample/htmlsample.css (1)
127-141: Inconsistent gap unit usage:0pxvs unitless values elsewhere.Line 131 uses
gap: 0pxwhile other properties in this file use unitless values (e.g., line 84gap: 8px, line 132width: 480). For a zero value, the unit is unnecessary. Consider being consistent with the codebase conventions.✏️ Minor consistency fix
flex-wrap: wrap; align-content: flex-start; - gap: 0px; + gap: 0; width: 480;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/game_htmlsample/htmlsample.css` around lines 127 - 141, The stylesheet uses inconsistent unit notation for zero values: in .responsiveViewport the property gap is set as "0px" while other numeric properties in this file are unitless (e.g., width: 480) — change gap: 0px to gap: 0 to match the project’s unitless convention for zero values and keep formatting consistent within the .responsiveViewport rule (also review .responsiveBox if similar unit issues exist).src/framework/ui/uiwidget.h (1)
717-718: Avoid exposing a mutable reference tom_children.Returning
UIWidgetList&lets any caller bypassaddChild/removeChild/moveChildToIndex, which can desyncm_childrenById,m_childIndex, parent pointers, and layout bookkeeping. SincelayoutFlexis already friended, I'd keep only the const accessor public.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/framework/ui/uiwidget.h` around lines 717 - 718, The public non-const accessor getChildrenRef() exposes m_children and allows external mutation that will bypass invariants maintained by addChild/removeChild/moveChildToIndex and corrupt m_childrenById, m_childIndex, parent pointers and layout bookkeeping; remove or make the non-const UIWidgetList& getChildrenRef() inaccessible (e.g., delete it or move it to private/protected) and keep only the const UIWidgetList& getChildrenRef() public, leaving layoutFlex (already friended) able to access m_children when needed so all external modifications go through addChild/removeChild/moveChildToIndex.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@modules/game_forge/game_forge.css`:
- Around line 399-406: The .history-footer rule block contains an extraneous
blank line that triggers Stylelint's declaration-empty-line-before rule; remove
the stray empty line inside the .history-footer CSS block so declarations follow
directly (no blank line) to satisfy the linter and keep the stylesheet
lint-clean.
In `@modules/game_htmlsample/htmlsample.css`:
- Around line 62-74: Stylelint is flagging the OTUI custom type selector
"window" used in selectors like ".examplePreview window" and ".examplePreview >
.exampleRoot" as unknown; update the project's Stylelint config
(.stylelintrc.json) to allow OTUI types by either adding a
"selector-type-allowed-list" that includes "window" (and other OTUI types like
"uicreature") or by adjusting "selector-no-unknown" to ignore custom type
selectors so these valid selectors aren't reported as errors.
In `@modules/game_htmlsample/htmlsample.lua`:
- Around line 87-96: The code sets self.selectedExampleFile = htmlFiles[1] and
handles the nil case, but the error message uses a hard-coded 'ejemplosFlex'
path; update the call to self:showExampleMessage to reference the actual path
variable self.exampleBasePath (e.g., self:showExampleMessage('No .html files
found in ' .. self.exampleBasePath)) so users see the correct directory, and
keep the existing nil check around self.selectedExampleFile and the calls to
combo:setCurrentOption, self.isExamplesTab and self:renderSelectedExample as
they are.
In `@src/framework/ui/uiwidget.cpp`:
- Around line 1156-1204: The fast-path currently treats any positionChanged as a
pure translation; narrow it so it only runs for pure translations by also
verifying the size hasn't changed (compare oldRect.size() vs clampedRect.size()
or check width/height equality) before computing delta and shifting descendants.
Update the condition that gates the translation block (the if using isOnHtml(),
m_inFlexLayout, m_parent->m_inFlexLayout, m_displayType and positionChanged) to
require no size change (e.g., && oldRect.size() == clampedRect.size()), so flex
layout/rewrapping still runs when the container is resized.
---
Duplicate comments:
In `@src/framework/ui/uiwidget.h`:
- Around line 821-831: setMarginLeftAuto and setMarginRightAuto currently only
call updateParentLayout(), missing the HTML flex invalidation path; update both
methods (setMarginLeftAuto(bool) and setMarginRightAuto(bool)) to also call
scheduleParentHtmlFlexLayout() after updateParentLayout() so that toggling
left/right auto margins triggers the HTML flex re-layout for flex parents.
---
Nitpick comments:
In `@modules/game_htmlsample/htmlsample.css`:
- Around line 127-141: The stylesheet uses inconsistent unit notation for zero
values: in .responsiveViewport the property gap is set as "0px" while other
numeric properties in this file are unitless (e.g., width: 480) — change gap:
0px to gap: 0 to match the project’s unitless convention for zero values and
keep formatting consistent within the .responsiveViewport rule (also review
.responsiveBox if similar unit issues exist).
In `@modules/game_htmlsample/htmlsample.lua`:
- Around line 180-201: The palette table has inconsistent indentation for its
first element ("red") which harms readability; open the palette declaration
(variable name "palette") and align all string entries to the same indentation
level (indent the "red" entry to match the other lines) so every element in the
table uses consistent spacing and column alignment.
- Around line 8-10: HtmlSample:isThingsLoaded currently returns nil when
modules.game_things is missing; change it to explicitly return false in that
case by checking modules.game_things first and only calling
modules.game_things.isLoaded() when present, otherwise return false so the
function consistently yields a boolean (refer to HtmlSample:isThingsLoaded and
modules.game_things.isLoaded).
- Around line 49-61: Replace magic numeric delays used in scheduleEvent calls
with named constants and/or brief comments explaining their purpose; e.g.,
define descriptive constants like EXAMPLES_RENDER_DELAY, RESPONSIVE_INIT_DELAY,
RESPONSIVE_STEP_DELAY, RESPONSIVE_START_DELAY and use them in the calls to
self:scheduleEvent around renderSelectedExample, updateResponsiveViewport, and
startResponsiveDemo so the intent of 111, 1, 30, 80, 90 is clear and
maintainable.
- Around line 144-169: The code currently schedules refreshPreviewLayout three
times (via self:scheduleEvent) at 1, 30 and 80 ms which is likely redundant;
replace these multiple calls with a single deferred refresh (choose a single
reasonable delay, e.g. 30ms) or implement a simple debounce/once-only scheduling
around refreshPreviewLayout so it only queues one future execution (check/track
a boolean like refreshScheduled or use existing scheduler token) and call
self:scheduleEvent(refreshPreviewLayout, <delay>) only when not already
scheduled; target the refreshPreviewLayout closure and the three
self:scheduleEvent(...) calls in this block to make the change.
In `@src/framework/ui/uiwidget.h`:
- Around line 717-718: The public non-const accessor getChildrenRef() exposes
m_children and allows external mutation that will bypass invariants maintained
by addChild/removeChild/moveChildToIndex and corrupt m_childrenById,
m_childIndex, parent pointers and layout bookkeeping; remove or make the
non-const UIWidgetList& getChildrenRef() inaccessible (e.g., delete it or move
it to private/protected) and keep only the const UIWidgetList& getChildrenRef()
public, leaving layoutFlex (already friended) able to access m_children when
needed so all external modifications go through
addChild/removeChild/moveChildToIndex.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2103533c-8796-4041-9e40-159908074a6c
📒 Files selected for processing (9)
modules/game_forge/game_forge.cssmodules/game_htmlsample/htmlsample.cssmodules/game_htmlsample/htmlsample.htmlmodules/game_htmlsample/htmlsample.luasrc/framework/ui/uilayoutflexbox.cppsrc/framework/ui/uiwidget.cppsrc/framework/ui/uiwidget.hsrc/framework/ui/uiwidgethtml.cppsrc/framework/ui/uiwidgettext.cpp
🚧 Files skipped from review as they are similar to previous changes (3)
- modules/game_htmlsample/htmlsample.html
- src/framework/ui/uilayoutflexbox.cpp
- src/framework/ui/uiwidgethtml.cpp
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
1 participant
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.
Warning
This was made with AI, I’m a noob in .cpp
I’m not a web designer
2026-03-24.17-23-31.mp4
example:
Flexbox Complete Support
The engine now implements a full flex layout flow, including container and item properties, wrapping, spacing, reverse directions, and nested flex containers.
Flex Container Properties
flex-direction:row,row-reverse,column,column-reverseflex-wrap:nowrap,wrap,wrap-reverseflex-flow: shorthand forflex-direction+flex-wrapjustify-content:flex-start,flex-end,center,space-between,space-around,space-evenlyalign-items:stretch,flex-start,flex-end,center,baselinealign-content:stretch,flex-start,flex-end,center,space-between,space-around,space-evenlygap,row-gap,column-gap: spacing between flex items/linesFlex Item Properties
order: visual ordering of itemsflex-grow: free-space growth factorflex-shrink: shrink factor when space is missingflex-basis:auto,content,<number>,<number>px,<number>%flex: shorthand (auto,none, orgrow shrink basis)align-self:auto,stretch,flex-start,flex-end,center,baselineFlex Behavior Notes
margin: auto(or side-specificmargin-left/right/top/bottom: auto) is supported for flex items and absorbs free space on the main axis.row-reverse,column-reverse, andwrap-reverseare supported.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Style