Feat/css grid integration tests part3 #785
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds many CSS Grid integration tests across parsing, layout, animation, edge cases, and interactions; extends parser shorthand handling and inset shorthand support; adjusts display resolution and length percentage rules; major grid layout changes (scrollable size, out-of-flow child handling); and small config/docs/.gitignore updates. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
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 (3)
integration_tests/specs/css/css-text-mixin/text_comprehensive_test.ts (1)
74-74: Add a comment explaining why this test is skipped, or re-enable it if accidentally disabled.The test at line 74 uses
xit()but has no comment documenting why it's skipped. The test implementation appears complete and functional. If this is intentional (e.g., pending implementation of a feature or known limitation), add a comment explaining the reason. If it was accidentally disabled, re-enable it by changingxit()toit().cli/src/react.ts (1)
517-531: Inconsistent array type handling in multi-component path.The multi-component generation path (lines 517-531) still uses
getPointerType(type.value)directly instead ofgenRT(type.value), which was updated in the single-component path (lines 389-394). This inconsistency could lead to different type output for the same input depending on the code path.🔎 Proposed fix to align with single-component path
if (type.isArray && typeof type.value === 'object' && !Array.isArray(type.value)) { - const elemType = getPointerType(type.value); - if (elemType === 'Type') return 'any[]'; - if (typeof elemType === 'string' && elemType.startsWith('typeof ')) { - const ident = elemType.substring('typeof '.length).trim(); - return `(typeof __webfTypes.${ident})[]`; - } - if (typeof elemType === 'string' && /^(?:[A-Za-z_][A-Za-z0-9_]*)(?:\.[A-Za-z_][A-Za-z0-9_]*)*$/.test(elemType)) { - const base = elemType.split('.')[0]; - if (!localTypeNames.has(base)) { - return `__webfTypes.${elemType}[]`; - } - } - return `${elemType}[]`; + const elemType = genRT(type.value); + if (!elemType) return 'any[]'; + if (/^[A-Za-z_][A-Za-z0-9_]*(?:\\.[A-Za-z_][A-Za-z0-9_]*)*$/.test(elemType)) { + return `${elemType}[]`; + } + return `(${elemType})[]`; }bridge/bindings/qjs/script_promise_resolver.cc (1)
51-74: Remove prematureSetIsIdle(true)call before promise resolution.The
SetIsIdle(true)at line 54 is called before the promise resolver/rejecter function executes viaJS_Call(). This marks the context as idle while JavaScript code that may trigger additional work (nested promises, microtasks) is still running. The idle state should only be set after all promise resolution and microtask draining completes. Remove the earlySetIsIdle(true)call;DrainMicrotasks()already callsSetIsIdle(true)at the proper time via its internal call toDrainPendingPromiseJobs().
🟡 Minor comments (13)
.gitmodules-5-7 (1)
5-7: Explicitly specify the branch for the flutter_easy_refresh submodule.The repository is accessible, but it contains only a single branch (
v3). Without an explicitbranch = v3specification in the submodule configuration, the pin relies on the repository's default branch—unlike thequickjssubmodule (line 4) which explicitly specifiesbranch = feat/official_version.Add
branch = v3to match the pattern used forquickjsand ensure stability if additional branches are added to the repository in the future.integration_tests/specs/css/css-grid/grid-model/grid-items-types.ts-27-27 (1)
27-27: Use.toBeCloseTo()for floating-point width comparisons.
getBoundingClientRect()returns floating-point DOMRect values. Using.toBe()for exact equality can cause flaky tests if sub-pixel rounding occurs across different rendering contexts or platforms.🔎 Proposed fix
- expect(items[0].getBoundingClientRect().width).toBe(120); + expect(items[0].getBoundingClientRect().width).toBeCloseTo(120, 0);Apply the same pattern to lines 58, 146, and 175.
Also applies to: 58-58, 146-146, 175-175
integration_tests/specs/css/css-grid/layout-algorithm/circular-dependencies.ts-156-156 (1)
156-156: Grid width exceeds the documented maximum.The grid width is set to
400px, but according to the coding guidelines, "The max width of testing window is 340px". This may cause issues with test rendering or viewport constraints.Consider reducing the width to fit within the 340px constraint, or verify that exceeding this limit is acceptable for this specific test case.
- grid.style.width = '400px'; + grid.style.width = '300px';Based on coding guidelines for integration tests.
integration_tests/specs/rendering/widget_cupertino_context_menu_popup_preview_width.ts-102-102 (1)
102-102: Use idiomatic viewport restoration pattern.Replace
await resizeViewport(-1, -1)withawait resizeViewport(). The function has default parameters (width = -1, height = -1) for restoration, making the explicit arguments redundant. This matches the pattern used elsewhere in the test suite (e.g.,specs/svg/styling/display.ts).integration_tests/specs/css/css-grid/edge-cases/single-cell.ts-144-159 (1)
144-159: Fix incorrect width assertion for empty grid.Line 156 asserts that the grid's width equals the body's width, but the grid has
gridTemplateColumns: '200px', which sets a fixed width. A 200px grid will not match the body width (which can be up to 340px per test constraints). This assertion will fail.🔎 Suggested fix
Either:
- Assert the expected fixed width:
- expect(grid.getBoundingClientRect().width).toBe(document.body.getBoundingClientRect().width); + expect(grid.getBoundingClientRect().width).toBe(200);
- Or if you want to test full-width behavior, change the grid template:
- grid.style.gridTemplateColumns = '200px'; + grid.style.gridTemplateColumns = '1fr';integration_tests/specs/css/css-grid/grid-model/grid-containers.ts-31-64 (1)
31-64: Grid width exceeds maximum testing window width.The test sets
grid.style.width = '400px'(line 34), but the coding guidelines specify "The max width of testing window is 340px". This may cause the grid to extend beyond the visible area, potentially affecting snapshot comparisons and test reliability.Consider reducing the width or using relative sizing:
🔎 Suggested fix
it('respects width and height on grid container', async () => { const grid = document.createElement('div'); grid.style.display = 'grid'; - grid.style.width = '400px'; - grid.style.height = '300px'; + grid.style.width = '320px'; + grid.style.height = '240px'; grid.style.gridTemplateColumns = '1fr 1fr'; grid.style.gridTemplateRows = '1fr 1fr'; grid.style.gap = '10px'; grid.style.backgroundColor = '#e3f2fd'; // ... - expect(grid.getBoundingClientRect().width).toBe(400); - expect(grid.getBoundingClientRect().height).toBe(300); + expect(grid.getBoundingClientRect().width).toBe(320); + expect(grid.getBoundingClientRect().height).toBe(240); // fr units should distribute remaining space const items = Array.from(grid.children) as HTMLElement[]; - expect(items[0].getBoundingClientRect().width).toBe(195); // (400 - 10) / 2 + expect(items[0].getBoundingClientRect().width).toBe(155); // (320 - 10) / 2integration_tests/scripts/babel_transform_snapshot.js-31-32 (1)
31-32: Unreachable code: redundant length check.The condition at line 32 (
if (args.length > filenameArgIndex)) is unreachable. Line 21 already returns early ifargs.length !== 0 && args.length !== filenameArgIndex, so by the time execution reaches line 32,args.lengthis guaranteed to be either0or exactlyfilenameArgIndex—never greater.🔎 Suggested fix
const snapshotFilepath = filepath.relative( opts.workspacePath, filepath.join( opts.snapshotPath, filepath.relative(opts.testPath, filename), ) ); - // If the call already has a filename argument, do nothing. - if (args.length > filenameArgIndex) return; - // Fill missing positional args with `undefined` so `snapshotFilepath` lands // on the intended slot.integration_tests/specs/css/css-grid/animation/track-size-transition.ts-192-229 (1)
192-229: Fr unit test also exceeds max window width.Same issue as the unit transition test -
width: '400px'(line 195) exceeds the 340px limit. The fr calculation math is correct (400 - 150 - 10 = 240), but consider adjusting for the window constraint.Based on learnings: The max width of testing window is 340px.
integration_tests/specs/css/css-grid/animation/track-size-transition.ts-80-115 (1)
80-115: Grid width exceeds testing window maximum.The grid has
width: '400px'(line 83) which exceeds the 340px max testing window width per coding guidelines. This may cause unexpected layout behavior or snapshot differences. Consider reducing to 300px or less.🔎 Suggested fix
- grid.style.width = '400px'; - grid.style.gridTemplateColumns = '100px 200px'; + grid.style.width = '300px'; + grid.style.gridTemplateColumns = '75px 150px'; ... // Change to percentage - grid.style.gridTemplateColumns = '25% 50%'; + grid.style.gridTemplateColumns = '25% 50%'; ... - expect(items[0].getBoundingClientRect().width).toBe(100); // 25% of 400 - expect(items[1].getBoundingClientRect().width).toBe(200); // 50% of 400 + expect(items[0].getBoundingClientRect().width).toBe(75); // 25% of 300 + expect(items[1].getBoundingClientRect().width).toBe(150); // 50% of 300Based on learnings: The max width of testing window is 340px.
integration_tests/specs/css/css-grid/absolute-positioning/offset-properties.ts-219-259 (1)
219-259: Width exceeds testing window constraint.Line 227 sets
grid.style.width = '400px', which exceeds the documented maximum testing window width of 340px. This may cause rendering or test issues.🔎 Proposed fix to comply with width constraint
- grid.style.width = '400px'; + grid.style.width = '340px'; grid.style.height = '300px';Also update the related assertions:
- // Right: 15% of 400px = 60px from right edge - expect(absItem.getBoundingClientRect().right).toBe(grid.getBoundingClientRect().right - 60); + // Right: 15% of 340px = 51px from right edge + expect(absItem.getBoundingClientRect().right).toBe(grid.getBoundingClientRect().right - 51);Based on learnings, the max width for the testing window is 340px.
bridge/scripts/code_generator/src/json/make_dart_blink_css_ids.ts-59-61 (1)
59-61: Add error handling for file I/O and JSON parsing operations.The code reads from the filesystem and parses JSON5 without error handling. If
css_value_keywords.json5is missing or malformed, the generator will throw an unhandled exception with an unclear error message. Wrap the file read and JSON5 parsing operations in a try-catch block to provide clear feedback to developers during the build process.integration_tests/specs/css/css-grid/absolute-positioning/mixed-positioning.ts-322-365 (1)
322-365: Test name doesn't match the test content.The test is named "handles fixed positioning within grid" but actually tests
position: sticky. Consider renaming to "handles sticky positioning within grid" for accuracy.🔎 Suggested fix
- xit('handles fixed positioning within grid', async () => { + xit('handles sticky positioning within grid', async () => {bridge/core/css/style_engine.cc-1328-1331 (1)
1328-1331: Unreachable code: checking for non-variable IDs inside variable-only block.Lines 1329-1331 are inside an
if (id == CSSPropertyID::kVariable)block, soidcannot bekWhiteSpaceCollapseorkTextWrap. This check is dead code.Suggested fix
if (value_string.IsEmpty()) { value_string = String(" "); } - - // Skip white-space longhands; will emit shorthand later - if (id == CSSPropertyID::kWhiteSpaceCollapse || id == CSSPropertyID::kTextWrap) { - continue; - } - // Already cleared above. auto key_ns = prop_name.ToStylePropertyNameNativeString();
🧹 Nitpick comments (52)
bridge/core/css/blink_inline_style_validation_test.cc (2)
36-48: Minor edge case: Leading uppercase would produce a leading hyphen.If a property name starts with an uppercase letter (e.g.,
"WebkitTransform"), this would produce"-webkit-transform"with a leading hyphen. For typical camelCase properties likefontSizeorbackgroundColor, this works correctly. If vendor-prefixed properties are tested, consider handling the leading hyphen case.🔎 Optional fix if vendor prefixes are needed
std::string ConvertCamelCaseToKebabCase(const std::string& property_name) { std::string result; result.reserve(property_name.size() + 8); - for (unsigned char c : property_name) { + for (size_t i = 0; i < property_name.size(); ++i) { + unsigned char c = property_name[i]; if (std::isupper(c)) { - result.push_back('-'); + if (i > 0) { + result.push_back('-'); + } result.push_back(static_cast<char>(std::tolower(c))); } else { result.push_back(static_cast<char>(c)); } } return result; }
50-90: Code duplication betweenHasSetStyleWithKeyValueandHasSetStyleByIdWithKeyValue.The value decoding logic (lines 78-84) is duplicated in
HasSetStyleByIdWithKeyValue(lines 105-111). Consider extracting a helper function for maintainability.🔎 Optional helper extraction
std::string DecodeStyleValue(const UICommandItem& item) { if (item.string_01 < 0) { return getValueName(static_cast<CSSValueID>(-item.string_01 - 1)); } auto* value_ptr = reinterpret_cast<SharedNativeString*>(static_cast<uintptr_t>(item.string_01)); return SharedNativeStringToUTF8(value_ptr); }Then use
DecodeStyleValue(item)in both functions.integration_tests/specs/css/css-grid/grid-definition/explicit-tracks.ts (1)
245-255: Remove commented-out debug code.The commented-out console.log statements and alternative snapshot call appear to be debugging artifacts left over from development. Commented-out code reduces readability and should be removed before merging.
🔎 Proposed cleanup
document.body.appendChild(grid); await waitForFrame(); - // Debug: ensure line names + placement parse correctly. - // eslint-disable-next-line no-console - // console.log('[grid][duplicate-line-names] templateCols:', getComputedStyle(grid).gridTemplateColumns); - // // eslint-disable-next-line no-console - // console.log('[grid][duplicate-line-names] item gridColumnStart/end:', getComputedStyle(item).gridColumnStart, getComputedStyle(item).gridColumnEnd); - // // eslint-disable-next-line no-console - // console.log('[grid][duplicate-line-names] item style gridColumn:', item.style.gridColumn); - // // eslint-disable-next-line no-console - // console.log('[grid][duplicate-line-names] item style gridColumnStart/end:', item.style.gridColumnStart, item.style.gridColumnEnd); await snapshot(); - // await snapshot(1); const gridRect = grid.getBoundingClientRect();integration_tests/specs/css/css-grid/grid-definition/repeat-notation.ts (1)
59-69: Consider removing commented debugging code.The commented-out console.log statements and snapshot call appear to be debugging artifacts from investigating the named grid lines behavior. Since these debugging traces are no longer active, consider removing them to keep the test clean and maintainable.
If these debug logs might be useful for future troubleshooting, document their purpose or consider a more structured debugging approach.
🔎 Proposed cleanup
document.body.appendChild(grid); await waitForFrame(); - // Debug: ensure line names in repeat + placement parse correctly. - // eslint-disable-next-line no-console - // console.log('[grid][named-lines-repeat] templateCols:', getComputedStyle(grid).gridTemplateColumns); - // // eslint-disable-next-line no-console - // console.log('[grid][named-lines-repeat] item gridColumnStart/end:', getComputedStyle(item).gridColumnStart, getComputedStyle(item).gridColumnEnd); - // // eslint-disable-next-line no-console - // console.log('[grid][named-lines-repeat] item style gridColumn:', item.style.gridColumn); - // // eslint-disable-next-line no-console - // console.log('[grid][named-lines-repeat] item style gridColumnStart/end:', item.style.gridColumnStart, item.style.gridColumnEnd); await snapshot(); - // await snapshot(1); const gridRect = grid.getBoundingClientRect();integration_tests/specs/blink/css1/box_properties/padding_left.ts (1)
154-156: Add cleanup for BODY style modification to ensure test independence.The font-family style set on BODY persists after the test completes, potentially affecting subsequent tests. Additionally, this test is the only one in the file that modifies BODY.style, creating an inconsistency.
Based on coding guidelines, tests should maintain independence and perform cleanup of modifications.
🔎 Proposed fix to add cleanup
it('padding-left 25px on ul with list items', async () => { + const originalFontFamily = BODY.style.fontFamily; const p = createElement('p', {}, createText('The style declarations which apply to the text below are:')); const pre = createElement('pre', {}, createText('.two {padding-left: 25px; background-color: aqua;}')); const hr = createElement('hr', {}, []); const style = createElement('style', {}, [ createText('.two {padding-left: 25px; background-color: aqua;}') ]); const li1 = createElement('li', {}, [ createText('The left padding on this unordered list has been set to 25 pixels, which will require some extra test in order to test.') ]); const li2 = createElementWithStyle('li', { paddingLeft: '25px', backgroundColor: 'white' }, [ createText('Another list item might not be such a bad idea, either, considering that such things do need to be double-checked. This list item has its left padding also set to 25 pixels, which should combine with the list\'s padding to make 50 pixels of margin.') ]); const ul = createElementWithStyle('ul', { paddingLeft: '25px', backgroundColor: 'gray' }, [ li1, li2 ]); BODY.style.fontFamily = 'Times'; append(BODY, style); append(BODY, p); append(BODY, pre); append(BODY, hr); append(BODY, ul); await snapshot(); + + // Cleanup + BODY.style.fontFamily = originalFontFamily; });Alternatively, if Times font is required for snapshot consistency across all tests, consider moving it to a
beforeEachhook and resetting inafterEachat the describe block level.native_plugins/README.md (1)
1-80: Consider adding migration guidance for removed deeplink module.The PR removes the
webf_deeplinkmodule documentation. If this represents a breaking change for existing users, consider adding a migration guide or deprecation notice that directs users to any replacement functionality or alternative approaches.bridge/core/html/parser/html_parser.cc (3)
234-236: Misleading comment: comments are now handled.The comment states "Ignore other node types (comments, etc.)" but comments are explicitly handled in lines 210-221 above. Consider updating to reflect actual behavior.
🔎 Suggested fix
} - // Ignore other node types (comments, etc.) + // Ignore other node types (e.g., document nodes) }
302-410: Significant code duplication withtraverseHTMLloop.The
append_gumbo_childlambda duplicates most of the node-handling logic fromtraverseHTML(lines 138-234). Both implement nearly identical handling for ELEMENT, TEXT, WHITESPACE, COMMENT, and CDATA nodes. This increases maintenance burden and risk of divergent behavior if only one path is updated.Consider extracting the common node-processing logic into a shared helper function that both
traverseHTMLandtraverse_fragment_rootcan invoke.
287-297: Remove or enable the commented-out error logging.This commented-out block appears to be debug/verbose logging for parsing errors. If useful for debugging, consider enabling it behind a flag or at
VERBOSElog level. Otherwise, remove the dead code to reduce clutter.integration_tests/specs/css/css-grid/edge-cases/empty-grids.ts (1)
76-85: Batch DOM operations using DocumentFragment.The loop appends items one-by-one to a grid already in the DOM. To minimize potential reflows and align with coding guidelines, consider using a DocumentFragment to batch the operations.
🔎 Proposed refactor using DocumentFragment
- // Add items - for (let i = 0; i < 4; i++) { - const item = document.createElement('div'); - item.textContent = `${i + 1}`; - item.style.backgroundColor = `hsl(${40 + i * 15}, 70%, 65%)`; - item.style.display = 'flex'; - item.style.alignItems = 'center'; - item.style.justifyContent = 'center'; - item.style.color = 'white'; - grid.appendChild(item); - } + // Add items + const fragment = document.createDocumentFragment(); + for (let i = 0; i < 4; i++) { + const item = document.createElement('div'); + item.textContent = `${i + 1}`; + item.style.backgroundColor = `hsl(${40 + i * 15}, 70%, 65%)`; + item.style.display = 'flex'; + item.style.alignItems = 'center'; + item.style.justifyContent = 'center'; + item.style.color = 'white'; + fragment.appendChild(item); + } + grid.appendChild(fragment);Based on learnings, batch DOM operations to minimize reflows.
integration_tests/specs/css/css-grid/grid-model/grid-items-types.ts (2)
1-179: Consider extracting common grid setup logic.All six tests follow a similar pattern: create grid container, configure grid properties, populate items, append to DOM, snapshot, validate, and cleanup. While the current structure is clear and each test is self-contained, extracting a helper function for common grid setup could reduce duplication and improve maintainability.
Example helper function
function createTestGrid(config: { columns: string; rows: string; gap?: string; backgroundColor?: string; padding?: string; }): HTMLDivElement { const grid = document.createElement('div'); grid.style.display = 'grid'; grid.style.gridTemplateColumns = config.columns; grid.style.gridTemplateRows = config.rows; if (config.gap) grid.style.gap = config.gap; if (config.backgroundColor) grid.style.backgroundColor = config.backgroundColor; if (config.padding) grid.style.padding = config.padding; return grid; }This refactor is optional—the current explicit approach aids test readability and independence.
1-179: Consider adding edge case tests for grid items.The current test coverage validates common element types as grid items. To strengthen the test suite, consider adding tests for edge cases such as:
- Elements with
display: none(should not participate in grid layout)- Absolutely positioned grid items (different layout behavior)
- Nested grids as grid items
- Other replaced elements (video, canvas, iframe)
- Empty text nodes vs. whitespace-only text nodes
integration_tests/specs/css/css-grid/layout-algorithm/circular-dependencies.ts (1)
82-91: Consider batching DOM append operations (optional).The coding guidelines suggest batching DOM operations to minimize reflows. Currently, elements are appended to the grid one at a time in the loop, and then the grid is appended to the body.
While the current approach is clear and readable (which is important for tests), you could defer the
document.body.appendChild(grid)until after all items are added to the grid to reduce reflows. However, the performance impact in test context is minimal, so this is an optional optimization.Based on coding guidelines for integration tests.
integration_tests/specs/rendering/widget_cupertino_context_menu_popup_preview_width.ts (3)
8-8: Consider removing theas anytype cast.The type cast bypasses TypeScript's type checking. If
backgroundColoris a valid CSS property (which it is), this should work without casting. If there's a specific reason for the cast, consider adding a comment explaining why it's necessary.🔎 Suggested fix
- (document.body.style as any).backgroundColor = '#ffffff'; + document.body.style.backgroundColor = '#ffffff';
95-101: Consider logging suppressed errors during cleanup.The empty catch blocks on lines 95-97 and 98-101 suppress all errors during cleanup, which could hide important failures or resource leaks. Consider at least logging these errors to help with debugging flaky tests.
🔎 Suggested improvement
try { await dismissFlutterOverlays(); - } catch (_) {} + } catch (e) { + console.warn('Failed to dismiss Flutter overlays during cleanup:', e); + } try { const root = document.getElementById('root'); root?.remove(); - } catch (_) {} + } catch (e) { + console.warn('Failed to remove root element during cleanup:', e); + }
76-87: Hard-coded sleep values are consistent with codebase patterns and reasonably used here.The
sleep(0.8)calls are standard throughout the WebF test suite (used in multiple rendering and dom tests with values ranging 0.1–1.0 seconds). The test already combines this with frame-based waiting vianextFrames(8)on line 87, anddismissFlutterOverlays()is already in place for cleanup. No exposed Flutter API exists to await overlay animation completion, making the current approach reasonable given available testing utilities.cli/src/peerDeps.ts (2)
4-6: Consider adding error handling for malformed JSON.
readJsonFilewill throw if the JSON is malformed. Since this utility is used in peer dependency checks, a parsing failure could produce an unclear error message.🔎 Suggested improvement
export function readJsonFile(jsonPath: string): any { - return JSON.parse(fs.readFileSync(jsonPath, 'utf-8')); + try { + return JSON.parse(fs.readFileSync(jsonPath, 'utf-8')); + } catch (error) { + throw new Error(`Failed to parse JSON at ${jsonPath}: ${error instanceof Error ? error.message : error}`); + } }
17-20: Clarify the default-true behavior when no types field exists.When
package.jsonhas notypesfield,isPackageTypesReadyreturnstrue. This is intentional (the package doesn't declare types, so there's nothing to wait for), but a brief inline comment would help future maintainers understand this design choice.🔎 Suggested improvement
export function isPackageTypesReady(pkgDir: string): boolean { const typesFile = getPackageTypesFileFromDir(pkgDir); + // If no types field is declared, consider the package ready (no types to wait for) return typesFile ? fs.existsSync(typesFile) : true; }cli/test/peerDeps.test.ts (1)
6-8: Potential issue with temp directory creation path.The
makeTempDirfunction creates temp directories underprocess.cwd()/test/, which assumes thetestdirectory exists. If Jest runs from the CLI root and notestfolder exists there,mkdtempSyncwill fail.Consider using
os.tmpdir()for a more robust location:🔎 Suggested improvement
+import os from 'os'; + function makeTempDir(prefix: string): string { - return fs.mkdtempSync(path.join(process.cwd(), 'test', prefix)); + return fs.mkdtempSync(path.join(os.tmpdir(), prefix)); }cli/src/generator.ts (1)
435-442: Consider simplifying the typesContent array construction.The current logic uses
hasAny ? value : ''which produces empty strings that are then filtered. This works but is slightly convoluted.🔎 Suggested simplification
- const typesContent = [ - '/* Generated by WebF CLI - aggregated type declarations */', - hasAny ? typeAliasDecl : '', - hasAny ? constDecl : '', - hasAny ? enumDecl : '', - hasAny ? '' : 'export {};', - '' - ].filter(Boolean).join('\n'); + const typesContentParts = ['/* Generated by WebF CLI - aggregated type declarations */']; + if (hasAny) { + if (typeAliasDecl) typesContentParts.push(typeAliasDecl); + if (constDecl) typesContentParts.push(constDecl); + if (enumDecl) typesContentParts.push(enumDecl); + } else { + typesContentParts.push('export {};'); + } + typesContentParts.push(''); + const typesContent = typesContentParts.join('\n');cli/src/commands.ts (2)
1180-1199: Consider extracting helper functions to module scope.The functions
getInstalledPackageJsonPath,getInstalledPackageDir, andfindUpare defined insidebuildPackagebut don't capture any local state. Moving them to module scope would improve readability and enable reuse.
1190-1199: Potential infinite loop if path.dirname returns unexpected value.The
findUpfunction relies onparent === dirto detect the root. This is standard, but consider adding a depth limit as a safety measure.🔎 Optional safety enhancement
- function findUp(startDir: string, relativePathToFind: string): string | null { + function findUp(startDir: string, relativePathToFind: string, maxDepth: number = 20): string | null { let dir = path.resolve(startDir); - while (true) { + let depth = 0; + while (depth < maxDepth) { const candidate = path.join(dir, relativePathToFind); if (fs.existsSync(candidate)) return candidate; const parent = path.dirname(dir); if (parent === dir) return null; dir = parent; + depth++; } + return null; }.github/workflows/build_checks.yml (1)
15-127: Add explicit permissions blocks to jobs.Following the principle of least privilege, each job should declare its required permissions explicitly rather than relying on workflow-level defaults.
🔎 Proposed addition
Add permissions blocks to each job:
build-android-bridge: name: Build Android Bridge runs-on: [self-hosted, macos] + permissions: + contents: read steps:build-ios-example: name: Build iOS Example runs-on: [self-hosted, macos] + permissions: + contents: read steps:Based on static analysis hints.
.github/workflows/publish_npm_packages.yml (1)
101-102: Consider whether global npm upgrade is necessary.Installing
npm@latestglobally adds build time and potential instability. Theactions/setup-node@v4action typically provides a recent npm version that supports OIDC trusted publishing (available since npm 9.x).Alternative approach
You can either:
- Remove this step if the npm version from setup-node is sufficient (verify it's ≥9.0.0):
- - name: Upgrade npm (for OIDC Trusted Publishing) - run: npm i -g npm@latest
- Or pin to a specific minimum version for reproducibility:
- name: Upgrade npm (for OIDC Trusted Publishing) - run: npm i -g npm@latest + run: npm i -g npm@10integration_tests/specs/css/css-grid/interactions/grid-in-flex.ts (1)
2-50: Consider adding width constraint to properly test flex-shrink behavior.The test comment states that the grid should not be flex-shrunk due to its automatic minimum size. However, the flex container has no explicit width constraint, so there may be sufficient space for both items without any shrinking pressure. This means the test might pass even if flex-shrink logic is broken.
To properly test the intended behavior, consider adding a width constraint to the flex container that forces a space conflict:
flex.style.width = '300px'; // Forces shrinking pressureThis ensures the test validates that the grid maintains its minimum content size under actual shrinking conditions.
Based on learnings, tests should focus on testing behavior rather than implementation, and this adjustment would strengthen the behavioral validation.
integration_tests/HOW_TO_TEST.md (1)
113-113: Consider rewording to improve readability.The static analysis tool correctly identified that three consecutive sentences begin with "If", which can make the text harder to read.
🔎 Suggested rewording
-- If snapshots aren't being generated, check that your test is actually running (look for PASS in the output). -- If tests are failing, verify that the feature is working as expected and that your test logic is correct. -- If the same test generates different snapshots on different runs, the feature might have non-deterministic behavior that needs to be fixed. +- If snapshots aren't being generated, check that your test is actually running (look for PASS in the output). +- When tests fail, verify that the feature is working as expected and that your test logic is correct. +- Non-deterministic snapshots across multiple runs may indicate that the feature has non-deterministic behavior that needs to be fixed.bridge/core/css/css_rule.d.ts (1)
1-5: Standard CSSOM properties correctly added.The
typeandcssTextreadonly properties are standard CSSRule interface members from the CSSOM specification. The types are correct:
type: Numeric constant identifying the rule typecssText: String representation of the ruleOptional: Consider using
numberinstead ofdoublefor TypeScript idiom.While
doubleis valid TypeScript,numberis the more common and idiomatic type in TypeScript:🔎 Optional improvement
- readonly type: double; + readonly type: number; readonly cssText: string;bridge/test/css_unittests.cmake (1)
109-109: Remove the commented-out duplicate line.Line 80 already contains the active
gtest_discover_tests(webf_css_unittests)call, so this commented duplicate is unnecessary and could cause confusion.🔎 Suggested cleanup
-# Enable test discovery for CTest -#include(GoogleTest) -#gtest_discover_tests(webf_css_unittests)integration_tests/specs/css/css-grid/parsing/computed-values-extended.ts (2)
2-23: Consider validating actual computed pixel values.The test title promises verification of computed values for mixed units (px, %, fr), but the assertion only checks that
gridTemplateColumnsis truthy. To properly validate the sizing resolution algorithm, consider asserting that the computed value contains concrete pixel measurements for all tracks.Example stronger assertion
const computed = getComputedStyle(grid); -// Should compute percentages and fr units to pixel values -expect(computed.gridTemplateColumns).toBeTruthy(); +// Should compute percentages and fr units to pixel values +const tracks = computed.gridTemplateColumns.split(' '); +expect(tracks.length).toBe(4); +expect(tracks[0]).toBe('100px'); // fixed px value +expect(parseFloat(tracks[1])).toBeGreaterThan(0); // resolved % value +expect(parseFloat(tracks[2])).toBeGreaterThan(0); // resolved fr value +expect(parseFloat(tracks[3])).toBeGreaterThan(0); // resolved fr value
25-48: Weak assertion for auto track sizing.Similar to the previous test, the assertion only verifies that
gridTemplateRowsis truthy rather than validating thatautotracks are resolved to concrete pixel heights based on content.integration_tests/specs/css/css-grid/parsing/invalid-values.ts (4)
2-25: Consider verifying the actual fallback value.The test confirms that the invalid value is rejected but doesn't verify what value the browser falls back to. For completeness, consider asserting that
displayremains'grid'(the previous valid value) rather than just checking it's not the invalid string.Enhanced assertion
const computed = getComputedStyle(grid); -expect(computed.display).not.toBe('invalid-grid'); +expect(computed.display).not.toBe('invalid-grid'); +expect(computed.display).toBe('grid'); // Should retain previous valid value
40-50: Unused variableinitialComputedshould be used for validation.The test captures the initial computed value but never uses it. To properly verify that invalid values are ignored and the previous valid value is retained, compare the final computed value against
initialComputed.Use the captured value
const computed = getComputedStyle(grid); -// Should keep previous valid value or use initial -expect(computed.gridTemplateColumns).toBeTruthy(); +// Should keep previous valid value +expect(computed.gridTemplateColumns).toBe(initialComputed);
54-81: Capture and verify the original gap value is preserved.Similar to the
grid-template-columnstest, this should capture the initial valid gap value and verify it's unchanged after attempting to set an invalid value, rather than just checking truthiness.Verify value preservation
document.body.appendChild(grid); await waitForFrame(); + +const initialGap = getComputedStyle(grid).rowGap; // Try to set invalid gap grid.style.gap = 'invalid'; await waitForFrame(); await snapshot(); const computed = getComputedStyle(grid); -// Should have some valid gap value -expect(computed.rowGap).toBeTruthy(); +// Should retain previous valid gap value +expect(computed.rowGap).toBe(initialGap);
138-161: Weak assertion for invalid minmax() syntax.The test only checks that
gridTemplateColumnsis truthy. Consider verifying either:
- That the browser preserves the previous valid
minmax(100px, 200px) 150pxvalue, or- That the computed value differs from the invalid declaration.
According to CSS spec,
minmax(min, max)where min > max should be treated as invalid and the declaration ignored.Verify fallback behavior
document.body.appendChild(grid); await waitForFrame(); + +const initialColumns = getComputedStyle(grid).gridTemplateColumns; // Try to set invalid minmax (min > max) grid.style.gridTemplateColumns = 'minmax(300px, 100px) 150px'; await waitForFrame(); await snapshot(); const computed = getComputedStyle(grid); -// Should handle invalid minmax gracefully -expect(computed.gridTemplateColumns).toBeTruthy(); +// Should ignore invalid minmax and retain previous value +expect(computed.gridTemplateColumns).toBe(initialColumns);integration_tests/specs/cssom/document-styleSheets.ts (1)
64-106: Sleep delays may cause flakiness in CI environments.The test uses
sleep(0.02)(20ms) at lines 86 and 96 to wait for style recalculation. WhilewaitForFrame()is used for DOM mutations, these additional sleeps suggest uncertainty about when computed styles are ready. Consider whetherwaitForFrame()alone is sufficient, or if a more deterministic signal (like a style recalc completion event) could replace arbitrary delays.Based on learnings: Tests should use async/await and proper async patterns for asynchronous operations in tests.
integration_tests/specs/css/css-grid/layout-algorithm/sizing-resolution.ts (1)
76-112: Weak assertion for spanning item sizing.The test verifies that a spanning item has non-zero width but doesn't validate the actual sizing relationship. Consider asserting that the spanning item's width approximately equals the sum of the two tracks it spans plus the gap between them.
Stronger assertion
+const items = Array.from(grid.children).slice(1) as HTMLElement[]; // Regular items +const track1Width = items[0].getBoundingClientRect().width; +const track2Width = items[1].getBoundingClientRect().width; +const gap = 10; // From line 81 +const expectedSpanWidth = track1Width + track2Width + gap; + // Spanning item should distribute space across tracks -expect(spanItem.getBoundingClientRect().width).toBeGreaterThan(0); +expect(Math.abs(spanItem.getBoundingClientRect().width - expectedSpanWidth)).toBeLessThan(1);integration_tests/specs/css/css-grid/grid-model/writing-modes.ts (1)
1-191: Test suite structure looks good, but assertions are weak for skipped tests.The tests follow the coding guidelines well: proper cleanup with
grid.remove(), use ofsnapshot()for visual regression, async/await patterns, and clear test names. However, most skipped tests (lines 34-160) only verifyitems.length, which doesn't validate the actual writing-mode or direction behavior.When these tests are enabled, consider adding layout assertions similar to the first test (line 29) that verifies RTL positioning:
// Example for vertical-lr: verify items flow vertically const items = Array.from(grid.children) as HTMLElement[]; expect(items[0].getBoundingClientRect().left).toBeLessThan(items[2].getBoundingClientRect().left);The skipped tests (
xit) are appropriate if the features are not yet implemented. As per coding guidelines, tests should "Test behavior, not implementation."CSS_GRID_PLAN.md (1)
210-225: Add blank line before table for proper markdown rendering.Per markdownlint MD058, tables should be surrounded by blank lines for consistent rendering across markdown parsers.
🔎 Suggested fix
#### Updated WPT Test Distribution + | Category | Test Count | Percentage | WebF Coverage | |----------|-----------|------------|---------------|integration_tests/specs/css/css-grid/edge-cases/overlarge-grids.ts (1)
85-115: Good coverage of auto-fill with minmax.The test validates the auto-placement algorithm with 100 items. Consider adding an assertion for the actual number of columns created based on container width to strengthen the test.
bridge/core/dom/element.cc (1)
1086-1125: Document the value_slot encoding scheme.The encoding where negative
value_slotrepresents an identifier ID (-static_cast<int64_t>(ident.GetValueID()) - 1) and positive values represent a pointer to a native string is clever but non-obvious. Consider adding a comment explaining this dual-purpose encoding.Additionally, casting a pointer to
int64_ton line 1119 assumes pointer size ≤ 64 bits. While this is true on all current platforms, astatic_assertor comment would clarify the assumption.🔎 Suggested documentation
+ // value_slot encoding: + // - Negative values: encoded identifier ID as -(id + 1), decoded on Dart side + // - Positive values: pointer to SharedNativeString (freed by Dart) int64_t value_slot = 0; if ((*value_ptr)->IsIdentifierValue()) { const auto& ident = To<CSSIdentifierValue>(*(*value_ptr)); + // Encode identifier ID as negative to distinguish from string pointers value_slot = -static_cast<int64_t>(ident.GetValueID()) - 1; } else { String value_string = inline_style->GetPropertyValueWithHint(prop_name, i); if (value_string.IsNull()) { value_string = (*value_ptr)->CssTextForSerialization(); } if (!value_string.IsEmpty()) { auto* value_ns = stringToNativeString(value_string).release(); + // Pointer fits in int64_t on all supported platforms (32/64-bit) value_slot = static_cast<int64_t>(reinterpret_cast<intptr_t>(value_ns)); } }bridge/foundation/shared_ui_command.h (1)
9-9: Fix typo in header guard.The header guard has a typo: "DOUBULE" should be "DOUBLE". While this appears to be pre-existing (not marked with ~), it's worth correcting for consistency.
🔎 Suggested fix
-#ifndef MULTI_THREADING_DOUBULE_UI_COMMAND_H_ -#define MULTI_THREADING_DOUBULE_UI_COMMAND_H_ +#ifndef MULTI_THREADING_DOUBLE_UI_COMMAND_H_ +#define MULTI_THREADING_DOUBLE_UI_COMMAND_H_And at the end of the file:
-#endif // MULTI_THREADING_DOUBULE_UI_COMMAND_H_ +#endif // MULTI_THREADING_DOUBLE_UI_COMMAND_H_integration_tests/specs/css/css-grid/absolute-positioning/z-index-stacking.ts (1)
13-46: Consider batching DOM operations.While the current approach is clear and readable, you could minimize reflows by batching appendChild operations using a DocumentFragment. However, given this is test code where clarity is paramount and performance is less critical, the current implementation is acceptable.
🔎 Example of batched approach (if performance becomes a concern)
+const fragment = document.createDocumentFragment(); + // Lower z-index (behind) const abs1 = document.createElement('div'); // ... style abs1 ... -grid.appendChild(abs1); +fragment.appendChild(abs1); // Higher z-index (in front) const abs2 = document.createElement('div'); // ... style abs2 ... -grid.appendChild(abs2); +fragment.appendChild(abs2); +grid.appendChild(fragment); document.body.appendChild(grid);Based on learnings, batch DOM operations to minimize reflows.
integration_tests/lib/webf_tester.dart (1)
75-96: Dynamic cast reduces type safety.The
(module as dynamic).setCallback(...)pattern bypasses compile-time type checking. Consider defining an interface or abstract class for modules that support callbacks.That said, for test harness code this pragmatic approach is acceptable if
MethodChannelCallbackModuleis only used in tests.integration_tests/specs/css/css-grid/absolute-positioning/grid-area-containing-block.ts (1)
150-205: Remove debug console.log statements before enabling this test.The skipped test (
xit) contains severalconsole.logdebug statements (lines 187, 189, 195-196, 199, 202) that should be removed before the test is enabled. These appear to be leftover debugging aids.🔎 Suggested cleanup when enabling this test
// Item spans 2x2: 100 + 10 + 100 = 210px width, 70 + 10 + 70 = 150px height expect(item.getBoundingClientRect().width).toBe(210); - console.log('-----------------------------0') expect(item.getBoundingClientRect().height).toBe(150); - console.log('-----------------------------1') // Absolute child centered in item const itemCenterX = item.getBoundingClientRect().left + 105; const itemCenterY = item.getBoundingClientRect().top + 75; const childCenterX = absChild.getBoundingClientRect().left + 40; const childCenterY = absChild.getBoundingClientRect().top + 30; - console.log("itemCenterX:", itemCenterX, "itemCenterY:", itemCenterY); - console.log("childCenterX:", childCenterX, "childCenterY:", childCenterY); expect(Math.round(childCenterX)).toBe(Math.round(itemCenterX)); - console.log('-----------------------------2') expect(Math.round(childCenterY)).toBe(Math.round(itemCenterY)); - console.log('-----------------------------3') grid.remove();bridge/core/css/css_rule_list.h (1)
29-29: Consider forward declaration instead of full include.The header only uses
CSSRule*return type. A forward declaration (class CSSRule;) is already present on line 33, so this#includemay not be necessary. However, if downstream code relies onCSSRulebeing fully defined when including this header, it's acceptable.integration_tests/specs/css/css-grid/absolute-positioning/mixed-positioning.ts (1)
54-105: Skipped test (xit) – consider adding a TODO comment explaining why.The test is skipped but the reason isn't documented. Adding a brief comment would help future maintainers understand whether this is a known limitation, pending feature, or flaky test.
bridge/core/js_function_ref.h (1)
33-35: Consider adding a comment about thread affinity.The comment on line 33 says "Only accessed on the JS thread" but applies to both
ctxandfunction. Consider extending the comment to make this clearer.🔎 Suggested clarification
- // Only accessed on the JS thread that owns the context. + // Only accessed on the JS thread that owns the context; must not be read/written from Dart thread. JSContext* ctx{nullptr}; JSValue function{JS_NULL};bridge/core/dart_binding_object.h (1)
24-25: Consider using an enum for method type return values.The magic numbers 0/1/2 are documented in a comment, but an enum would be more self-documenting and type-safe.
🔎 Suggested improvement
// In a suitable header or within the class: enum class BindingMethodType : int { kNone = 0, kSync = 1, kAsync = 2 }; // Then update the method signature: BindingMethodType GetBindingMethodType(const AtomicString& method, ExceptionState& exception_state) const;bridge/webf_bridge.cc (1)
498-503: Consider documenting memory ownership forCopyToCStr.The function allocates memory via
dart_mallocwhich transfers ownership to the caller (Dart side). As per coding guidelines, memory ownership should be documented clearly in FFI implementations.Suggested documentation
+// Allocates a null-terminated C string using dart_malloc. +// Ownership is transferred to the caller (typically Dart FFI layer). static char* CopyToCStr(const std::string& s) {bridge/core/css/style_engine.cc (2)
1166-1171: Display none detection via string comparison may be fragile.The detection relies on string comparison with
StripWhiteSpace().LowerASCII() == "none". Consider using the identifier value check instead for robustness:Suggested improvement
- bool display_none_for_invalidation = false; - if (property_set && !property_set->IsEmpty()) { - String display_value = property_set->GetPropertyValue(CSSPropertyID::kDisplay); - display_none_for_invalidation = display_value.StripWhiteSpace().LowerASCII() == "none"; - } + bool display_none_for_invalidation = false; + if (property_set && !property_set->IsEmpty()) { + const auto* display_value = property_set->GetPropertyCSSValue(CSSPropertyID::kDisplay); + if (display_value && display_value->IsIdentifierValue()) { + display_none_for_invalidation = + To<CSSIdentifierValue>(*display_value).GetValueID() == CSSValueID::kNone; + } + }
1352-1373: The value_slot encoding scheme should be documented.The encoding uses negative values for CSS identifier IDs (
-valueID - 1) and positive values for string pointers. This is a valid discriminated union pattern, but the convention should be documented for maintainability.Suggested documentation
+ // value_slot encoding: + // - Negative: CSS identifier value ID encoded as (-valueID - 1) + // - Zero/Positive: Pointer to SharedNativeString with property value int64_t value_slot = 0; SharedNativeString* base_href = nullptr; if ((*value_ptr)->IsIdentifierValue()) { const auto& ident = To<CSSIdentifierValue>(*(*value_ptr)); value_slot = -static_cast<int64_t>(ident.GetValueID()) - 1;bridge/core/css/invalidation/pending_invalidations_test.cc (1)
129-256: Consider extracting or simplifying the debug trace lambda.The
build_debug_tracelambda (lines 129-256) is extensive and provides valuable debugging information. However, it adds significant code complexity to the test. Consider:
- Extracting it to a test utility function for reuse
- Only enabling it conditionally (e.g., via a preprocessor flag)
| - name: Setup Android SDK and NDK | ||
| uses: nttld/setup-ndk@v1 | ||
| id: setup-ndk | ||
| with: | ||
| ndk-version: r27d | ||
| env: | ||
| ANDROID_NDK_HOME: ${{ steps.setup-ndk.outputs.ndk-path }} | ||
|
|
There was a problem hiding this comment.
Critical: Step output referenced in its own environment block.
Line 38 attempts to use steps.setup-ndk.outputs.ndk-path within the env block of the setup-ndk step itself. Step outputs are only available to subsequent steps, not within the step's own execution context. This will result in an empty or undefined value for ANDROID_NDK_HOME during NDK setup.
🔎 Proposed fix
Remove the premature env block and use the NDK path in subsequent steps:
- name: Setup Android SDK and NDK
uses: nttld/setup-ndk@v1
id: setup-ndk
with:
ndk-version: r27d
- env:
- ANDROID_NDK_HOME: ${{ steps.setup-ndk.outputs.ndk-path }}The environment variables are already correctly set in the "Build Android bridge" step at lines 62-65.
🧰 Tools
🪛 actionlint (1.7.9)
38-38: property "setup-ndk" is not defined in object type {}
(expression)
🤖 Prompt for AI Agents
.github/workflows/build_checks.yml lines 32-39: the setup-ndk step is
referencing its own output in its env block (ANDROID_NDK_HOME: ${{
steps.setup-ndk.outputs.ndk-path }}), which is invalid because step outputs are
only available to subsequent steps; remove the env entry from the setup-ndk step
and instead consume the ndk-path output in a following step (or in jobs.env) by
adding ANDROID_NDK_HOME: ${{ steps.setup-ndk.outputs.ndk-path }} to the env of
the next step that needs it (e.g., the "Build Android bridge" step), ensuring
the NDK path is read after setup completes.
| - name: Publish to pub.dev | ||
| if: ${{ !inputs.dry_run }} | ||
| uses: k-paxian/dart-package-publisher@master | ||
| with: | ||
| credentialJson: ${{ secrets.PUB_DEV_TOKEN }} | ||
| flutter: true | ||
| force: ${{ inputs.force }} | ||
| skipTests: ${{ inputs.skip_tests }} | ||
| relativePath: ${{ matrix.relativePath }} |
There was a problem hiding this comment.
Pin third-party action to specific version or commit SHA.
Using @master for the k-paxian/dart-package-publisher action introduces supply chain risk, as the master branch could be updated with malicious or breaking changes at any time.
🔎 Recommended fix
Pin to a specific version tag or commit SHA:
- name: Publish to pub.dev
if: ${{ !inputs.dry_run }}
- uses: k-paxian/dart-package-publisher@master
+ uses: k-paxian/[email protected]
with:
credentialJson: ${{ secrets.PUB_DEV_TOKEN }}
flutter: true
force: ${{ inputs.force }}
skipTests: ${{ inputs.skip_tests }}
relativePath: ${{ matrix.relativePath }}Check the releases page for the latest stable version.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Publish to pub.dev | |
| if: ${{ !inputs.dry_run }} | |
| uses: k-paxian/dart-package-publisher@master | |
| with: | |
| credentialJson: ${{ secrets.PUB_DEV_TOKEN }} | |
| flutter: true | |
| force: ${{ inputs.force }} | |
| skipTests: ${{ inputs.skip_tests }} | |
| relativePath: ${{ matrix.relativePath }} | |
| - name: Publish to pub.dev | |
| if: ${{ !inputs.dry_run }} | |
| uses: k-paxian/dart-package-publisher@v1.6.0 | |
| with: | |
| credentialJson: ${{ secrets.PUB_DEV_TOKEN }} | |
| flutter: true | |
| force: ${{ inputs.force }} | |
| skipTests: ${{ inputs.skip_tests }} | |
| relativePath: ${{ matrix.relativePath }} |
🤖 Prompt for AI Agents
.github/workflows/publish_webf_native_plugins.yml lines 117-125: the workflow
currently references the third-party action using @master which is a
supply-chain risk; update the uses entry to pin the action to a specific
released version tag or a commit SHA (e.g., a stable tag from the action's
releases page or the commit SHA) instead of @master, and verify the tag/SHA is
the intended stable release before committing.
| std::vector<NativeValue> native_args; | ||
| native_args.reserve(argc); | ||
| native_args.emplace_back(NativeValueConverter<NativeTypeString>::ToNativeValue(ctx, class_name)); | ||
|
|
||
| for (int i = 1; i < argc; i++) { | ||
| native_args.emplace_back(ScriptValue(ctx, argv[i]).ToNative(ctx, exception_state)); | ||
| if (UNLIKELY(exception_state.HasException())) { | ||
| return exception_state.ToQuickJS(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the NativeValue definition and destructor
find bridge -name "*.h" -o -name "*.hpp" | xargs grep -l "class NativeValue" | head -5Repository: openwebf/webf
Length of output: 85
🏁 Script executed:
# Search for NativeValue class definition
rg "class NativeValue\b" bridge/ -A 15Repository: openwebf/webf
Length of output: 1029
🏁 Script executed:
# Look for NativeValueConverter implementation
rg "NativeValueConverter" bridge/foundation/ -lRepository: openwebf/webf
Length of output: 99
🏁 Script executed:
# Get the full NativeValue class definition
rg "class NativeValue" bridge/include/webf_bridge.h -A 30Repository: openwebf/webf
Length of output: 1282
🏁 Script executed:
# Check native_value_converter.h for the converter implementation
cat bridge/foundation/native_value_converter.h | head -100Repository: openwebf/webf
Length of output: 3508
🏁 Script executed:
# Search specifically for NativeTypeString in the converter
rg "NativeTypeString" bridge/foundation/native_value_converter.h -B 2 -A 8Repository: openwebf/webf
Length of output: 548
🏁 Script executed:
# Look for NativeValue destructor and member cleanup
rg "~NativeValue|NativeValue::" bridge/ -A 5Repository: openwebf/webf
Length of output: 34624
🏁 Script executed:
# Find the actual NativeValue definition
rg "^struct NativeValue|^class NativeValue" bridge/foundation/ -A 20Repository: openwebf/webf
Length of output: 17500
🏁 Script executed:
# Find native_value.h header
find bridge -name "native_value.h"Repository: openwebf/webf
Length of output: 89
🏁 Script executed:
# Search for Native_NewString function
rg "Native_NewString|Native_NewCString" bridge/foundation/ -B 2 -A 5Repository: openwebf/webf
Length of output: 2978
🏁 Script executed:
# Check the actual file under review to see full context around lines 64-73
cat bridge/bindings/qjs/qjs_dart_binding_object.cc | sed -n '50,85p'Repository: openwebf/webf
Length of output: 1484
🏁 Script executed:
# Check what happens with native_args after the loop - does it get passed to callback?
cat bridge/bindings/qjs/qjs_dart_binding_object.cc | sed -n '60,90p'Repository: openwebf/webf
Length of output: 1389
🏁 Script executed:
# Look for how NativeValue cleanup is typically handled elsewhere
rg "NativeValue.*exception|exception.*NativeValue" bridge/bindings/qjs/ -B 3 -A 3 | head -50Repository: openwebf/webf
Length of output: 1585
🏁 Script executed:
# Check if there are any RAII wrappers for NativeValue cleanup
rg "class.*NativeValue|struct.*NativeValue" bridge/foundation/ -A 10 | grep -E "destructor|~|cleanup|free"Repository: openwebf/webf
Length of output: 39
🏁 Script executed:
# Check AutoFreeNativeString to understand memory ownership
rg "AutoFreeNativeString" bridge/foundation/ -B 2 -A 15 | head -60Repository: openwebf/webf
Length of output: 4745
🏁 Script executed:
# Check SharedNativeString definition and memory management
rg "class SharedNativeString|struct SharedNativeString" bridge/foundation/ -A 20 | head -80Repository: openwebf/webf
Length of output: 2996
🏁 Script executed:
# Search for how native_args is used after exception cases in similar code
rg "native_args|NativeValue.*vector" bridge/bindings/qjs/ -B 2 -A 5 | head -100Repository: openwebf/webf
Length of output: 2011
🏁 Script executed:
# Check if there's any automatic cleanup of NativeValue allocations
rg "free.*NativeString|delete.*NativeString|SharedNativeString.*new|new.*SharedNativeString" bridge/foundation/ -B 2 -A 2 | head -50Repository: openwebf/webf
Length of output: 1094
🏁 Script executed:
# Verify NativeValue is POD with no destructor
cat bridge/foundation/native_value.hRepository: openwebf/webf
Length of output: 2099
🏁 Script executed:
# Check if createBindingObject takes ownership of native_args
rg "createBindingObject" bridge/bindings/ -A 10 | head -60Repository: openwebf/webf
Length of output: 1108
🏁 Script executed:
# Check if there's any cleanup code in exception paths elsewhere
rg "exception_state.HasException|UNLIKELY.*exception" bridge/bindings/qjs/qjs_dart_binding_object.cc -B 3 -A 5Repository: openwebf/webf
Length of output: 655
🏁 Script executed:
# Search for how createBindingObject uses the native_args
rg "createBindingObject" bridge/ -A 15 | head -100Repository: openwebf/webf
Length of output: 9936
🏁 Script executed:
# Check AutoFreeNativeString full definition
rg "struct AutoFreeNativeString|class AutoFreeNativeString" bridge/foundation/ -A 25Repository: openwebf/webf
Length of output: 482
🏁 Script executed:
# Check the destructor of AutoFreeNativeString
rg "AutoFreeNativeString::" bridge/foundation/ -A 10Repository: openwebf/webf
Length of output: 633
🏁 Script executed:
# Verify memory leak by checking if NativeValue stores raw pointers and vector cleanup
rg "std::vector.*NativeValue" bridge/ -B 3 -A 8 | head -80Repository: openwebf/webf
Length of output: 4542
Memory leak in exception paths for NativeValue string conversions.
When ScriptValue::ToNative() fails during argument conversion (lines 70-72), the early return skips the callback to createBindingObject, which would normally transfer ownership to Dart for cleanup. However, entries already added to native_args—particularly those from NativeValueConverter<NativeTypeString>::ToNativeValue()—contain heap-allocated SharedNativeString* pointers that are never freed. Since std::vector<NativeValue> only destroys POD values on scope exit, these string allocations leak.
Add explicit cleanup of the vector entries on the exception path before returning, or wrap string pointers in RAII holders.
| JSValue data[1]; | ||
| data[0] = JS_AtomToString(ctx, atom); | ||
| // The function takes ownership of `data`. | ||
| JSValue fn = JS_NewCFunctionData(ctx, InvokeMethodCallback, 0, method_type, 1, data); | ||
| JS_FreeValue(ctx, data[0]); | ||
| return fn; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the actual code in the file
cat -n bridge/core/dart_binding_object.cc | sed -n '150,170p'Repository: openwebf/webf
Length of output: 914
🏁 Script executed:
# Search for JS_NewCFunctionData usage patterns in the codebase
rg -n "JS_NewCFunctionData" --type cpp -B 3 -A 5Repository: openwebf/webf
Length of output: 3535
🏁 Script executed:
# Check if there's any QuickJS header documentation or comments about data ownership
rg -n "JS_FreeValue.*data" --type cpp -A 2 -B 2Repository: openwebf/webf
Length of output: 6281
🏁 Script executed:
# Search for any comments or documentation about JS_NewCFunctionData in the codebase
rg -n "JS_NewCFunctionData|CFunctionData" --type hRepository: openwebf/webf
Length of output: 39
Data freed before function can access it: use-after-free bug.
data[0] is freed at line 160 immediately after passing it to JS_NewCFunctionData, but the returned function fn holds a reference to this data. When the function later tries to access its data array, it will access freed memory. The JS_NewCFunctionData API stores references to the data array elements, not copies; the caller must keep the data alive for the function's lifetime. Either remove the JS_FreeValue(ctx, data[0]) call, or defer it until after the function is no longer needed (note: data[0] is stack-allocated but its JSValue content requires cleanup).
🤖 Prompt for AI Agents
In bridge/core/dart_binding_object.cc around lines 156-161, the code frees
data[0] immediately after calling JS_NewCFunctionData causing a use-after-free
because the created function retains a reference; remove the JS_FreeValue(ctx,
data[0]) call so the JSValue remains alive for the function's lifetime, and
ensure the value is released later when the function is destroyed (e.g., free
the stored JSValue in the function's finalizer/cleanup path).
| // Track JS function handles that were passed across the bridge (JS -> Dart). | ||
| // Must be called on the owning JS thread. | ||
| void RegisterJSFunctionRef(NativeJSFunctionRef* ref); | ||
| void UnregisterJSFunctionRef(NativeJSFunctionRef* ref); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Document memory ownership of NativeJSFunctionRef.
The new RegisterJSFunctionRef/UnregisterJSFunctionRef APIs manage function references across the JS↔Dart bridge. Please clarify in comments:
- Who owns the
NativeJSFunctionRef*lifetime (caller or ExecutingContext) - When/how refs should be unregistered
- Thread-safety guarantees (already noted "Must be called on the owning JS thread")
As per coding guidelines, "Document memory ownership clearly in FFI implementations" for bridge code.
🤖 Prompt for AI Agents
In bridge/core/executing_context.h around lines 167-170, the
RegisterJSFunctionRef/UnregisterJSFunctionRef declarations lack documentation of
memory ownership and lifecycle expectations; update the comment block above
these methods to state that the caller retains ownership of the
NativeJSFunctionRef* (ExecutingContext stores only a non-owning/raw pointer),
specify that callers must call UnregisterJSFunctionRef from the same owning JS
thread before destroying the referenced NativeJSFunctionRef (or provide a clear
transfer-of-ownership statement if ownership is transferred), and reiterate the
thread-safety contract (must be invoked on the owning JS thread) along with any
guarantees about when ExecutingContext will drop its internal references (e.g.,
on explicit unregister or on context teardown), so users know who frees memory
and when to unregister.
| const properties = new ClassObject(); | ||
| properties.name = 'TestComponentProperties'; | ||
| properties.kind = ClassObjectKind.interface; | ||
| const eo = new EnumObject(); | ||
| eo.name = 'CupertinoColors'; | ||
| const m1 = new EnumMemberObject(); m1.name = "'red'"; m1.initializer = '0x0f'; | ||
| const m2 = new EnumMemberObject(); m2.name = "'bbb'"; m2.initializer = '0x00'; | ||
| eo.members = [m1, m2]; | ||
| blob.objects = [eo as any]; | ||
| blob.objects = [properties, eo as any]; |
There was a problem hiding this comment.
Code duplication and same issues as first test.
-
Code Duplication: The properties object creation (lines 23-25) is identical to lines 8-10. Consider extracting this to a helper function or
beforeEachblock to follow DRY principles. -
Type Safety & Incomplete Assertions: This test has the same issues as the first test regarding the
as anycast and missing assertions for the properties object's impact.
🔎 Proposed refactor to eliminate duplication
describe('React generator - declare const support', () => {
+ function createPropertiesObject(): ClassObject {
+ const properties = new ClassObject();
+ properties.name = 'TestComponentProperties';
+ properties.kind = ClassObjectKind.interface;
+ return properties;
+ }
+
it('emits export declare const for constants from typings', () => {
const blob = new IDLBlob('/test/source', '/test/target', 'ConstOnly', 'test', '');
- const properties = new ClassObject();
- properties.name = 'TestComponentProperties';
- properties.kind = ClassObjectKind.interface;
+ const properties = createPropertiesObject();
const constObj = new ConstObject();
constObj.name = 'WEBF_CUPERTINO_SYMBOL';
constObj.type = 'unique symbol';
blob.objects = [properties, constObj as any];
const output = generateReactComponent(blob);
expect(output).toContain('export declare const WEBF_CUPERTINO_SYMBOL: unique symbol;');
});
it('emits export declare enum for enums from typings', () => {
const blob = new IDLBlob('/test/source', '/test/target', 'EnumOnly', 'test', '');
- const properties = new ClassObject();
- properties.name = 'TestComponentProperties';
- properties.kind = ClassObjectKind.interface;
+ const properties = createPropertiesObject();
const eo = new EnumObject();
eo.name = 'CupertinoColors';
const m1 = new EnumMemberObject(); m1.name = "'red'"; m1.initializer = '0x0f';
const m2 = new EnumMemberObject(); m2.name = "'bbb'"; m2.initializer = '0x00';
eo.members = [m1, m2];
blob.objects = [properties, eo as any];
const output = generateReactComponent(blob);
expect(output).toContain("export enum CupertinoColors { 'red' = 0x0f, 'bbb' = 0x00 }");
});
});Committable suggestion skipped: line range outside the PR's diff.
| double contextId = controller.view.contextId; | ||
| Pointer<Void> testContext = initTestFramework(contextId); | ||
| registerDartTestMethodsToCpp(contextId); | ||
| registerDartTestMethodsToCpp(testContext); | ||
| // Timer(Duration(seconds: 1), () { | ||
| // executeTest(testContext, contextId); | ||
| // }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for initTestFramework implementation
echo "=== Searching for initTestFramework implementation ==="
rg -n 'initTestFramework' --type dart --type cc --type cpp -A 8 | head -50
# Search for registerDartTestMethodsToCpp implementation
echo -e "\n=== Searching for registerDartTestMethodsToCpp implementation ==="
rg -n 'registerDartTestMethodsToCpp' --type dart --type cc --type cpp -A 8 | head -50
# Check the full context of playground.dart around the specified lines
echo -e "\n=== Full context of playground.dart ==="
wc -l integration_tests/lib/playground.dartRepository: openwebf/webf
Length of output: 332
🏁 Script executed:
#!/bin/bash
# Get more context around the target lines
echo "=== Context around lines 100-130 in playground.dart ==="
sed -n '100,130p' integration_tests/lib/playground.dart
# Search for any cleanup of testContext in the file
echo -e "\n=== Search for testContext usage and cleanup in playground.dart ==="
rg -n 'testContext' integration_tests/lib/playground.dart -B 2 -A 2Repository: openwebf/webf
Length of output: 1674
🏁 Script executed:
#!/bin/bash
# Find FFI-related files that might contain initTestFramework and registerDartTestMethodsToCpp
echo "=== Find files with initTestFramework ==="
fd -t f -e dart -e cc -e cpp -e h | xargs rg -l 'initTestFramework' 2>/dev/null
echo -e "\n=== Find files with registerDartTestMethodsToCpp ==="
fd -t f -e dart -e cc -e cpp -e h | xargs rg -l 'registerDartTestMethodsToCpp' 2>/dev/nullRepository: openwebf/webf
Length of output: 488
🏁 Script executed:
#!/bin/bash
# Read to_native.dart for initTestFramework definition
echo "=== initTestFramework in to_native.dart ==="
rg -n 'initTestFramework' integration_tests/lib/bridge/to_native.dart -B 3 -A 10
# Read from_native.dart for registerDartTestMethodsToCpp definition
echo -e "\n=== registerDartTestMethodsToCpp in from_native.dart ==="
rg -n 'registerDartTestMethodsToCpp' integration_tests/lib/bridge/from_native.dart -B 3 -A 10
# Check webf_tester.dart for context
echo -e "\n=== registerDartTestMethodsToCpp in webf_tester.dart ==="
rg -n 'registerDartTestMethodsToCpp' integration_tests/lib/webf_tester.dart -B 2 -A 5Repository: openwebf/webf
Length of output: 2276
🏁 Script executed:
#!/bin/bash
# Read the header file to understand the C++ side
echo "=== webf_bridge_test.h ==="
cat bridge/include/webf_bridge_test.h
# Read the C++ test implementation
echo -e "\n=== webf_bridge_test.cc (first 100 lines) ==="
head -100 bridge/webf_bridge_test.ccRepository: openwebf/webf
Length of output: 3735
🏁 Script executed:
#!/bin/bash
# Check if there's any cleanup or free of testContext in webf_tester.dart
echo "=== Full webf_tester.dart for context ==="
wc -l integration_tests/lib/webf_tester.dart
# Check from_native.dart for cleanup patterns
echo -e "\n=== Check from_native.dart size ==="
wc -l integration_tests/lib/bridge/from_native.dart
# Check to_native.dart size
echo -e "\n=== Check to_native.dart size ==="
wc -l integration_tests/lib/bridge/to_native.dartRepository: openwebf/webf
Length of output: 310
🏁 Script executed:
#!/bin/bash
# Read full webf_tester.dart to see complete context and cleanup
echo "=== Full webf_tester.dart content ==="
cat integration_tests/lib/webf_tester.dart | head -100
echo -e "\n=== Search for testContext cleanup in webf_tester.dart ==="
rg -n 'testContext' integration_tests/lib/webf_tester.dart -B 2 -A 2Repository: openwebf/webf
Length of output: 4596
🏁 Script executed:
#!/bin/bash
# Check if there's a delete/cleanup function for testContext in the bridge
echo "=== Search for cleanup/delete functions for test context ==="
rg -n 'delete.*TestContext|free.*TestContext|cleanup.*Test' bridge/ --type cc --type h
# Check webf_test_context.h to understand the lifecycle
echo -e "\n=== Check webf_test_context.h ==="
find bridge -name "webf_test_context.h" -o -name "*test_context.h" | head -5
# Look for destructor or cleanup in the test context
echo -e "\n=== Search for WebFTestContext destructor or cleanup ==="
rg -n '~WebFTestContext|destroyTestContext|deleteTestContext' bridge/ --type cc --type hRepository: openwebf/webf
Length of output: 313
🏁 Script executed:
#!/bin/bash
# Check the complete executeTest function in webf_bridge_test.cc
echo "=== Complete executeTest function ==="
rg -n 'void executeTest' bridge/webf_bridge_test.cc -A 20
# Search for cleanup of testContext after executeTest completes
echo -e "\n=== Search for cleanup after executeTest ==="
rg -n 'delete|free' bridge/webf_bridge_test.cc | grep -i 'test'Repository: openwebf/webf
Length of output: 1090
🏁 Script executed:
#!/bin/bash
# Read the webf_test_context.h header
echo "=== webf_test_context.h ==="
cat bridge/test/webf_test_context.h
# Search for destructor implementation
echo -e "\n=== Search for WebFTestContext implementation ==="
find bridge -name "*.cc" | xargs rg -l 'WebFTestContext' | head -5Repository: openwebf/webf
Length of output: 1547
🏁 Script executed:
#!/bin/bash
# Look for any cleanup/delete code for testContext across the codebase
echo "=== Search for testContext deletion/cleanup ==="
rg -n 'delete.*testContext|free.*testContext|testContext.*delete' --type dart
# Check if there's any cleanup in playground.dart's dispose or similar
echo -e "\n=== Full playground.dart to check for cleanup ==="
cat integration_tests/lib/playground.dart
# Look for destructor in WebFTestContext
echo -e "\n=== Search for ~WebFTestContext ==="
find bridge/test -type f \( -name "*.cc" -o -name "*.h" \) -exec rg -l '~WebFTestContext' {} \;Repository: openwebf/webf
Length of output: 4174
🏁 Script executed:
#!/bin/bash
# Check the executeTest wrapper in to_native.dart to see the full chain
echo "=== Full to_native.dart content ==="
cat integration_tests/lib/bridge/to_native.dartRepository: openwebf/webf
Length of output: 3157
🏁 Script executed:
#!/bin/bash
# Check for any cleanup/delete function for testContext in bridge
echo "=== Search for functions that delete or cleanup testContext ==="
rg -n 'delete|cleanup|destroy' bridge/webf_bridge_test.cc | head -20
# Read the WebFTestContext destructor implementation
echo -e "\n=== webf_test_context.cc (destructor) ==="
cat bridge/test/webf_test_context.cc | head -60
# Check if there's a deleteTestContext or similar function exported
echo -e "\n=== Check for exported test cleanup functions ==="
rg -n 'WEBF_EXPORT' bridge/include/webf_bridge_test.h
# Search for any memory cleanup patterns in test-related code
echo -e "\n=== Search for persistent handle cleanup in test code ==="
rg -n 'Dart_DeletePersistentHandle' bridge/webf_bridge_test.ccRepository: openwebf/webf
Length of output: 2737
🏁 Script executed:
#!/bin/bash
# Check if the persistent handle created in executeTest is cleaned up
echo "=== Full executeTest and related code in webf_bridge_test.cc ==="
rg -n 'void executeTest' bridge/webf_bridge_test.cc -A 30
# Look for Dart_DeletePersistentHandle_DL usage in the codebase
echo -e "\n=== Search for Dart_DeletePersistentHandle_DL usage ==="
rg -n 'Dart_DeletePersistentHandle_DL' bridge/
# Check the invokeExecuteTest method that's called from the async callback
echo -e "\n=== Search for invokeExecuteTest implementation ==="
rg -n 'invokeExecuteTest' bridge/ -A 20 | head -50Repository: openwebf/webf
Length of output: 5806
🏁 Script executed:
#!/bin/bash
# Check if there's any cleanup when WebFTestContext is destroyed
echo "=== Search for WebFTestContext destructor full implementation ==="
rg -n '~WebFTestContext' bridge/test/webf_test_context.cc -A 10
# Look for all member functions of WebFTestContext
echo -e "\n=== Full webf_test_context.cc to understand lifecycle ==="
wc -l bridge/test/webf_test_context.ccRepository: openwebf/webf
Length of output: 716
🏁 Script executed:
#!/bin/bash
# Check the complete invokeExecuteTest implementation to see the full cleanup
echo "=== Complete invokeExecuteTest implementation ==="
rg -n 'void WebFTestContext::invokeExecuteTest' bridge/test/webf_test_context.cc -A 50 | head -70
# Look for any exported cleanup function for testContext
echo -e "\n=== Check webf_bridge_test.h for all exported functions ==="
cat bridge/include/webf_bridge_test.h
# Check if there's a deleteTestContext or cleanup function anywhere
echo -e "\n=== Search for any cleanup/delete function in test bridge ==="
rg -n 'delete|cleanup|destroy|free.*Test' bridge/webf_bridge_test.ccRepository: openwebf/webf
Length of output: 3409
🏁 Script executed:
#!/bin/bash
# Read the complete destructor
echo "=== Full WebFTestContext destructor ==="
sed -n '409,446p' bridge/test/webf_test_context.cc
# Check if there's a delete statement for testContext anywhere in playground.dart or webf_tester.dart
echo -e "\n=== Check for any malloc.free or similar in integration tests ==="
rg -n 'malloc\.free|delete' integration_tests/lib/ --type dartRepository: openwebf/webf
Length of output: 2018
🏁 Script executed:
#!/bin/bash
# Check if testContext is stored anywhere for cleanup in playground.dart
echo "=== Check if testContext is stored as member variable in playground ==="
cat integration_tests/lib/playground.dart | grep -n 'Pointer<Void>'
# Check the full lifecycle - is there any cleanup when controller is disposed?
echo -e "\n=== Search for controller.dispose() patterns ==="
rg -n 'dispose\|cleanup' integration_tests/lib/playground.dart
# Look for any deleteTestContext or cleanup function that might exist
echo -e "\n=== Final check: any delete/cleanup function for testContext ==="
rg -n 'deleteTestContext|cleanupTestContext|freeTestContext|deleteTest' bridge/ --type cpp --type hRepository: openwebf/webf
Length of output: 328
Missing cleanup for testContext pointer causes memory leak.
The testContext pointer obtained from initTestFramework() is never freed. In playground.dart, it becomes a local variable that goes out of scope, leaking the heap-allocated WebFTestContext C++ object. In webf_tester.dart, it's stored as a member variable but never cleaned up.
No cleanup function is exported from the bridge to delete this context. Add a cleanup function (e.g., deleteTestContext()) to the C++ bridge and call it when the context is no longer needed, or ensure the Dart-side cleanup calls the appropriate C++ destructor. This should be done in a finally block or dispose method to guarantee cleanup on exceptions and controller disposal.
Learnings indicate: Always free allocated memory in Dart FFI and ensure cleanup in finally blocks to prevent leaks.
🤖 Prompt for AI Agents
In integration_tests/lib/playground.dart around lines 114 to 119 the
Pointer<Void> testContext returned by initTestFramework(contextId) is never
freed which leaks the heap-allocated WebFTestContext; add a C++ bridge function
(e.g., deleteTestContext(Pointer<Void>)) that calls the native destructor,
expose it to Dart, and call that function from Dart when the context is no
longer needed — specifically update webf_tester.dart to store the testContext as
a member and invoke deleteTestContext inside a finally block or the controller’s
dispose method to guarantee cleanup on exceptions and disposal.
| declare function createElement(tagName: string, props?: any, children?: any[]): HTMLElement; | ||
| declare function createElementWithStyle(tagName: string, style?: any, children?: any[]): HTMLElement; | ||
| declare const BODY: HTMLBodyElement; | ||
| declare function snapshotFlutter(x?: number, y?: number, w?: number, h?: number, filename?: String, postfix?: boolean | string): Promise<void>; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use lowercase string instead of String for the type annotation.
In TypeScript, type annotations should use the primitive type string (lowercase) rather than the String object wrapper type. The primitive type is the recommended practice for type annotations.
🔎 Proposed fix
-declare function snapshotFlutter(x?: number, y?: number, w?: number, h?: number, filename?: String, postfix?: boolean | string): Promise<void>;
+declare function snapshotFlutter(x?: number, y?: number, w?: number, h?: number, filename?: string, postfix?: boolean | string): Promise<void>;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| declare function snapshotFlutter(x?: number, y?: number, w?: number, h?: number, filename?: String, postfix?: boolean | string): Promise<void>; | |
| declare function snapshotFlutter(x?: number, y?: number, w?: number, h?: number, filename?: string, postfix?: boolean | string): Promise<void>; |
🤖 Prompt for AI Agents
In integration_tests/runtime/kraken.d.ts around line 94, the function signature
uses the boxed String type for filename; change the type annotation to the
primitive lowercase string (i.e., replace String with string) so the declaration
uses the correct TypeScript primitive type.
| it('handles flex-grow on grid item', async () => { | ||
| const flex = document.createElement('div'); | ||
| flex.style.display = 'flex'; | ||
| flex.style.width = '600px'; | ||
| flex.style.gap = '10px'; | ||
| flex.style.backgroundColor = '#e3f2fd'; | ||
| flex.style.padding = '15px'; | ||
|
|
||
| const grid = document.createElement('div'); | ||
| grid.style.display = 'grid'; | ||
| grid.style.gridTemplateColumns = 'repeat(2, 1fr)'; | ||
| grid.style.gridTemplateRows = 'repeat(2, 60px)'; | ||
| grid.style.gap = '8px'; | ||
| grid.style.flexGrow = '1'; | ||
| grid.style.backgroundColor = '#2196F3'; | ||
| grid.style.padding = '10px'; | ||
|
|
||
| for (let i = 0; i < 4; i++) { | ||
| const item = document.createElement('div'); | ||
| item.textContent = `${i + 1}`; | ||
| item.style.backgroundColor = '#90CAF9'; | ||
| item.style.display = 'flex'; | ||
| item.style.alignItems = 'center'; | ||
| item.style.justifyContent = 'center'; | ||
| item.style.color = 'white'; | ||
| grid.appendChild(item); | ||
| } | ||
|
|
||
| flex.appendChild(grid); | ||
|
|
||
| const sidebar = document.createElement('div'); | ||
| sidebar.textContent = 'Sidebar'; | ||
| sidebar.style.width = '150px'; | ||
| sidebar.style.backgroundColor = '#1976D2'; | ||
| sidebar.style.padding = '20px'; | ||
| sidebar.style.color = 'white'; | ||
| flex.appendChild(sidebar); | ||
|
|
||
| document.body.appendChild(flex); | ||
| await waitForFrame(); | ||
| await snapshot(); | ||
|
|
||
| // Grid should grow to fill available space | ||
| expect(grid.getBoundingClientRect().width).toBeGreaterThan(300); | ||
|
|
||
| flex.remove(); | ||
| }); |
There was a problem hiding this comment.
Flex container width exceeds viewport constraint.
The flex container is set to 600px width, but the coding guidelines specify that "The max width of testing window is 340px." This exceeds the viewport constraint and could cause:
- Horizontal overflow and clipping
- Inconsistent snapshot rendering
- Test failures or unreliable results
🔎 Proposed fix to respect viewport constraints
- flex.style.width = '600px';
+ flex.style.width = '320px';Adjust the grid template and sidebar dimensions proportionally if needed:
grid.style.gridTemplateColumns = 'repeat(2, 1fr)';The 1fr tracks will still demonstrate flex-grow behavior within the smaller container.
As per coding guidelines, the test window has a maximum width of 340px.
🤖 Prompt for AI Agents
In integration_tests/specs/css/css-grid/interactions/grid-in-flex.ts around
lines 52 to 98, the test sets the flex container width to 600px which exceeds
the test viewport max width (340px); change flex.style.width to a value <=
"340px" (e.g., "340px" or "320px"), proportionally reduce sidebar.width (e.g.,
from 150px to ~90px) and/or reduce padding/gaps so the grid still has room to
grow, and update the assertion threshold (e.g.,
expect(grid.getBoundingClientRect().width).toBeGreaterThan(150) or another value
matching the new layout) so the test respects the viewport constraint and
remains deterministic.
| it('handles multiple grids in flex row', async () => { | ||
| const flex = document.createElement('div'); | ||
| flex.style.display = 'flex'; | ||
| flex.style.gap = '15px'; | ||
| flex.style.backgroundColor = '#e8f5e9'; | ||
| flex.style.padding = '15px'; | ||
|
|
||
| for (let g = 0; g < 3; g++) { | ||
| const grid = document.createElement('div'); | ||
| grid.style.display = 'grid'; | ||
| grid.style.gridTemplateColumns = 'repeat(2, 70px)'; | ||
| grid.style.gridTemplateRows = 'repeat(2, 60px)'; | ||
| grid.style.gap = '6px'; | ||
| grid.style.backgroundColor = `hsl(${120 + g * 15}, 60%, 55%)`; | ||
| grid.style.padding = '10px'; | ||
|
|
||
| for (let i = 0; i < 4; i++) { | ||
| const item = document.createElement('div'); | ||
| item.textContent = `${g + 1}.${i + 1}`; | ||
| item.style.backgroundColor = `hsl(${120 + g * 15}, 40%, 85%)`; | ||
| item.style.display = 'flex'; | ||
| item.style.alignItems = 'center'; | ||
| item.style.justifyContent = 'center'; | ||
| item.style.color = '#333'; | ||
| item.style.fontSize = '10px'; | ||
| grid.appendChild(item); | ||
| } | ||
|
|
||
| flex.appendChild(grid); | ||
| } | ||
|
|
||
| document.body.appendChild(flex); | ||
| await waitForFrame(); | ||
| await snapshot(); | ||
|
|
||
| const grids = Array.from(flex.children) as HTMLElement[]; | ||
| expect(grids.length).toBe(3); | ||
|
|
||
| flex.remove(); | ||
| }); |
There was a problem hiding this comment.
Multiple grids exceed viewport width constraint.
This test creates 3 grids, each requiring approximately 166px width (2 columns of 70px + gap + padding), plus flex gaps and container padding. The total width exceeds 550px, which significantly exceeds the 340px viewport constraint specified in the coding guidelines.
🔎 Proposed fix to fit within viewport
Reduce the number of grids or their dimensions:
- for (let g = 0; g < 3; g++) {
+ for (let g = 0; g < 2; g++) {
const grid = document.createElement('div');
grid.style.display = 'grid';
- grid.style.gridTemplateColumns = 'repeat(2, 70px)';
+ grid.style.gridTemplateColumns = 'repeat(2, 60px)';
grid.style.gridTemplateRows = 'repeat(2, 60px)';This would reduce total width to approximately 280px, fitting within the viewport while still demonstrating multiple grids in a flex row.
As per coding guidelines, the test window has a maximum width of 340px.
🤖 Prompt for AI Agents
integration_tests/specs/css/css-grid/interactions/grid-in-flex.ts lines 188-227:
The three grids as sized (2×70px columns + gaps + padding, plus flex gaps and
container padding) exceed the test viewport max width (340px); adjust the test
so the total width fits the viewport by either reducing the number of grids to 2
or shrinking grid dimensions (e.g., use 50px columns, smaller grid gap and
padding, and/or smaller flex gap) so the combined width stays under 340px;
update any related expectations or snapshots after resizing.
andycall
force-pushed
the
feat/css_grid_integration_tests_3
branch
from
3f4084b to
8e3caf1
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
integration_tests/specs/css/css-grid/interactions/grid-in-flex.ts (2)
52-98: Flex container width exceeds viewport constraint.The flex container is set to 600px width, but the coding guidelines specify that "The max width of testing window is 340px." This exceeds the viewport constraint and could cause horizontal overflow, clipping, and unreliable snapshot results.
🔎 Proposed fix to respect viewport constraints
- flex.style.width = '600px'; + flex.style.width = '320px'; grid.style.gap = '10px';Adjust the sidebar width proportionally:
- sidebar.style.width = '150px'; + sidebar.style.width = '90px';And update the assertion threshold:
- expect(grid.getBoundingClientRect().width).toBeGreaterThan(300); + expect(grid.getBoundingClientRect().width).toBeGreaterThan(150);As per coding guidelines, the test window has a maximum width of 340px.
188-227: Multiple grids exceed viewport width constraint.This test creates 3 grids, each approximately 166px wide (2×70px columns + gap + padding), plus flex gaps and container padding. The total width significantly exceeds the 340px viewport constraint specified in the coding guidelines, which could cause horizontal overflow and unreliable snapshots.
🔎 Proposed fix to fit within viewport
Reduce the number of grids or their dimensions:
- for (let g = 0; g < 3; g++) { + for (let g = 0; g < 2; g++) { const grid = document.createElement('div'); grid.style.display = 'grid'; - grid.style.gridTemplateColumns = 'repeat(2, 70px)'; + grid.style.gridTemplateColumns = 'repeat(2, 60px)'; grid.style.gridTemplateRows = 'repeat(2, 60px)';This reduces the total width to approximately 280px, fitting within the viewport while still demonstrating multiple grids in a flex row.
As per coding guidelines, the test window has a maximum width of 340px.
🧹 Nitpick comments (4)
integration_tests/specs/css/css-grid/absolute-positioning/mixed-positioning.ts (1)
12-21: Consider batching DOM operations to minimize reflows.The loop appends each item individually, triggering multiple reflows. For better performance, consider using a document fragment:
const fragment = document.createDocumentFragment(); for (let i = 0; i < 4; i++) { const item = document.createElement('div'); // ... set properties fragment.appendChild(item); } grid.appendChild(fragment);This pattern applies to similar loops throughout the file (lines 81-90, 177-186, 227-236, 331-340).
As per coding guidelines: batch DOM operations to minimize reflows.
integration_tests/specs/css/css-grid/absolute-positioning/basic-positioning.ts (1)
59-68: Consider batching DOM operations.Similar to other tests in this PR, the loop appends items one by one. Consider using a document fragment to batch operations and minimize reflows.
As per coding guidelines: batch DOM operations to minimize reflows.
integration_tests/specs/css/css-grid/edge-cases/overlarge-grids.ts (1)
12-22: Consider batching DOM operations.Multiple loops throughout this file append items individually. Consider using document fragments to batch operations and reduce reflows.
As per coding guidelines: batch DOM operations to minimize reflows.
Also applies to: 54-64, 95-105, 128-138, 176-186, 213-223
integration_tests/specs/css/css-grid/edge-cases/extreme-values.ts (1)
10-14: Consider batching DOM operations.Multiple loops append items individually. Using document fragments would reduce reflows and improve performance.
As per coding guidelines: batch DOM operations to minimize reflows.
Also applies to: 40-50, 72-81, 102-111, 135-144, 168-179
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (147)
integration_tests/snapshots/css/css-grid/absolute-positioning/basic-positioning.ts.28bcad9f1.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/absolute-positioning/basic-positioning.ts.9b76adbb1.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/absolute-positioning/basic-positioning.ts.a0ad58961.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/absolute-positioning/basic-positioning.ts.a20394661.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/absolute-positioning/basic-positioning.ts.db4bca831.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/absolute-positioning/basic-positioning.ts.e96c05291.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/absolute-positioning/grid-area-containing-block.ts.2791ecbc1.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/absolute-positioning/grid-area-containing-block.ts.41ad10e31.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/absolute-positioning/grid-area-containing-block.ts.4f9d0c5e1.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/absolute-positioning/grid-area-containing-block.ts.7d4653a31.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/absolute-positioning/grid-area-containing-block.ts.c68891721.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/absolute-positioning/grid-area-containing-block.ts.ea4f42271.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/absolute-positioning/mixed-positioning.ts.028acfb41.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/absolute-positioning/mixed-positioning.ts.1a9728df1.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/absolute-positioning/mixed-positioning.ts.d5b51e041.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/absolute-positioning/mixed-positioning.ts.dbffb6a71.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/absolute-positioning/mixed-positioning.ts.e9601c991.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/absolute-positioning/offset-properties.ts.141ecd301.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/absolute-positioning/offset-properties.ts.3ae0c0451.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/absolute-positioning/offset-properties.ts.68dee0011.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/absolute-positioning/offset-properties.ts.ac6f25cd1.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/absolute-positioning/offset-properties.ts.dae08e381.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/absolute-positioning/offset-properties.ts.e66d21a91.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/absolute-positioning/offset-properties.ts.fddfdb901.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/absolute-positioning/z-index-stacking.ts.22649a731.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/absolute-positioning/z-index-stacking.ts.28b6b6b01.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/absolute-positioning/z-index-stacking.ts.33507a671.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/absolute-positioning/z-index-stacking.ts.9ad4b2ed1.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/absolute-positioning/z-index-stacking.ts.cdad5f421.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/absolute-positioning/z-index-stacking.ts.dc53801a1.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/absolute-positioning/z-index-stacking.ts.e1fd88961.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/animation/gap-transition.ts.059bb6411.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/animation/gap-transition.ts.059bb6412.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/animation/gap-transition.ts.07e3adbe1.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/animation/gap-transition.ts.07e3adbe2.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/animation/gap-transition.ts.13fa2fbe1.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/animation/gap-transition.ts.13fa2fbe2.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/animation/gap-transition.ts.211b53671.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/animation/gap-transition.ts.211b53672.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/animation/gap-transition.ts.b63f190b1.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/animation/gap-transition.ts.b63f190b2.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/animation/gap-transition.ts.bd4d2fe11.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/animation/gap-transition.ts.bd4d2fe12.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/animation/track-size-transition.ts.038fd85e1.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/animation/track-size-transition.ts.038fd85e2.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/animation/track-size-transition.ts.36246cff1.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/animation/track-size-transition.ts.36246cff2.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/animation/track-size-transition.ts.5108343f1.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/animation/track-size-transition.ts.5108343f2.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/animation/track-size-transition.ts.5cac593f1.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/animation/track-size-transition.ts.5cac593f2.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/animation/track-size-transition.ts.71f129d61.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/animation/track-size-transition.ts.71f129d62.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/animation/track-size-transition.ts.9a68fd351.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/animation/track-size-transition.ts.9a68fd352.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/edge-cases/empty-grids.ts.0cc469de1.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/edge-cases/empty-grids.ts.3f4d6cf31.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/edge-cases/empty-grids.ts.8e8761f51.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/edge-cases/empty-grids.ts.92110d3f1.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/edge-cases/empty-grids.ts.c0a268df1.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/edge-cases/empty-grids.ts.e435756e1.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/edge-cases/extreme-values.ts.0f19f7561.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/edge-cases/extreme-values.ts.814603561.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/edge-cases/extreme-values.ts.b77cadf91.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/edge-cases/extreme-values.ts.bf43c1e01.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/edge-cases/extreme-values.ts.c801b6241.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/edge-cases/extreme-values.ts.f906e8301.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/edge-cases/overlarge-grids.ts.6c4049521.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/edge-cases/overlarge-grids.ts.7299c48f1.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/edge-cases/overlarge-grids.ts.7d968f741.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/edge-cases/overlarge-grids.ts.7d968f742.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/edge-cases/overlarge-grids.ts.c070f3e11.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/edge-cases/overlarge-grids.ts.d2a36ce11.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/edge-cases/overlarge-grids.ts.de72fda81.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/edge-cases/overlarge-grids.ts.de72fda82.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/edge-cases/single-cell.ts.19c530c11.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/edge-cases/single-cell.ts.70041cfd1.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/edge-cases/single-cell.ts.7b16a8d81.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/edge-cases/single-cell.ts.8eaad60a1.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/edge-cases/single-cell.ts.ac5f61a41.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/edge-cases/single-cell.ts.aef485221.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/grid-model/display-grid.ts.48c2911b1.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/grid-model/display-grid.ts.559ac83a1.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/grid-model/display-grid.ts.830e52921.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/grid-model/display-grid.ts.9087d1231.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/grid-model/display-grid.ts.b9867d5a1.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/grid-model/display-grid.ts.d9bb9f631.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/grid-model/grid-containers.ts.289242731.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/grid-model/grid-containers.ts.2bc48e101.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/grid-model/grid-containers.ts.5d3a98b81.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/grid-model/grid-containers.ts.67fbd5221.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/grid-model/grid-containers.ts.7281cab11.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/grid-model/grid-containers.ts.9f91419a1.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/grid-model/grid-items-types.ts.316397011.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/grid-model/grid-items-types.ts.4cb79d5e1.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/grid-model/grid-items-types.ts.5aabd7611.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/grid-model/grid-items-types.ts.7eda30731.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/grid-model/grid-items-types.ts.ced74cdd1.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/grid-model/grid-items-types.ts.ddcc8ddf1.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/grid-model/writing-modes.ts.3a5256f51.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/interactions/grid-in-flex.ts.2463a4281.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/interactions/grid-in-flex.ts.39e566571.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/interactions/grid-in-flex.ts.4bc545c31.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/interactions/grid-in-flex.ts.91e24b9d1.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/interactions/grid-in-flex.ts.97b3e1cc1.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/interactions/grid-in-flex.ts.c56da9c61.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/interactions/nested-grids.ts.25c43f781.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/interactions/nested-grids.ts.330773ec1.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/interactions/nested-grids.ts.39acb8491.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/interactions/nested-grids.ts.5db5f5ff1.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/interactions/nested-grids.ts.6cf98d4a1.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/interactions/nested-grids.ts.d20338fd1.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/layout-algorithm/circular-dependencies.ts.1e6cf25f1.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/layout-algorithm/circular-dependencies.ts.56788ab41.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/layout-algorithm/circular-dependencies.ts.d27cb34a1.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/layout-algorithm/circular-dependencies.ts.f1955aa01.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/layout-algorithm/sizing-resolution.ts.063ffe1f1.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/layout-algorithm/sizing-resolution.ts.9e1142381.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/layout-algorithm/sizing-resolution.ts.f4352ac11.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/layout-algorithm/sizing-resolution.ts.fc0807cc1.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/layout-algorithm/sizing-resolution.ts.fedd994c1.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/layout-algorithm/sizing-resolution.ts.ffecd24e1.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/parsing/computed-values-extended.ts.09ab9dcc1.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/parsing/computed-values-extended.ts.0dafd02b1.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/parsing/computed-values-extended.ts.21488ae51.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/parsing/computed-values-extended.ts.736d24441.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/parsing/computed-values-extended.ts.bfb12c911.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/parsing/computed-values-extended.ts.fc4df38d1.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/parsing/getComputedStyle-complete.ts.00edf2c91.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/parsing/getComputedStyle-complete.ts.79ee80021.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/parsing/getComputedStyle-complete.ts.7a597f691.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/parsing/getComputedStyle-complete.ts.bbf64a6c1.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/parsing/getComputedStyle-complete.ts.c116e0261.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/parsing/getComputedStyle-complete.ts.f3349d481.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/parsing/invalid-values.ts.066bd65e1.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/parsing/invalid-values.ts.190e5d211.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/parsing/invalid-values.ts.6af74a251.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/parsing/invalid-values.ts.7c5b16e81.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/parsing/invalid-values.ts.d09f5d111.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/parsing/invalid-values.ts.d3019f1c1.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/parsing/serialization.ts.5a79f1811.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/parsing/serialization.ts.60cfa40a1.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/parsing/serialization.ts.6416aa201.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/parsing/serialization.ts.7615af761.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/parsing/serialization.ts.ead509201.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/parsing/serialization.ts.f5e534ba1.pngis excluded by!**/*.pngintegration_tests/snapshots/css/css-grid/parsing/serialization.ts.fb34c7561.pngis excluded by!**/*.png
📒 Files selected for processing (30)
.gitignoreCSS_GRID_PLAN.mdbridge/core/css/parser/css_property_parser.ccintegration_tests/lib/main.dartintegration_tests/specs/css/css-grid/absolute-positioning/basic-positioning.tsintegration_tests/specs/css/css-grid/absolute-positioning/grid-area-containing-block.tsintegration_tests/specs/css/css-grid/absolute-positioning/mixed-positioning.tsintegration_tests/specs/css/css-grid/absolute-positioning/offset-properties.tsintegration_tests/specs/css/css-grid/absolute-positioning/z-index-stacking.tsintegration_tests/specs/css/css-grid/animation/gap-transition.tsintegration_tests/specs/css/css-grid/animation/track-size-transition.tsintegration_tests/specs/css/css-grid/edge-cases/empty-grids.tsintegration_tests/specs/css/css-grid/edge-cases/extreme-values.tsintegration_tests/specs/css/css-grid/edge-cases/overlarge-grids.tsintegration_tests/specs/css/css-grid/edge-cases/single-cell.tsintegration_tests/specs/css/css-grid/grid-definition/explicit-tracks.tsintegration_tests/specs/css/css-grid/grid-definition/repeat-notation.tsintegration_tests/specs/css/css-grid/grid-items/baseline.tsintegration_tests/specs/css/css-grid/grid-model/display-grid.tsintegration_tests/specs/css/css-grid/grid-model/grid-containers.tsintegration_tests/specs/css/css-grid/grid-model/grid-items-types.tsintegration_tests/specs/css/css-grid/grid-model/writing-modes.tsintegration_tests/specs/css/css-grid/interactions/grid-in-flex.tsintegration_tests/specs/css/css-grid/interactions/nested-grids.tsintegration_tests/specs/css/css-grid/layout-algorithm/circular-dependencies.tsintegration_tests/specs/css/css-grid/layout-algorithm/sizing-resolution.tsintegration_tests/specs/css/css-grid/parsing/computed-values-extended.tsintegration_tests/specs/css/css-grid/parsing/getComputedStyle-complete.tsintegration_tests/specs/css/css-grid/parsing/invalid-values.tsintegration_tests/specs/css/css-grid/parsing/serialization.ts
✅ Files skipped from review due to trivial changes (2)
- .gitignore
- integration_tests/specs/css/css-grid/grid-definition/explicit-tracks.ts
🚧 Files skipped from review as they are similar to previous changes (13)
- integration_tests/specs/css/css-grid/parsing/invalid-values.ts
- integration_tests/specs/css/css-grid/grid-model/writing-modes.ts
- integration_tests/specs/css/css-grid/parsing/computed-values-extended.ts
- integration_tests/specs/css/css-grid/parsing/getComputedStyle-complete.ts
- integration_tests/specs/css/css-grid/absolute-positioning/offset-properties.ts
- integration_tests/specs/css/css-grid/grid-model/display-grid.ts
- integration_tests/lib/main.dart
- integration_tests/specs/css/css-grid/absolute-positioning/grid-area-containing-block.ts
- integration_tests/specs/css/css-grid/animation/track-size-transition.ts
- integration_tests/specs/css/css-grid/grid-definition/repeat-notation.ts
- integration_tests/specs/css/css-grid/absolute-positioning/z-index-stacking.ts
- integration_tests/specs/css/css-grid/edge-cases/empty-grids.ts
- integration_tests/specs/css/css-grid/animation/gap-transition.ts
🧰 Additional context used
📓 Path-based instructions (6)
integration_tests/specs/**/*.ts
📄 CodeRabbit inference engine (integration_tests/CLAUDE.md)
integration_tests/specs/**/*.ts: Place tests in appropriate directories underspecs/(css/, dom/, or window/)
Use TypeScript (.ts extension) for test files
Usedone()callback for async tests
Usesnapshot()for visual regression tests to capture current rendering
UsesimulateClickwith coordinates for hit testing tests
Test assets should reference files inassets/directory
Usefdescribe()instead ofdescribe()to run only specific test specs (Jasmine feature)
Usefit()instead ofit()to run only specific test cases
Snapshots are stored as images for comparison and failed snapshots generate diff images
The max width of testing window is 340px
Test specs will always pass if there are no existing snapshots
Group related tests in describe blocks
Each test should be independent
Remove created elements after tests (test cleanup)
Use clear, descriptive test names
Test behavior, not implementation
Include edge cases and error conditions in tests
Batch DOM operations to minimize reflows
Use async/await and proper async patterns for asynchronous operations in tests
Measure operations usingperformance.now()for timing in performance-critical tests
Files:
integration_tests/specs/css/css-grid/interactions/grid-in-flex.tsintegration_tests/specs/css/css-grid/absolute-positioning/mixed-positioning.tsintegration_tests/specs/css/css-grid/grid-model/grid-items-types.tsintegration_tests/specs/css/css-grid/grid-model/grid-containers.tsintegration_tests/specs/css/css-grid/edge-cases/overlarge-grids.tsintegration_tests/specs/css/css-grid/absolute-positioning/basic-positioning.tsintegration_tests/specs/css/css-grid/layout-algorithm/sizing-resolution.tsintegration_tests/specs/css/css-grid/edge-cases/single-cell.tsintegration_tests/specs/css/css-grid/parsing/serialization.tsintegration_tests/specs/css/css-grid/interactions/nested-grids.tsintegration_tests/specs/css/css-grid/edge-cases/extreme-values.tsintegration_tests/specs/css/css-grid/grid-items/baseline.tsintegration_tests/specs/css/css-grid/layout-algorithm/circular-dependencies.ts
bridge/**/*.{cc,cpp,h,hpp}
📄 CodeRabbit inference engine (bridge/CLAUDE.md)
bridge/**/*.{cc,cpp,h,hpp}: C++ code should follow Chromium style (.clang-format) with C++17 standard, 120 character column limit, and 2-space indentation
UseWEBF_EXPORT_Cmacro for exporting C functions to Dart FFI
In FFI contexts, useDart_Handleinstead ofHandlefor type compatibility
For C++ FFI function naming: useGetObjectPropertiesFromDartfor C++ exports,NativeGetObjectPropertiesFuncfor Dart typedefs, andGetObjectPropertiesFuncfor Dart functions
Lambda signatures in C++ must match expected function signatures to avoid FFI type mismatches
Choose power-of-2 buffer sizes (512, 1024, 2048) for ring buffer implementation, with smaller sizes for DEBUG_BUILD, MOBILE constraints, and larger sizes for DESKTOP performance
Files:
bridge/core/css/parser/css_property_parser.cc
bridge/**/*.{cc,cpp}
📄 CodeRabbit inference engine (bridge/CLAUDE.md)
bridge/**/*.{cc,cpp}: For async FFI operations, useDart_NewPersistentHandle_DLto keep Dart objects alive, convert back withDart_HandleFromPersistent_DLbefore use, and always callDart_DeletePersistentHandle_DLafter the async operation completes
For string handling in FFI, copy strings that might be freed usingstd::string(const_char_ptr), and usetoNativeUtf8()with proper memory deallocation
For async callbacks with potential errors, always provide error path in callbacks (e.g.,callback(object, nullptr)), handle cancellation cases in async operations, and propagate errors through callback parameters rather than exceptions
For thread-safe error reporting in FFI, copy error messages before crossing thread boundaries usingstd::stringto ensure string lifetime, and consider error callbacks separate from result callbacks
AvoidPostToJsSyncwhen threads may interdepend to prevent synchronous deadlocks in FFI communication
Ensure callback functions aren't invoked after context destruction to prevent use-after-free errors in FFI async operations
Implement ring buffer overflow handling with metrics and alerts to monitor command buffer capacity
Process multiple UI commands per frame in a loop with a MAX_COMMANDS_PER_FRAME limit to balance responsiveness and performance
Pin threads to cores for optimal cache usage in ring buffer operations by setting CPU affinity for UI threads
Use PostToJs for executing operations on the JS thread from other threads, PostToDart for returning results to Dart isolate, and avoid PostToJsSync to prevent deadlocks
Files:
bridge/core/css/parser/css_property_parser.cc
bridge/**/*.{cc,h}
📄 CodeRabbit inference engine (AGENTS.md)
C++ code in bridge module must use C++17 standard with 2-space indentation, 120 column limit, and follow Chromium style guide as defined in
.clang-format
bridge/**/*.{cc,h}: Use RAII patterns in C++ where possible for resource management
UsePostToJsfor executing operations on the JS thread in FFI
UsePostToDartfor returning results to Dart isolate
AvoidPostToJsSyncsynchronous execution when possibleC++ bridge code must use 2-space indent, 120 column line limit, and follow Chromium style via
.clang-format
Files:
bridge/core/css/parser/css_property_parser.cc
{bridge/**/*.{cc,h},webf/**/*.dart}
📄 CodeRabbit inference engine (CLAUDE.md)
Document memory ownership clearly in FFI implementations
Files:
bridge/core/css/parser/css_property_parser.cc
bridge/core/**
📄 CodeRabbit inference engine (bridge/AGENTS.md)
Core bridge code should be located in
bridge/core/directory
Files:
bridge/core/css/parser/css_property_parser.cc
🧠 Learnings (19)
📓 Common learnings
Learnt from: CGQAQ
Repo: openwebf/webf PR: 777
File: integration_tests/specs/css/css-text-mixin/text_comprehensive_test.ts:389-391
Timestamp: 2025-12-26T09:18:25.075Z
Learning: In WebF integration tests, the background shorthand property syntax `background: <gradient> text` is valid CSS per MDN spec, where "text" is a valid background-clip value. The explicit backgroundClip assignment that follows is a workaround for WebF's current CSS parser limitation, not because the syntax is invalid.
Learnt from: CR
Repo: openwebf/webf PR: 0
File: integration_tests/CLAUDE.md:0-0
Timestamp: 2025-12-08T23:27:27.888Z
Learning: Applies to integration_tests/specs/**/*.ts : Place tests in appropriate directories under `specs/` (css/, dom/, or window/)
📚 Learning: 2025-12-08T23:27:27.888Z
Learnt from: CR
Repo: openwebf/webf PR: 0
File: integration_tests/CLAUDE.md:0-0
Timestamp: 2025-12-08T23:27:27.888Z
Learning: Applies to integration_tests/specs/**/*.ts : Place tests in appropriate directories under `specs/` (css/, dom/, or window/)
Applied to files:
integration_tests/specs/css/css-grid/interactions/grid-in-flex.tsintegration_tests/specs/css/css-grid/absolute-positioning/mixed-positioning.tsintegration_tests/specs/css/css-grid/grid-model/grid-items-types.tsCSS_GRID_PLAN.mdintegration_tests/specs/css/css-grid/grid-model/grid-containers.tsintegration_tests/specs/css/css-grid/edge-cases/overlarge-grids.tsintegration_tests/specs/css/css-grid/absolute-positioning/basic-positioning.tsintegration_tests/specs/css/css-grid/layout-algorithm/sizing-resolution.tsintegration_tests/specs/css/css-grid/edge-cases/single-cell.tsintegration_tests/specs/css/css-grid/parsing/serialization.tsintegration_tests/specs/css/css-grid/interactions/nested-grids.tsintegration_tests/specs/css/css-grid/edge-cases/extreme-values.tsintegration_tests/specs/css/css-grid/grid-items/baseline.tsintegration_tests/specs/css/css-grid/layout-algorithm/circular-dependencies.ts
📚 Learning: 2025-12-08T23:27:27.888Z
Learnt from: CR
Repo: openwebf/webf PR: 0
File: integration_tests/CLAUDE.md:0-0
Timestamp: 2025-12-08T23:27:27.888Z
Learning: Applies to integration_tests/specs/**/*.ts : Batch DOM operations to minimize reflows
Applied to files:
integration_tests/specs/css/css-grid/interactions/grid-in-flex.tsintegration_tests/specs/css/css-grid/absolute-positioning/mixed-positioning.tsintegration_tests/specs/css/css-grid/grid-model/grid-items-types.tsCSS_GRID_PLAN.mdintegration_tests/specs/css/css-grid/grid-model/grid-containers.tsintegration_tests/specs/css/css-grid/edge-cases/overlarge-grids.tsintegration_tests/specs/css/css-grid/absolute-positioning/basic-positioning.tsintegration_tests/specs/css/css-grid/layout-algorithm/sizing-resolution.tsintegration_tests/specs/css/css-grid/edge-cases/single-cell.tsintegration_tests/specs/css/css-grid/parsing/serialization.tsintegration_tests/specs/css/css-grid/interactions/nested-grids.tsintegration_tests/specs/css/css-grid/edge-cases/extreme-values.tsintegration_tests/specs/css/css-grid/grid-items/baseline.tsintegration_tests/specs/css/css-grid/layout-algorithm/circular-dependencies.ts
📚 Learning: 2025-12-08T23:27:27.888Z
Learnt from: CR
Repo: openwebf/webf PR: 0
File: integration_tests/CLAUDE.md:0-0
Timestamp: 2025-12-08T23:27:27.888Z
Learning: Applies to integration_tests/specs/**/*.ts : Test behavior, not implementation
Applied to files:
integration_tests/specs/css/css-grid/interactions/grid-in-flex.tsintegration_tests/specs/css/css-grid/absolute-positioning/mixed-positioning.tsintegration_tests/specs/css/css-grid/grid-model/grid-items-types.tsCSS_GRID_PLAN.mdintegration_tests/specs/css/css-grid/grid-model/grid-containers.tsintegration_tests/specs/css/css-grid/edge-cases/overlarge-grids.tsintegration_tests/specs/css/css-grid/absolute-positioning/basic-positioning.tsintegration_tests/specs/css/css-grid/edge-cases/single-cell.tsintegration_tests/specs/css/css-grid/parsing/serialization.tsintegration_tests/specs/css/css-grid/interactions/nested-grids.tsintegration_tests/specs/css/css-grid/edge-cases/extreme-values.tsintegration_tests/specs/css/css-grid/grid-items/baseline.tsintegration_tests/specs/css/css-grid/layout-algorithm/circular-dependencies.ts
📚 Learning: 2025-12-08T23:27:27.888Z
Learnt from: CR
Repo: openwebf/webf PR: 0
File: integration_tests/CLAUDE.md:0-0
Timestamp: 2025-12-08T23:27:27.888Z
Learning: Applies to integration_tests/specs/**/*.ts : Use `snapshot()` for visual regression tests to capture current rendering
Applied to files:
integration_tests/specs/css/css-grid/interactions/grid-in-flex.tsintegration_tests/specs/css/css-grid/absolute-positioning/mixed-positioning.tsintegration_tests/specs/css/css-grid/grid-model/grid-items-types.tsintegration_tests/specs/css/css-grid/grid-model/grid-containers.tsintegration_tests/specs/css/css-grid/edge-cases/overlarge-grids.tsintegration_tests/specs/css/css-grid/absolute-positioning/basic-positioning.tsintegration_tests/specs/css/css-grid/layout-algorithm/sizing-resolution.tsintegration_tests/specs/css/css-grid/edge-cases/single-cell.tsintegration_tests/specs/css/css-grid/parsing/serialization.tsintegration_tests/specs/css/css-grid/interactions/nested-grids.tsintegration_tests/specs/css/css-grid/edge-cases/extreme-values.tsintegration_tests/specs/css/css-grid/layout-algorithm/circular-dependencies.ts
📚 Learning: 2025-12-08T23:27:27.888Z
Learnt from: CR
Repo: openwebf/webf PR: 0
File: integration_tests/CLAUDE.md:0-0
Timestamp: 2025-12-08T23:27:27.888Z
Learning: Applies to integration_tests/specs/**/*.ts : Group related tests in describe blocks
Applied to files:
integration_tests/specs/css/css-grid/interactions/grid-in-flex.tsintegration_tests/specs/css/css-grid/absolute-positioning/mixed-positioning.tsintegration_tests/specs/css/css-grid/grid-model/grid-items-types.tsintegration_tests/specs/css/css-grid/grid-model/grid-containers.tsintegration_tests/specs/css/css-grid/edge-cases/overlarge-grids.tsintegration_tests/specs/css/css-grid/absolute-positioning/basic-positioning.tsintegration_tests/specs/css/css-grid/edge-cases/single-cell.tsintegration_tests/specs/css/css-grid/parsing/serialization.tsintegration_tests/specs/css/css-grid/interactions/nested-grids.tsintegration_tests/specs/css/css-grid/layout-algorithm/circular-dependencies.ts
📚 Learning: 2025-12-08T23:27:27.888Z
Learnt from: CR
Repo: openwebf/webf PR: 0
File: integration_tests/CLAUDE.md:0-0
Timestamp: 2025-12-08T23:27:27.888Z
Learning: Applies to integration_tests/specs/**/*.ts : The max width of testing window is 340px
Applied to files:
integration_tests/specs/css/css-grid/interactions/grid-in-flex.tsintegration_tests/specs/css/css-grid/grid-model/grid-containers.tsintegration_tests/specs/css/css-grid/edge-cases/overlarge-grids.tsintegration_tests/specs/css/css-grid/layout-algorithm/sizing-resolution.tsintegration_tests/specs/css/css-grid/edge-cases/single-cell.tsintegration_tests/specs/css/css-grid/edge-cases/extreme-values.tsintegration_tests/specs/css/css-grid/layout-algorithm/circular-dependencies.ts
📚 Learning: 2025-12-08T23:27:27.888Z
Learnt from: CR
Repo: openwebf/webf PR: 0
File: integration_tests/CLAUDE.md:0-0
Timestamp: 2025-12-08T23:27:27.888Z
Learning: Applies to integration_tests/specs/**/*.ts : Include edge cases and error conditions in tests
Applied to files:
integration_tests/specs/css/css-grid/interactions/grid-in-flex.tsintegration_tests/specs/css/css-grid/absolute-positioning/mixed-positioning.tsintegration_tests/specs/css/css-grid/grid-model/grid-items-types.tsCSS_GRID_PLAN.mdintegration_tests/specs/css/css-grid/grid-model/grid-containers.tsintegration_tests/specs/css/css-grid/edge-cases/overlarge-grids.tsintegration_tests/specs/css/css-grid/absolute-positioning/basic-positioning.tsintegration_tests/specs/css/css-grid/edge-cases/single-cell.tsintegration_tests/specs/css/css-grid/parsing/serialization.tsintegration_tests/specs/css/css-grid/interactions/nested-grids.tsintegration_tests/specs/css/css-grid/edge-cases/extreme-values.tsintegration_tests/specs/css/css-grid/layout-algorithm/circular-dependencies.ts
📚 Learning: 2025-12-08T23:27:27.888Z
Learnt from: CR
Repo: openwebf/webf PR: 0
File: integration_tests/CLAUDE.md:0-0
Timestamp: 2025-12-08T23:27:27.888Z
Learning: Applies to integration_tests/specs/**/*.ts : Each test should be independent
Applied to files:
integration_tests/specs/css/css-grid/interactions/grid-in-flex.tsintegration_tests/specs/css/css-grid/grid-model/grid-containers.tsintegration_tests/specs/css/css-grid/edge-cases/single-cell.tsintegration_tests/specs/css/css-grid/interactions/nested-grids.tsintegration_tests/specs/css/css-grid/layout-algorithm/circular-dependencies.ts
📚 Learning: 2025-12-08T23:27:27.888Z
Learnt from: CR
Repo: openwebf/webf PR: 0
File: integration_tests/CLAUDE.md:0-0
Timestamp: 2025-12-08T23:27:27.888Z
Learning: Applies to integration_tests/specs/**/*.ts : Use clear, descriptive test names
Applied to files:
integration_tests/specs/css/css-grid/interactions/grid-in-flex.tsintegration_tests/specs/css/css-grid/grid-model/grid-items-types.tsintegration_tests/specs/css/css-grid/edge-cases/overlarge-grids.tsintegration_tests/specs/css/css-grid/absolute-positioning/basic-positioning.tsintegration_tests/specs/css/css-grid/edge-cases/single-cell.tsintegration_tests/specs/css/css-grid/parsing/serialization.tsintegration_tests/specs/css/css-grid/interactions/nested-grids.tsintegration_tests/specs/css/css-grid/edge-cases/extreme-values.ts
📚 Learning: 2025-12-08T23:27:27.888Z
Learnt from: CR
Repo: openwebf/webf PR: 0
File: integration_tests/CLAUDE.md:0-0
Timestamp: 2025-12-08T23:27:27.888Z
Learning: Applies to integration_tests/specs/**/*.ts : Remove created elements after tests (test cleanup)
Applied to files:
integration_tests/specs/css/css-grid/interactions/grid-in-flex.tsintegration_tests/specs/css/css-grid/grid-model/grid-items-types.tsintegration_tests/specs/css/css-grid/grid-model/grid-containers.tsintegration_tests/specs/css/css-grid/edge-cases/overlarge-grids.tsintegration_tests/specs/css/css-grid/absolute-positioning/basic-positioning.tsintegration_tests/specs/css/css-grid/edge-cases/single-cell.tsintegration_tests/specs/css/css-grid/parsing/serialization.tsintegration_tests/specs/css/css-grid/interactions/nested-grids.tsintegration_tests/specs/css/css-grid/edge-cases/extreme-values.tsintegration_tests/specs/css/css-grid/layout-algorithm/circular-dependencies.ts
📚 Learning: 2025-12-08T23:27:27.888Z
Learnt from: CR
Repo: openwebf/webf PR: 0
File: integration_tests/CLAUDE.md:0-0
Timestamp: 2025-12-08T23:27:27.888Z
Learning: Applies to integration_tests/specs/**/*.ts : Use `simulateClick` with coordinates for hit testing tests
Applied to files:
integration_tests/specs/css/css-grid/absolute-positioning/mixed-positioning.tsintegration_tests/specs/css/css-grid/absolute-positioning/basic-positioning.ts
📚 Learning: 2025-12-08T23:27:27.888Z
Learnt from: CR
Repo: openwebf/webf PR: 0
File: integration_tests/CLAUDE.md:0-0
Timestamp: 2025-12-08T23:27:27.888Z
Learning: Applies to integration_tests/specs/**/*.ts : Use async/await and proper async patterns for asynchronous operations in tests
Applied to files:
integration_tests/specs/css/css-grid/absolute-positioning/mixed-positioning.tsintegration_tests/specs/css/css-grid/absolute-positioning/basic-positioning.tsintegration_tests/specs/css/css-grid/parsing/serialization.ts
📚 Learning: 2025-12-08T23:27:27.888Z
Learnt from: CR
Repo: openwebf/webf PR: 0
File: integration_tests/CLAUDE.md:0-0
Timestamp: 2025-12-08T23:27:27.888Z
Learning: Applies to integration_tests/specs/**/*.ts : Test assets should reference files in `assets/` directory
Applied to files:
integration_tests/specs/css/css-grid/absolute-positioning/mixed-positioning.tsintegration_tests/specs/css/css-grid/layout-algorithm/circular-dependencies.ts
📚 Learning: 2025-12-08T23:27:27.888Z
Learnt from: CR
Repo: openwebf/webf PR: 0
File: integration_tests/CLAUDE.md:0-0
Timestamp: 2025-12-08T23:27:27.888Z
Learning: Applies to integration_tests/specs/**/*.ts : Snapshots are stored as images for comparison and failed snapshots generate diff images
Applied to files:
integration_tests/specs/css/css-grid/absolute-positioning/mixed-positioning.tsintegration_tests/specs/css/css-grid/grid-model/grid-items-types.tsintegration_tests/specs/css/css-grid/grid-model/grid-containers.tsintegration_tests/specs/css/css-grid/edge-cases/overlarge-grids.tsintegration_tests/specs/css/css-grid/parsing/serialization.tsintegration_tests/specs/css/css-grid/interactions/nested-grids.tsintegration_tests/specs/css/css-grid/edge-cases/extreme-values.ts
📚 Learning: 2025-12-08T23:27:41.357Z
Learnt from: CR
Repo: openwebf/webf PR: 0
File: scripts/CLAUDE.md:0-0
Timestamp: 2025-12-08T23:27:41.357Z
Learning: Test TypeScript typings by running `npm test` in the `bridge/typings` directory to ensure types compile correctly without DOM lib
Applied to files:
integration_tests/specs/css/css-grid/grid-model/grid-items-types.ts
📚 Learning: 2025-12-08T23:28:11.651Z
Learnt from: CR
Repo: openwebf/webf PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-08T23:28:11.651Z
Learning: Applies to cli/test/**/*.test.ts : TypeScript test files must use `.test.ts` extension and target at least 70% code coverage for core modules
Applied to files:
CSS_GRID_PLAN.md
📚 Learning: 2025-11-26T10:24:13.090Z
Learnt from: CR
Repo: openwebf/webf PR: 0
File: cli/CLAUDE.md:0-0
Timestamp: 2025-11-26T10:24:13.090Z
Learning: Ensure test coverage threshold of at least 70% across all core modules
Applied to files:
CSS_GRID_PLAN.md
📚 Learning: 2025-12-08T23:27:27.888Z
Learnt from: CR
Repo: openwebf/webf PR: 0
File: integration_tests/CLAUDE.md:0-0
Timestamp: 2025-12-08T23:27:27.888Z
Learning: Applies to integration_tests/specs/**/*.ts : Measure operations using `performance.now()` for timing in performance-critical tests
Applied to files:
integration_tests/specs/css/css-grid/edge-cases/extreme-values.ts
🧬 Code graph analysis (1)
bridge/core/css/parser/css_property_parser.cc (1)
bridge/core/css/properties/css_parsing_utils.cc (2)
MaybeConsumeImportant(6218-6240)MaybeConsumeImportant(6218-6218)
🪛 ast-grep (0.40.3)
integration_tests/specs/css/css-grid/layout-algorithm/circular-dependencies.ts
[warning] 9-10: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: item.innerHTML =
'Item with percentage height'
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
[warning] 9-10: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: item.innerHTML =
'Item with percentage height'
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
🪛 markdownlint-cli2 (0.18.1)
CSS_GRID_PLAN.md
211-211: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build_windows_bridge (Debug)
- GitHub Check: Analyze (c-cpp)
- GitHub Check: Analyze (rust)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (13)
integration_tests/specs/css/css-grid/grid-items/baseline.ts (1)
331-331: LGTM! Clear documentation of planned work.The TODO comment appropriately documents the vertical writing mode limitation for this baseline test, which is already disabled with
xit.integration_tests/specs/css/css-grid/grid-model/grid-containers.ts (1)
1-202: LGTM! Well-structured test suite.The test suite comprehensively validates CSS Grid container behavior with proper DOM batching, cleanup, and clear assertions. All tests follow best practices from the coding guidelines.
integration_tests/specs/css/css-grid/interactions/nested-grids.ts (1)
1-295: LGTM! Comprehensive nested grid test coverage.The test suite thoroughly validates nested grid behavior with proper batching, independent tests, snapshots, and cleanup. Well-structured according to coding guidelines.
integration_tests/specs/css/css-grid/grid-model/grid-items-types.ts (1)
1-179: LGTM! Thorough validation of grid item types.The test suite correctly validates that various DOM element types (div, span, img, text nodes, button, input) can function as grid items. Proper use of
childNodesfor text node validation and consistent cleanup throughout.integration_tests/specs/css/css-grid/parsing/serialization.ts (1)
1-173: LGTM! Comprehensive serialization validation.The test suite thoroughly validates CSS Grid property serialization, including shorthand/longhand equivalence and whitespace normalization. Tests correctly verify that shorthand properties (gap, grid-column, place-items, place-self) set their corresponding longhand properties, aligning with the parser enhancements mentioned in the PR context.
bridge/core/css/parser/css_property_parser.cc (1)
125-173: LGTM! Shorthand expansion logic is correctly implemented.The implementation properly:
- Saves the properties state before parsing for potential rollback
- Parses the shorthand into longhands with
important=false- Reverts expanded properties on parse failure
- Post-processes the
!importantannotation separately- Validates stream state after expansion
- Applies the important flag to all expanded longhands
This matches the established pattern for handling CSS shorthands and the
!importantannotation.integration_tests/specs/css/css-grid/edge-cases/single-cell.ts (1)
1-160: LGTM! Comprehensive single-cell grid test coverage.The test suite provides excellent coverage of single-cell grid scenarios:
- Fixed sizing, auto sizing, and fr units
- Minmax constraints
- Item alignment and centering
- Edge case with empty grid
All tests follow best practices:
- Clear, descriptive names
- Proper async patterns with
waitForFrame()andsnapshot()- Independent tests with cleanup
- Appropriate assertions for each scenario
- All dimensions respect the 340px viewport constraint
integration_tests/specs/css/css-grid/absolute-positioning/mixed-positioning.ts (1)
54-105: Re-enable skipped test when implementation is ready.This test is currently skipped with
xit. Ensure this test is re-enabled once the underlying functionality is implemented.integration_tests/specs/css/css-grid/absolute-positioning/basic-positioning.ts (1)
273-309: Re-enable skipped test when implementation is ready.This test verifies that absolutely positioned items ignore grid placement properties. Ensure this is re-enabled once the behavior is fully implemented.
integration_tests/specs/css/css-grid/edge-cases/overlarge-grids.ts (1)
34-39: LGTM! Well-designed scroll behavior tests.The tests correctly verify scroll behavior by capturing element positions before and after scroll, then asserting the expected displacement. The use of
snapshot(0.5)for visual regression after scroll is appropriate.Also applies to: 75-80
integration_tests/specs/css/css-grid/edge-cases/extreme-values.ts (2)
126-158: LGTM! Proper handling of fractional pixel values.The test correctly handles fractional pixel scenarios by using tolerance-based comparisons (
Math.abs(totalWidth - 333)).toBeLessThan(2)). This accounts for rounding differences in CSS layout calculations.
160-192: LGTM! Zero-width track test is well-designed.Setting
minWidth: '0'(line 177) is necessary to allow grid items to collapse to zero width, making this a valid test of zero-width track behavior.CSS_GRID_PLAN.md (1)
93-95: LGTM! Documentation accurately reflects completed phases.The updates correctly mark Phases 1-8 as complete and document the comprehensive test coverage (529 tests across 77 files).
| **Total Completed: 77 files, 529 integration tests** ✅ | ||
|
|
||
| #### Updated WPT Test Distribution | ||
| | Category | Test Count | Percentage | WebF Coverage | |
There was a problem hiding this comment.
Fix markdown formatting.
Add blank lines before and after the table to comply with markdown formatting standards.
🔎 Proposed fix
**Total Completed: 77 files, 529 integration tests** ✅
#### Updated WPT Test Distribution
+
| Category | Test Count | Percentage | WebF Coverage |
|----------|-----------|------------|---------------|
| Grid Lanes (Track Sizing) | 789 | 33% | ✅ **Good** (89 tests) |
+
**Updated Findings:**As per static analysis: tables should be surrounded by blank lines.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| | Category | Test Count | Percentage | WebF Coverage | | |
| **Total Completed: 77 files, 529 integration tests** ✅ | |
| #### Updated WPT Test Distribution | |
| | Category | Test Count | Percentage | WebF Coverage | | |
| |----------|-----------|------------|---------------| | |
| | Grid Lanes (Track Sizing) | 789 | 33% | ✅ **Good** (89 tests) | | |
| **Updated Findings:** |
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
211-211: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
🤖 Prompt for AI Agents
In CSS_GRID_PLAN.md around line 211, the table row lacks blank lines before and
after the table block; add a single blank line immediately above the table start
and a single blank line immediately after the table end so the Markdown parser
treats it as a proper table block, then verify adjacent content (headings or
paragraphs) remains separated by those blank lines.
| xit('handles fixed positioning within grid', async () => { | ||
| const grid = document.createElement('div'); | ||
| grid.style.display = 'grid'; | ||
| grid.style.gridTemplateColumns = 'repeat(2, 130px)'; | ||
| grid.style.gridTemplateRows = 'repeat(2, 90px)'; | ||
| grid.style.gap = '10px'; | ||
| grid.style.backgroundColor = '#e0f2f1'; | ||
|
|
||
| // Regular grid items | ||
| for (let i = 0; i < 3; i++) { | ||
| const item = document.createElement('div'); | ||
| item.textContent = `${i + 1}`; | ||
| item.style.backgroundColor = `hsl(${160 + i * 20}, 60%, 50%)`; | ||
| item.style.display = 'flex'; | ||
| item.style.alignItems = 'center'; | ||
| item.style.justifyContent = 'center'; | ||
| item.style.color = 'white'; | ||
| grid.appendChild(item); | ||
| } | ||
|
|
||
| // Sticky positioned item (behaves like relative in grid) | ||
| const stickyItem = document.createElement('div'); | ||
| stickyItem.textContent = 'Sticky'; | ||
| stickyItem.style.position = 'sticky'; | ||
| stickyItem.style.top = '0'; | ||
| stickyItem.style.backgroundColor = '#009688'; | ||
| stickyItem.style.display = 'flex'; | ||
| stickyItem.style.alignItems = 'center'; | ||
| stickyItem.style.justifyContent = 'center'; | ||
| stickyItem.style.color = 'white'; | ||
| stickyItem.style.fontSize = '11px'; | ||
| grid.appendChild(stickyItem); | ||
|
|
||
| document.body.appendChild(grid); | ||
| await waitForFrame(); | ||
| await snapshot(); | ||
|
|
||
| // Sticky item takes up grid cell | ||
| const items = Array.from(grid.children) as HTMLElement[]; | ||
| expect(items.length).toBe(4); | ||
|
|
||
| grid.remove(); | ||
| }); |
There was a problem hiding this comment.
Fix misleading test name.
The test is titled "handles fixed positioning within grid" but actually tests sticky positioning. The title should accurately reflect what's being tested.
🔎 Proposed fix
- xit('handles fixed positioning within grid', async () => {
+ xit('handles sticky positioning within grid', async () => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| xit('handles fixed positioning within grid', async () => { | |
| const grid = document.createElement('div'); | |
| grid.style.display = 'grid'; | |
| grid.style.gridTemplateColumns = 'repeat(2, 130px)'; | |
| grid.style.gridTemplateRows = 'repeat(2, 90px)'; | |
| grid.style.gap = '10px'; | |
| grid.style.backgroundColor = '#e0f2f1'; | |
| // Regular grid items | |
| for (let i = 0; i < 3; i++) { | |
| const item = document.createElement('div'); | |
| item.textContent = `${i + 1}`; | |
| item.style.backgroundColor = `hsl(${160 + i * 20}, 60%, 50%)`; | |
| item.style.display = 'flex'; | |
| item.style.alignItems = 'center'; | |
| item.style.justifyContent = 'center'; | |
| item.style.color = 'white'; | |
| grid.appendChild(item); | |
| } | |
| // Sticky positioned item (behaves like relative in grid) | |
| const stickyItem = document.createElement('div'); | |
| stickyItem.textContent = 'Sticky'; | |
| stickyItem.style.position = 'sticky'; | |
| stickyItem.style.top = '0'; | |
| stickyItem.style.backgroundColor = '#009688'; | |
| stickyItem.style.display = 'flex'; | |
| stickyItem.style.alignItems = 'center'; | |
| stickyItem.style.justifyContent = 'center'; | |
| stickyItem.style.color = 'white'; | |
| stickyItem.style.fontSize = '11px'; | |
| grid.appendChild(stickyItem); | |
| document.body.appendChild(grid); | |
| await waitForFrame(); | |
| await snapshot(); | |
| // Sticky item takes up grid cell | |
| const items = Array.from(grid.children) as HTMLElement[]; | |
| expect(items.length).toBe(4); | |
| grid.remove(); | |
| }); | |
| xit('handles sticky positioning within grid', async () => { | |
| const grid = document.createElement('div'); | |
| grid.style.display = 'grid'; | |
| grid.style.gridTemplateColumns = 'repeat(2, 130px)'; | |
| grid.style.gridTemplateRows = 'repeat(2, 90px)'; | |
| grid.style.gap = '10px'; | |
| grid.style.backgroundColor = '#e0f2f1'; | |
| // Regular grid items | |
| for (let i = 0; i < 3; i++) { | |
| const item = document.createElement('div'); | |
| item.textContent = `${i + 1}`; | |
| item.style.backgroundColor = `hsl(${160 + i * 20}, 60%, 50%)`; | |
| item.style.display = 'flex'; | |
| item.style.alignItems = 'center'; | |
| item.style.justifyContent = 'center'; | |
| item.style.color = 'white'; | |
| grid.appendChild(item); | |
| } | |
| // Sticky positioned item (behaves like relative in grid) | |
| const stickyItem = document.createElement('div'); | |
| stickyItem.textContent = 'Sticky'; | |
| stickyItem.style.position = 'sticky'; | |
| stickyItem.style.top = '0'; | |
| stickyItem.style.backgroundColor = '#009688'; | |
| stickyItem.style.display = 'flex'; | |
| stickyItem.style.alignItems = 'center'; | |
| stickyItem.style.justifyContent = 'center'; | |
| stickyItem.style.color = 'white'; | |
| stickyItem.style.fontSize = '11px'; | |
| grid.appendChild(stickyItem); | |
| document.body.appendChild(grid); | |
| await waitForFrame(); | |
| await snapshot(); | |
| // Sticky item takes up grid cell | |
| const items = Array.from(grid.children) as HTMLElement[]; | |
| expect(items.length).toBe(4); | |
| grid.remove(); | |
| }); |
🤖 Prompt for AI Agents
In
integration_tests/specs/css/css-grid/absolute-positioning/mixed-positioning.ts
around lines 322 to 364, the test name "handles fixed positioning within grid"
is misleading because the test actually sets position: sticky; rename the test
description string to accurately reflect sticky positioning (for example
"handles sticky positioning within grid") so the title matches the behavior
being tested; keep the rest of the test unchanged.
| it('handles fit-content with percentage margins', async () => { | ||
| const grid = document.createElement('div'); | ||
| grid.style.display = 'grid'; | ||
| grid.style.gridTemplateColumns = 'fit-content(300px)'; | ||
| grid.style.gridTemplateRows = 'auto'; | ||
| grid.style.width = '400px'; | ||
| grid.style.backgroundColor = '#e8f5e9'; | ||
|
|
||
| const item = document.createElement('div'); | ||
| item.textContent = 'Content with percentage margin'; | ||
| item.style.marginLeft = '10%'; // Depends on containing block | ||
| item.style.marginRight = '10%'; | ||
| item.style.backgroundColor = '#66BB6A'; | ||
| item.style.padding = '10px'; | ||
| item.style.color = 'white'; | ||
| grid.appendChild(item); | ||
|
|
||
| document.body.appendChild(grid); | ||
| await waitForFrame(); | ||
| await snapshot(); | ||
|
|
||
| expect(item.getBoundingClientRect().width).toBeGreaterThan(0); | ||
|
|
||
| grid.remove(); | ||
| }); |
There was a problem hiding this comment.
Grid width exceeds viewport constraint.
The grid width is set to 400px, which exceeds the 340px viewport limit specified in the coding guidelines. This may cause horizontal overflow and inconsistent snapshot rendering.
🔎 Proposed fix to respect viewport constraints
- grid.style.width = '400px';
+ grid.style.width = '320px';
grid.style.gridTemplateColumns = 'fit-content(300px)';You may also want to reduce the fit-content max from 300px proportionally to ensure adequate testing margin.
As per coding guidelines, the test window has a maximum width of 340px.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
integration_tests/specs/css/css-grid/layout-algorithm/circular-dependencies.ts
around lines 151 to 175, the test sets grid.style.width = '400px' which exceeds
the 340px viewport constraint; change the grid width to at most '340px'
(preferably a bit smaller like '320px') and reduce the fit-content max (e.g.,
from 300px to ~260px) proportionally to avoid horizontal overflow and
inconsistent snapshots, then re-run the test/snapshot to confirm layout remains
valid.
| it('resolves maximum sizing constraints', async () => { | ||
| const grid = document.createElement('div'); | ||
| grid.style.display = 'grid'; | ||
| grid.style.width = '500px'; | ||
| grid.style.gridTemplateColumns = 'minmax(50px, 150px) minmax(50px, 200px) 1fr'; | ||
| grid.style.gridTemplateRows = 'auto'; | ||
| grid.style.gap = '10px'; | ||
| grid.style.backgroundColor = '#e8f5e9'; | ||
|
|
||
| for (let i = 0; i < 3; i++) { | ||
| const item = document.createElement('div'); | ||
| item.textContent = `Item ${i + 1}`; | ||
| item.style.backgroundColor = `hsl(${120 + i * 15}, 60%, 55%)`; | ||
| item.style.padding = '10px'; | ||
| item.style.color = 'white'; | ||
| grid.appendChild(item); | ||
| } | ||
|
|
||
| document.body.appendChild(grid); | ||
| await waitForFrame(); | ||
| await snapshot(); | ||
|
|
||
| const items = Array.from(grid.children) as HTMLElement[]; | ||
| // Should respect maximum sizes | ||
| expect(items[0].getBoundingClientRect().width).toBeLessThanOrEqual(150); | ||
| expect(items[1].getBoundingClientRect().width).toBeLessThanOrEqual(200); | ||
|
|
||
| grid.remove(); | ||
| }); |
There was a problem hiding this comment.
Grid width exceeds viewport constraint.
The grid is set to 500px width, but the coding guidelines specify that "The max width of testing window is 340px." This could cause horizontal overflow, clipping, and unreliable snapshot rendering.
🔎 Proposed fix to respect viewport constraints
- grid.style.width = '500px';
+ grid.style.width = '300px';
grid.style.gridTemplateColumns = 'minmax(50px, 150px) minmax(50px, 200px) 1fr';The test will still validate maximum sizing constraints with a smaller container width.
As per coding guidelines, the test window has a maximum width of 340px.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('resolves maximum sizing constraints', async () => { | |
| const grid = document.createElement('div'); | |
| grid.style.display = 'grid'; | |
| grid.style.width = '500px'; | |
| grid.style.gridTemplateColumns = 'minmax(50px, 150px) minmax(50px, 200px) 1fr'; | |
| grid.style.gridTemplateRows = 'auto'; | |
| grid.style.gap = '10px'; | |
| grid.style.backgroundColor = '#e8f5e9'; | |
| for (let i = 0; i < 3; i++) { | |
| const item = document.createElement('div'); | |
| item.textContent = `Item ${i + 1}`; | |
| item.style.backgroundColor = `hsl(${120 + i * 15}, 60%, 55%)`; | |
| item.style.padding = '10px'; | |
| item.style.color = 'white'; | |
| grid.appendChild(item); | |
| } | |
| document.body.appendChild(grid); | |
| await waitForFrame(); | |
| await snapshot(); | |
| const items = Array.from(grid.children) as HTMLElement[]; | |
| // Should respect maximum sizes | |
| expect(items[0].getBoundingClientRect().width).toBeLessThanOrEqual(150); | |
| expect(items[1].getBoundingClientRect().width).toBeLessThanOrEqual(200); | |
| grid.remove(); | |
| }); | |
| it('resolves maximum sizing constraints', async () => { | |
| const grid = document.createElement('div'); | |
| grid.style.display = 'grid'; | |
| grid.style.width = '300px'; | |
| grid.style.gridTemplateColumns = 'minmax(50px, 150px) minmax(50px, 200px) 1fr'; | |
| grid.style.gridTemplateRows = 'auto'; | |
| grid.style.gap = '10px'; | |
| grid.style.backgroundColor = '#e8f5e9'; | |
| for (let i = 0; i < 3; i++) { | |
| const item = document.createElement('div'); | |
| item.textContent = `Item ${i + 1}`; | |
| item.style.backgroundColor = `hsl(${120 + i * 15}, 60%, 55%)`; | |
| item.style.padding = '10px'; | |
| item.style.color = 'white'; | |
| grid.appendChild(item); | |
| } | |
| document.body.appendChild(grid); | |
| await waitForFrame(); | |
| await snapshot(); | |
| const items = Array.from(grid.children) as HTMLElement[]; | |
| // Should respect maximum sizes | |
| expect(items[0].getBoundingClientRect().width).toBeLessThanOrEqual(150); | |
| expect(items[1].getBoundingClientRect().width).toBeLessThanOrEqual(200); | |
| grid.remove(); | |
| }); |
🤖 Prompt for AI Agents
In integration_tests/specs/css/css-grid/layout-algorithm/sizing-resolution.ts
around lines 148 to 176, the test sets grid.style.width = '500px' which exceeds
the test window max width (340px); change the fixed width to respect the
viewport (e.g., grid.style.width = '340px' or use a responsive max-width) so the
grid fits the testing window and snapshots aren't clipped; no other logic
changes required, keep the existing assertions validating maximum sizing.
| it('resolves sizing with auto and fr mixed', async () => { | ||
| const grid = document.createElement('div'); | ||
| grid.style.display = 'grid'; | ||
| grid.style.width = '400px'; | ||
| grid.style.gridTemplateColumns = 'auto 1fr 2fr'; | ||
| grid.style.gridTemplateRows = 'auto'; | ||
| grid.style.gap = '10px'; | ||
| grid.style.backgroundColor = '#ede7f6'; | ||
|
|
||
| const item1 = document.createElement('div'); | ||
| item1.textContent = 'Auto fits content'; | ||
| item1.style.backgroundColor = '#9575CD'; | ||
| item1.style.padding = '10px'; | ||
| item1.style.color = 'white'; | ||
| grid.appendChild(item1); | ||
|
|
||
| const item2 = document.createElement('div'); | ||
| item2.textContent = '1fr'; | ||
| item2.style.backgroundColor = '#7E57C2'; | ||
| item2.style.padding = '10px'; | ||
| item2.style.color = 'white'; | ||
| grid.appendChild(item2); | ||
|
|
||
| const item3 = document.createElement('div'); | ||
| item3.textContent = '2fr'; | ||
| item3.style.backgroundColor = '#673AB7'; | ||
| item3.style.padding = '10px'; | ||
| item3.style.color = 'white'; | ||
| grid.appendChild(item3); | ||
|
|
||
| document.body.appendChild(grid); | ||
| await waitForFrame(); | ||
| await snapshot(); | ||
|
|
||
| const items = Array.from(grid.children) as HTMLElement[]; | ||
| // Auto should size to content, fr should distribute remaining space | ||
| expect(items[0].getBoundingClientRect().width).toBeGreaterThan(0); | ||
| // 2fr should be roughly twice 1fr | ||
| const ratio = items[2].getBoundingClientRect().width / items[1].getBoundingClientRect().width; | ||
| expect(Math.abs(ratio - 2)).toBeLessThan(0.1); | ||
|
|
||
| grid.remove(); | ||
| }); |
There was a problem hiding this comment.
Grid width exceeds viewport constraint.
The grid width is set to 400px, which exceeds the 340px viewport limit specified in the coding guidelines. This could result in horizontal overflow and unreliable snapshot results.
🔎 Proposed fix to fit within viewport
- grid.style.width = '400px';
+ grid.style.width = '320px';
grid.style.gridTemplateColumns = 'auto 1fr 2fr';The test will still validate the fr unit ratio with a smaller container.
As per coding guidelines, the test window has a maximum width of 340px.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('resolves sizing with auto and fr mixed', async () => { | |
| const grid = document.createElement('div'); | |
| grid.style.display = 'grid'; | |
| grid.style.width = '400px'; | |
| grid.style.gridTemplateColumns = 'auto 1fr 2fr'; | |
| grid.style.gridTemplateRows = 'auto'; | |
| grid.style.gap = '10px'; | |
| grid.style.backgroundColor = '#ede7f6'; | |
| const item1 = document.createElement('div'); | |
| item1.textContent = 'Auto fits content'; | |
| item1.style.backgroundColor = '#9575CD'; | |
| item1.style.padding = '10px'; | |
| item1.style.color = 'white'; | |
| grid.appendChild(item1); | |
| const item2 = document.createElement('div'); | |
| item2.textContent = '1fr'; | |
| item2.style.backgroundColor = '#7E57C2'; | |
| item2.style.padding = '10px'; | |
| item2.style.color = 'white'; | |
| grid.appendChild(item2); | |
| const item3 = document.createElement('div'); | |
| item3.textContent = '2fr'; | |
| item3.style.backgroundColor = '#673AB7'; | |
| item3.style.padding = '10px'; | |
| item3.style.color = 'white'; | |
| grid.appendChild(item3); | |
| document.body.appendChild(grid); | |
| await waitForFrame(); | |
| await snapshot(); | |
| const items = Array.from(grid.children) as HTMLElement[]; | |
| // Auto should size to content, fr should distribute remaining space | |
| expect(items[0].getBoundingClientRect().width).toBeGreaterThan(0); | |
| // 2fr should be roughly twice 1fr | |
| const ratio = items[2].getBoundingClientRect().width / items[1].getBoundingClientRect().width; | |
| expect(Math.abs(ratio - 2)).toBeLessThan(0.1); | |
| grid.remove(); | |
| }); | |
| it('resolves sizing with auto and fr mixed', async () => { | |
| const grid = document.createElement('div'); | |
| grid.style.display = 'grid'; | |
| grid.style.width = '320px'; | |
| grid.style.gridTemplateColumns = 'auto 1fr 2fr'; | |
| grid.style.gridTemplateRows = 'auto'; | |
| grid.style.gap = '10px'; | |
| grid.style.backgroundColor = '#ede7f6'; | |
| const item1 = document.createElement('div'); | |
| item1.textContent = 'Auto fits content'; | |
| item1.style.backgroundColor = '#9575CD'; | |
| item1.style.padding = '10px'; | |
| item1.style.color = 'white'; | |
| grid.appendChild(item1); | |
| const item2 = document.createElement('div'); | |
| item2.textContent = '1fr'; | |
| item2.style.backgroundColor = '#7E57C2'; | |
| item2.style.padding = '10px'; | |
| item2.style.color = 'white'; | |
| grid.appendChild(item2); | |
| const item3 = document.createElement('div'); | |
| item3.textContent = '2fr'; | |
| item3.style.backgroundColor = '#673AB7'; | |
| item3.style.padding = '10px'; | |
| item3.style.color = 'white'; | |
| grid.appendChild(item3); | |
| document.body.appendChild(grid); | |
| await waitForFrame(); | |
| await snapshot(); | |
| const items = Array.from(grid.children) as HTMLElement[]; | |
| // Auto should size to content, fr should distribute remaining space | |
| expect(items[0].getBoundingClientRect().width).toBeGreaterThan(0); | |
| // 2fr should be roughly twice 1fr | |
| const ratio = items[2].getBoundingClientRect().width / items[1].getBoundingClientRect().width; | |
| expect(Math.abs(ratio - 2)).toBeLessThan(0.1); | |
| grid.remove(); | |
| }); |
🤖 Prompt for AI Agents
integration_tests/specs/css/css-grid/layout-algorithm/sizing-resolution.ts lines
178-220: the test sets grid.style.width = '400px' which exceeds the 340px
viewport limit; change the width to a value within the viewport (e.g. '320px' or
'340px') so the grid fits without horizontal overflow, keep the rest of the test
the same to still validate auto vs fr sizing and the 2fr:1fr ratio.
…d fix compute grid property row-gap/column-gap/gap
…fix gap/grid short hand
… honor flex min-size:auto for grid items
… & snapshots, and support grid calc/percent resolution
…d support scroll
andycall
force-pushed
the
feat/css_grid_integration_tests_3
branch
from
69548f7 to
f9da526
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
webf/lib/src/rendering/grid.dart (1)
3604-3725: Relayout-for-stretched-tracks should also skip positioned grid childrenIn the
relayoutForStretchedTracksblock you re-layout every child as a grid item, but earlier passes deliberately skip_isPositionedGridChild(absolute/fixed/sticky) when:
- Placing items (
placementChildren),- Doing main layout,
- Row-baseline alignment,
- Alignment distribution, etc.
Re-running positioned children through
_gridItemConstraintsand the grid placement math here is inconsistent with that model and risks:
- Unnecessary layout work, and
- Overwriting
GridLayoutParentData.offsetfor out-of-flow children just beforeCSSPositionedLayoutapplies their absolute/sticky offsets.You probably want to mirror the earlier guards:
Proposed guard in relayout pass
- while (childForRelayout != null) { - CSSRenderStyle? childGridStyle; + while (childForRelayout != null) { + final GridLayoutParentData pd = childForRelayout.parentData as GridLayoutParentData; + if (_isPositionedGridChild(childForRelayout)) { + childForRelayout = pd.nextSibling; + continue; + } + + CSSRenderStyle? childGridStyle; if (childForRelayout is RenderBoxModel) { childGridStyle = childForRelayout.renderStyle; } else if (childForRelayout is RenderEventListener) { final RenderBox? wrapped = childForRelayout.child; if (wrapped is RenderBoxModel) { childGridStyle = wrapped.renderStyle; } } @@ - final GridLayoutParentData pd = childForRelayout.parentData as GridLayoutParentData; + // pd already obtained above
♻️ Duplicate comments (2)
CSS_GRID_PLAN.md (1)
210-225: Fix markdown table formatting to comply with standards.The WPT Test Distribution table (line 211) lacks required blank lines before and after per markdown standards (MD058). This issue was previously flagged in review comments but remains unresolved.
🔎 Proposed fix
**Total Completed: 77 files, 529 integration tests** ✅ +#### Updated WPT Test Distribution + | Category | Test Count | Percentage | WebF Coverage | |----------|-----------|------------|---------------| | Grid Lanes (Track Sizing) | 789 | 33% | ✅ **Good** (89 tests) | | Alignment | 476 | 20% | ✅ **Good** (90 tests) | | Abspos | 303 | 13% | ✅ **Good** (35 tests) | | Grid Items | 205 | 9% | ✅ **Good** (106 tests) | | Subgrid | 175 | 7% | ❌ Out of scope | | Grid Definition | 82 | 3% | ✅ **Excellent** (89 tests) | | Grid Model | 81 | 3% | ✅ **Good** (48 tests) | | Parsing | 61 | 3% | ✅ **Good** (48 tests) | | Layout Algorithm | 61 | 3% | ✅ **Good** (35 tests) | | Placement | 20 | 1% | ✅ **Excellent** (106 tests) | | Animation | 17 | 1% | ✅ **Good** (12 tests) | | Implicit Grids | 3 | <1% | ✅ **Excellent** (56 tests) | + **Updated Findings:**integration_tests/specs/css/css-grid/layout-algorithm/circular-dependencies.ts (1)
151-175: Grid width exceeds viewport constraint.The grid width is set to 400px on line 156, which exceeds the 340px viewport limit specified in the coding guidelines. This may cause horizontal overflow and inconsistent snapshot rendering.
As per coding guidelines, the test window has a maximum width of 340px. Consider reducing the width to 320px and adjusting the fit-content max proportionally (e.g., from 300px to 260px).
🧹 Nitpick comments (10)
integration_tests/specs/css/css-grid/layout-algorithm/circular-dependencies.ts (1)
2-42: Why is this test skipped?The test is disabled with
xit. If this is a known issue or limitation, consider adding a comment explaining why. If it's ready to enable, changexittoit.Do you want me to help investigate why this test might be failing or open an issue to track enabling it?
Note on static analysis warning: The innerHTML assignment on lines 10-11 is flagged as a potential XSS risk, but this is a false positive for test code with hardcoded content.
webf/test/src/rendering/css_grid_basic_layout_test.dart (1)
126-154: Consider tightening the height bounds if the expected value is known.The test uses relaxed bounds (
lessThanOrEqualTo(100)) with a comment noting "Give more headroom." While this may accommodate rendering variance, it could also mask unexpected behavior.Given the nested structure with 10px padding at three levels, can you calculate a more precise expected height? If the exact value is deterministic, using
closeTo(expectedHeight, tolerance)would make the test more robust.If the relaxed bounds are intentional due to font metrics or platform differences, consider documenting why the range is [80, 100].
integration_tests/specs/css/css-grid/grid-definition/repeat-notation.ts (1)
59-69: Remove commented-out debug code.The commented-out console.log statements and extra snapshot call are debugging artifacts that should be removed before merging to keep the test file clean and maintainable.
🔎 Proposed cleanup
document.body.appendChild(grid); await waitForFrame(); - // Debug: ensure line names in repeat + placement parse correctly. - // eslint-disable-next-line no-console - // console.log('[grid][named-lines-repeat] templateCols:', getComputedStyle(grid).gridTemplateColumns); - // // eslint-disable-next-line no-console - // console.log('[grid][named-lines-repeat] item gridColumnStart/end:', getComputedStyle(item).gridColumnStart, getComputedStyle(item).gridColumnEnd); - // // eslint-disable-next-line no-console - // console.log('[grid][named-lines-repeat] item style gridColumn:', item.style.gridColumn); - // // eslint-disable-next-line no-console - // console.log('[grid][named-lines-repeat] item style gridColumnStart/end:', item.style.gridColumnStart, item.style.gridColumnEnd); await snapshot(); - // await snapshot(1);integration_tests/specs/css/css-grid/grid-definition/explicit-tracks.ts (1)
245-255: Remove commented-out debug code.Similar to the other file, these debugging artifacts should be removed to maintain a clean test suite.
🔎 Proposed cleanup
document.body.appendChild(grid); await waitForFrame(); - // Debug: ensure line names + placement parse correctly. - // eslint-disable-next-line no-console - // console.log('[grid][duplicate-line-names] templateCols:', getComputedStyle(grid).gridTemplateColumns); - // // eslint-disable-next-line no-console - // console.log('[grid][duplicate-line-names] item gridColumnStart/end:', getComputedStyle(item).gridColumnStart, getComputedStyle(item).gridColumnEnd); - // // eslint-disable-next-line no-console - // console.log('[grid][duplicate-line-names] item style gridColumn:', item.style.gridColumn); - // // eslint-disable-next-line no-console - // console.log('[grid][duplicate-line-names] item style gridColumnStart/end:', item.style.gridColumnStart, item.style.gridColumnEnd); await snapshot(); - // await snapshot(1);integration_tests/specs/css/css-grid/grid-model/grid-items-types.ts (1)
63-88: Consider testing with actual image sources.The test validates images as grid items but omits the
srcattribute. For more realistic coverage, consider referencing image assets from theassets/directory to test actual image loading behavior, aspect ratios, andobject-fitrendering.Based on coding guidelines, test assets should reference files in the
assets/directory.integration_tests/specs/css/css-grid/grid-model/display-grid.ts (1)
181-235: Good display: contents edge case coverage.The test properly validates that a
display: contentswrapper allows its children to participate directly as grid items.The hardcoded pixel positions on lines 231-232 (110, 220) assume no body margin/padding and exact grid positioning. While this is a common pattern in layout tests, consider calculating expected positions based on grid configuration (e.g.,
100px column + 10px gap = 110px offset) to make the test more maintainable if defaults change.bridge/core/css/style_engine.h (1)
298-303: Document the StyleSheetContents deduplication strategy.The switch from tracking
CSSStyleSheet*toStyleSheetContentswith use-counting is a significant architectural change. Consider adding a brief comment explaining why raw pointers are used as map keys despite holdingshared_ptrvalues in the vector, to help future maintainers understand the safety guarantees.🔎 Suggested documentation comment
+ // Active author stylesheets registered by link/style processing. We track + // the shared StyleSheetContents used for rule matching / invalidation. + // Deduplication: Multiple CSSStyleSheet wrappers may share the same + // StyleSheetContents, so we maintain a use-count map keyed by raw pointer. + // This is safe because the shared_ptr instances in author_sheets_ keep the + // contents alive, and map entries are removed atomically when count reaches zero. std::vector<std::shared_ptr<StyleSheetContents>> author_sheets_;webf/test/src/css/inset_shorthand_test.dart (1)
4-78: Good coverage for inset shorthand expansion/removalThe tests exercise all key patterns (1–4 values, calc(), removal back to AUTO, and invalid shorthand rollback) directly against
CSSStyleDeclaration. This is a solid behavioral net around the new inset shorthand support. If you want to push coverage further later, you could add cases with negative values and mixedauto/length tokens, but not blocking for this change.webf/lib/src/rendering/grid.dart (1)
3341-3371: Row-gap percentage resolved late: multi-row alignment may ignore gap heightFor cyclic
%row-gap you:
- Lay out children with
rowGap == 0.0,- Later solve for the actual
rowGapfromusedContentHeightWithoutGaps, and- Translate child offsets by
rowGap * rowStart.This correctly spaces rows, but
cellHeightand related alignment calculations (e.g.,align-selffor items spanning multiple rows) were computed beforerowGapwas known, so the vertical alignment area for a multi-row grid area can be too small byrowGap * (rowSpan - 1).Not a blocker, but if you want full spec fidelity for exotic
%row-gaps with spanning items, consider also recomputing the effective cell height (or re-running the vertical alignment branch) after the finalrowGapis known, instead of only translating offsets.integration_tests/specs/css/css-grid/absolute-positioning/grid-area-containing-block.ts (1)
150-205: Remove debug console.log statements from skipped test.The skipped test contains multiple debug
console.logstatements (lines 187, 189, 195-196, 199, 202) that should be removed. If the test is intentionally skipped due to a known issue, consider adding a TODO comment explaining the reason instead of leaving debug artifacts.🔎 Proposed fix
// Item spans 2x2: 100 + 10 + 100 = 210px width, 70 + 10 + 70 = 150px height expect(item.getBoundingClientRect().width).toBe(210); - console.log('-----------------------------0') expect(item.getBoundingClientRect().height).toBe(150); - console.log('-----------------------------1') // Absolute child centered in item const itemCenterX = item.getBoundingClientRect().left + 105; const itemCenterY = item.getBoundingClientRect().top + 75; const childCenterX = absChild.getBoundingClientRect().left + 40; const childCenterY = absChild.getBoundingClientRect().top + 30; - console.log("itemCenterX:", itemCenterX, "itemCenterY:", itemCenterY); - console.log("childCenterX:", childCenterX, "childCenterY:", childCenterY); expect(Math.round(childCenterX)).toBe(Math.round(itemCenterX)); - console.log('-----------------------------2') expect(Math.round(childCenterY)).toBe(Math.round(itemCenterY)); - console.log('-----------------------------3') grid.remove();
| it('absolute child with percentage sizing in grid item', async () => { | ||
| const grid = document.createElement('div'); | ||
| grid.style.display = 'grid'; | ||
| grid.style.gridTemplateColumns = 'repeat(2, 200px)'; | ||
| grid.style.gridTemplateRows = 'repeat(2, 120px)'; | ||
| grid.style.gap = '10px'; | ||
| grid.style.backgroundColor = '#ede7f6'; | ||
|
|
There was a problem hiding this comment.
Grid width exceeds 340px testing window limit.
The grid configuration creates a total width of 410px (2 × 200px columns + 10px gap), which exceeds the maximum testing window width of 340px. This may cause visual regression test issues or clipping. As per the coding guidelines, the max width of the testing window is 340px.
🔎 Proposed fix to reduce column widths
const grid = document.createElement('div');
grid.style.display = 'grid';
- grid.style.gridTemplateColumns = 'repeat(2, 200px)';
- grid.style.gridTemplateRows = 'repeat(2, 120px)';
+ grid.style.gridTemplateColumns = 'repeat(2, 160px)';
+ grid.style.gridTemplateRows = 'repeat(2, 100px)';
grid.style.gap = '10px';
grid.style.backgroundColor = '#ede7f6';Then update the corresponding assertions (lines 286-295) to match the new dimensions:
- // Item is 200x120
- expect(item.getBoundingClientRect().width).toBe(200);
- expect(item.getBoundingClientRect().height).toBe(120);
+ // Item is 160x100
+ expect(item.getBoundingClientRect().width).toBe(160);
+ expect(item.getBoundingClientRect().height).toBe(100);
- // Absolute child: 80% of 200 = 160px, 80% of 120 = 96px
- expect(absChild.getBoundingClientRect().width).toBe(160);
- expect(absChild.getBoundingClientRect().height).toBe(96);
+ // Absolute child: 80% of 160 = 128px, 80% of 100 = 80px
+ expect(absChild.getBoundingClientRect().width).toBe(128);
+ expect(absChild.getBoundingClientRect().height).toBe(80);
- // Positioned at 10%: 20px left, 12px top
- expect(absChild.getBoundingClientRect().left).toBe(item.getBoundingClientRect().left + 20);
- expect(absChild.getBoundingClientRect().top).toBe(item.getBoundingClientRect().top + 12);
+ // Positioned at 10%: 16px left, 10px top
+ expect(absChild.getBoundingClientRect().left).toBe(item.getBoundingClientRect().left + 16);
+ expect(absChild.getBoundingClientRect().top).toBe(item.getBoundingClientRect().top + 10);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
integration_tests/specs/css/css-grid/absolute-positioning/grid-area-containing-block.ts
around lines 251-258, the grid total width (2×200px + 10px gap = 410px) exceeds
the 340px test window; change gridTemplateColumns to use smaller columns so
total width ≤ 340px — e.g., set grid.style.gridTemplateColumns = 'repeat(2,
165px)' (165px×2 + 10px gap = 340px) — and then update the related assertions on
lines 286-295 to match the new column widths and resulting element
positions/sizes.
| grid.remove(); | ||
| }); | ||
|
|
||
| // 异常 |
There was a problem hiding this comment.
Clarify or remove the non-English comment.
The Chinese comment "// 异常" (abnormal/exception) suggests an issue with this test but provides no context for English-speaking developers. If there's a known problem, document it clearly in English or create a tracking issue. Otherwise, remove the comment.
🔎 Suggested fix
- // 异常
it('repeats with named lines', async () => {Or if there's a genuine issue to track:
- // 异常
+ // TODO: Investigate named line placement behavior - see issue #XXX
it('repeats with named lines', async () => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // 异常 | |
| it('repeats with named lines', async () => { |
🤖 Prompt for AI Agents
In integration_tests/specs/css/css-grid/grid-definition/repeat-notation.ts
around line 37, replace the Chinese comment "// 异常" with a clear English comment
or remove it: if this test documents a known failure, change the comment to a
concise English explanation (e.g., "// Known issue: [brief description] — see
ISSUE-123" and link or create a tracking issue), otherwise simply delete the
comment so no non-English residue remains.
| // Out-of-flow positioned children (absolute/fixed/sticky) do not participate in | ||
| // grid item placement or track sizing. They are laid out and positioned after | ||
| // the grid container size and placeholder static positions are known. | ||
| final List<RenderBoxModel> positionedChildren = <RenderBoxModel>[]; | ||
| final List<RenderBoxModel> stickyChildren = <RenderBoxModel>[]; | ||
| final List<RenderBoxModel> absFixedChildren = <RenderBoxModel>[]; | ||
| RenderBox? positionedScan = firstChild; | ||
| while (positionedScan != null) { | ||
| final GridLayoutParentData pd = positionedScan.parentData as GridLayoutParentData; | ||
| if (positionedScan is RenderBoxModel) { | ||
| final RenderStyle? style = _unwrapGridChildStyle(positionedScan); | ||
| if (style != null && (style.isSelfPositioned() || style.isSelfStickyPosition())) { | ||
| positionedChildren.add(positionedScan); | ||
| if (style.isSelfStickyPosition()) { | ||
| stickyChildren.add(positionedScan); | ||
| } else { | ||
| absFixedChildren.add(positionedScan); | ||
| } | ||
| } | ||
| } | ||
| positionedScan = pd.nextSibling; | ||
| } | ||
|
|
||
| // Pre-layout sticky positioned children so their placeholders can reserve | ||
| // correct space during the subsequent grid layout pass. | ||
| for (final RenderBoxModel sticky in stickyChildren) { | ||
| CSSPositionedLayout.layoutPositionedChild(this, sticky); | ||
| } | ||
|
|
There was a problem hiding this comment.
Out-of-flow grid children wrapped in RenderEventListener are never collected
In the positioned scan you only add positionedScan to positionedChildren/stickyChildren/absFixedChildren when it is a RenderBoxModel. But earlier and elsewhere you rely on _unwrapGridChildStyle/_unwrapGridChildBoxModel because children are often wrapped in RenderEventListener.
Combined with:
final List<RenderBox> placementChildren = _collectChildren()
.where((RenderBox child) => !_isPositionedGridChild(child))
.toList(growable: false);positioned children inside RenderEventListener are:
- Classified as positioned by
_isPositionedGridChild(since it unwraps), so they are excluded from grid placement/layout, but - Never added to
positionedChildren/absFixedChildren/stickyChildren, so they are never laid out byCSSPositionedLayoutor have offsets applied.
This can make wrapped absolute/fixed/sticky grid children effectively disappear from layout.
I suggest unwrapping here as you do elsewhere and storing the underlying RenderBoxModel:
Proposed fix for positioned children collection
- RenderBox? positionedScan = firstChild;
- while (positionedScan != null) {
- final GridLayoutParentData pd = positionedScan.parentData as GridLayoutParentData;
- if (positionedScan is RenderBoxModel) {
- final RenderStyle? style = _unwrapGridChildStyle(positionedScan);
- if (style != null && (style.isSelfPositioned() || style.isSelfStickyPosition())) {
- positionedChildren.add(positionedScan);
- if (style.isSelfStickyPosition()) {
- stickyChildren.add(positionedScan);
- } else {
- absFixedChildren.add(positionedScan);
- }
- }
- }
- positionedScan = pd.nextSibling;
- }
+ RenderBox? positionedScan = firstChild;
+ while (positionedScan != null) {
+ final GridLayoutParentData pd = positionedScan.parentData as GridLayoutParentData;
+ final RenderBoxModel? box = _unwrapGridChildBoxModel(positionedScan);
+ if (box != null) {
+ final RenderStyle? style = box.renderStyle;
+ if (style != null && (style.isSelfPositioned() || style.isSelfStickyPosition())) {
+ positionedChildren.add(box);
+ if (style.isSelfStickyPosition()) {
+ stickyChildren.add(box);
+ } else {
+ absFixedChildren.add(box);
+ }
+ }
+ }
+ positionedScan = pd.nextSibling;
+ }
Summary:
Summary by CodeRabbit
New Features
Improvements
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.