fix(slides): improve text width overflow detection#2056
Conversation
Text-shape overflow was always reported as a warning, which let clearly broken pages pass the lint gate. Overflow > 10px now upgrades to error; smaller overflows stay as warning to avoid flagging near-fit cases.
Extract letterSpacing from content/paragraph attrs and factor it into width and line-count estimates, and stop short-circuiting the shape overflow check for autoFit shapes so that letterSpacing-heavy captions under normal-auto-fit no longer escape detection.
Large low-alpha text underneath other text shapes is typically a background design element; treat text_may_overflow_shape as info in that case instead of warning/error.
chartParsedValues is a server-injected roundtrip child tag under chartField, not an attribute. Move it from ROUNDTRIP_SXSD_ATTRS to a new ROUNDTRIP_SXSD_TAGS set and skip the tag (and its subtree) in the SXSD tag whitelist check.
📝 WalkthroughWalkthroughText overflow linting now accounts for paragraph and shape letter spacing, font attributes, padding, and decorative background text. Canvas validation now uses restricted element types and selective rotation bounds. Lint results preserve informational issues with document- and slide-level counts and lists. ChangesLint behavior
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant XMLContent
participant TextEstimator
participant OverflowDetector
participant LintResult
XMLContent->>TextEstimator: paragraph and shape text attributes
TextEstimator->>OverflowDetector: estimated widths and line counts
OverflowDetector->>LintResult: severity and diagnostic fields
LintResult->>LintResult: aggregate errors, warnings, and infos
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
the
size/L
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
skills/lark-slides/scripts/xml_text_overlap_lint.py (1)
910-932: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdvice is contradictory now that
autoFit="normal-auto-fit"shapes are no longer skipped.Removing the auto-fit early-continue means shapes that already declare
wrap="true" autoFit="normal-auto-fit"get an error telling them to set exactly that. Suppress that clause when the element already has those values.🐛 Suggested tweak
- message = ( - f'text shape {element["id"]} may overflow its own content box ' - f'(estimated {estimated_height:g}px, available {available_height:g}px); ' - 'consider setting content wrap="true" autoFit="normal-auto-fit"' - ) + already_auto_fit = element.get("autoFit") == "normal-auto-fit" + message = ( + f'text shape {element["id"]} may overflow its own content box ' + f'(estimated {estimated_height:g}px, available {available_height:g}px)' + ) + if not already_auto_fit: + message += '; consider setting content wrap="true" autoFit="normal-auto-fit"' + else: + message += '; autoFit="normal-auto-fit" will shrink the text, verify the rendered size'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/lark-slides/scripts/xml_text_overlap_lint.py` around lines 910 - 932, The overflow warning in the message construction should avoid recommending wrap and normal-auto-fit when the element already declares both settings. Update the logic around the existing message and element attributes in the text-overflow lint path, using an alternative hint for shapes that already have those values while preserving the current advice for all others.
🧹 Nitpick comments (6)
skills/lark-slides/scripts/xml_text_overlap_lint.py (3)
1269-1274: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe whitelist makes the
extra_elementscanvas pass dead code.
lint_xmlcallsdetect_elements_out_of_canvas(extra_elements, ...)with onlyicon/polyline/linekinds, none of which pass this filter, soextra_overflow_issuesis now always empty. Drop that call (or add the intended kinds to the whitelist) so the intent is explicit rather than silently no-op.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/lark-slides/scripts/xml_text_overlap_lint.py` around lines 1269 - 1274, The whitelist in the element iteration excludes all kinds passed to the extra_elements canvas check, making lint_xml’s extra_overflow_issues path ineffective. Update lint_xml and the related element filtering so the intended extra_elements kinds are processed, or remove the now-dead detect_elements_out_of_canvas(extra_elements, ...) call; keep the chosen behavior explicit and ensure extra_overflow_issues is not silently always empty.
938-956: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the magic thresholds into module constants.
96,0.5here and theoverflow > 10cutoff at line 909 are tuning knobs; this module already keeps such values as named constants (e.g.CANVAS_OVERFLOW_TOLERANCE), which makes them discoverable and testable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/lark-slides/scripts/xml_text_overlap_lint.py` around lines 938 - 956, Define module-level named constants for the decorative-text font-size threshold, alpha threshold, and the overflow cutoff currently using 96, 0.5, and 10. Update is_background_decorative_text and the overflow check near the existing tolerance constants to reference those constants, preserving the current threshold behavior.
809-815: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
estimate_text_max_line_widthstill ignores paragraph-levelletterSpacing.It splits
element["text"]on newlines and resolves spacing element-only, so a<p letterSpacing="4">override is invisible to horizontal-overflow and visual-bbox estimates even though line counting now honors it. Iteratingelement["paragraphs"](falling back to the split when absent) would keep both paths consistent.♻️ Sketch
def estimate_text_max_line_width(element: dict[str, Any]) -> int | float: font_size = element["fontSize"] if isinstance(element["fontSize"], (int, float)) else 16 - letter_spacing = resolve_letter_spacing(element) - paragraphs = [paragraph for paragraph in re.split(r"\n+", element["text"]) if paragraph] - return max( - [estimate_text_width(paragraph, font_size, letter_spacing) for paragraph in paragraphs] or [1] - ) + records = element.get("paragraphs") or [{"text": element["text"]}] + widths = [ + estimate_text_width(line, font_size, resolve_letter_spacing(element, record)) + for record in records + for line in re.split(r"\n+", record.get("text") or "") + if line + ] + return max(widths or [1])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/lark-slides/scripts/xml_text_overlap_lint.py` around lines 809 - 815, Update estimate_text_max_line_width to iterate element["paragraphs"] when available, resolving each paragraph’s letterSpacing override through the existing spacing-resolution logic; fall back to the current newline-split text when paragraphs are absent. Preserve the existing width calculation and default minimum width so horizontal-overflow and visual-bbox estimates match paragraph-level line counting.skills/lark-slides/scripts/xml_text_overlap_lint_test.py (3)
689-689: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale test name.
test_lint_xml_warns_when_text_may_overflow_its_own_shapenow asserts two errors; rename to reflect the error classification (and that auto-fit shapes are included).Also applies to: 714-725
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/lark-slides/scripts/xml_text_overlap_lint_test.py` at line 689, Rename test_lint_xml_warns_when_text_may_overflow_its_own_shape to describe that it asserts both error classifications and includes auto-fit shapes, updating any related references consistently.
799-861: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider parameterizing these three near-identical cases.
The alpha / no-foreground / order variants differ only in the shape XML; a single
subTestloop over (name, shapes, expected element id) would cut the duplication.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/lark-slides/scripts/xml_text_overlap_lint_test.py` around lines 799 - 861, Parameterize the three near-identical tests for text overflow severity into one test method using a subTest loop over each XML shape variant and expected element ID. Preserve the existing assertions that the matching text_may_overflow_shape issue has level "error", including the alpha, missing-foreground, and ordering cases.
1101-1143: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFixtures rely on short-circuit evaluation to avoid
KeyError: "type".The
table,chartandimagedicts omittype; the test only passes because theor/andordering in the filter (and inelement_canvas_bbox) never reacheselement["type"]for those kinds. Adding"type"to each fixture makes the test robust against a harmless reordering.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/lark-slides/scripts/xml_text_overlap_lint_test.py` around lines 1101 - 1143, Add the appropriate type field to the table, chart, and image fixtures in test_detect_elements_out_of_canvas_limits_detection_to_whitelist, so each element dictionary is complete and the test no longer depends on short-circuit evaluation avoiding element["type"].
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@skills/lark-slides/scripts/xml_text_overlap_lint.py`:
- Around line 905-909: The background-decoration classification in
`xml_text_overlap_lint.py` must match normalization behavior: either update
`normalize_issue`/`build_result` so `info` is preserved and counted correctly,
or change the classification near `is_background_decorative_text` to emit
`warning`. In `skills/lark-slides/scripts/xml_text_overlap_lint.py` lines
905-909, apply the chosen contract; in
`skills/lark-slides/scripts/xml_text_overlap_lint_test.py` lines 796-797, assert
the resulting level, updating the expectation to `warning` unless normalization
is changed to preserve `info`.
---
Outside diff comments:
In `@skills/lark-slides/scripts/xml_text_overlap_lint.py`:
- Around line 910-932: The overflow warning in the message construction should
avoid recommending wrap and normal-auto-fit when the element already declares
both settings. Update the logic around the existing message and element
attributes in the text-overflow lint path, using an alternative hint for shapes
that already have those values while preserving the current advice for all
others.
---
Nitpick comments:
In `@skills/lark-slides/scripts/xml_text_overlap_lint_test.py`:
- Line 689: Rename test_lint_xml_warns_when_text_may_overflow_its_own_shape to
describe that it asserts both error classifications and includes auto-fit
shapes, updating any related references consistently.
- Around line 799-861: Parameterize the three near-identical tests for text
overflow severity into one test method using a subTest loop over each XML shape
variant and expected element ID. Preserve the existing assertions that the
matching text_may_overflow_shape issue has level "error", including the alpha,
missing-foreground, and ordering cases.
- Around line 1101-1143: Add the appropriate type field to the table, chart, and
image fixtures in
test_detect_elements_out_of_canvas_limits_detection_to_whitelist, so each
element dictionary is complete and the test no longer depends on short-circuit
evaluation avoiding element["type"].
In `@skills/lark-slides/scripts/xml_text_overlap_lint.py`:
- Around line 1269-1274: The whitelist in the element iteration excludes all
kinds passed to the extra_elements canvas check, making lint_xml’s
extra_overflow_issues path ineffective. Update lint_xml and the related element
filtering so the intended extra_elements kinds are processed, or remove the
now-dead detect_elements_out_of_canvas(extra_elements, ...) call; keep the
chosen behavior explicit and ensure extra_overflow_issues is not silently always
empty.
- Around line 938-956: Define module-level named constants for the
decorative-text font-size threshold, alpha threshold, and the overflow cutoff
currently using 96, 0.5, and 10. Update is_background_decorative_text and the
overflow check near the existing tolerance constants to reference those
constants, preserving the current threshold behavior.
- Around line 809-815: Update estimate_text_max_line_width to iterate
element["paragraphs"] when available, resolving each paragraph’s letterSpacing
override through the existing spacing-resolution logic; fall back to the current
newline-split text when paragraphs are absent. Preserve the existing width
calculation and default minimum width so horizontal-overflow and visual-bbox
estimates match paragraph-level line counting.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: da340127-da0d-44be-9b2e-bf1a05ef28a5
📒 Files selected for processing (2)
skills/lark-slides/scripts/xml_text_overlap_lint.pyskills/lark-slides/scripts/xml_text_overlap_lint_test.py
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@ba422b07443875c4843f7bf8f099cf86ad83a660🧩 Skill updatenpx skills add larksuite/cli#fix/slides_text_width_detect -y -g |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2056 +/- ##
=======================================
Coverage 75.14% 75.14%
=======================================
Files 911 911
Lines 96322 96322
=======================================
Hits 72385 72385
Misses 18370 18370
Partials 5567 5567 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Replace the uniform 0.55em half-width coefficient with per-character-type coefficients, add font-family awareness (sans/serif), bold multiplier, letter-spacing support, and fix padding-aware line wrapping. - Split half-width chars into uppercase (0.57), lowercase (0.51 sans / 0.53 serif), digits (0.58), and punctuation (0.50) - Add classify_font_family() to apply slightly wider lowercase widths for serif fonts (Georgia, Source Han Serif/思源宋体, Times, etc.) - Add 5% width multiplier for bold text; detect <strong>/<b>/<i>/<em> tags and span-level bold/italic attributes in addition to content attrs - Fix estimate_text_line_count_for_text to subtract paddingLeft/paddingRight from available width before computing wrap lines - Add resolve_letter_spacing and wire letterSpacing through estimate_text_width - Extract fontFamily/bold/italic/letterSpacing into element dict during parse
There was a problem hiding this comment.
🧹 Nitpick comments (2)
skills/lark-slides/scripts/xml_text_overlap_lint.py (1)
717-750: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBold/italic detection logic looks correct; consider adding direct unit coverage for all 4 fallback paths.
The cascading checks (content attr → shape attr → inline
<strong>/<b>/<i>/<em>tag →<span bold/italic>attr) are logically sound. However, the visible test file only appears to exercise the content-attribute path (e.g.bold="true"on<content>); I don't see a dedicated test for the shape-level attribute, inline-tag, or span-attribute fallbacks. Since this is new, multi-branch logic feeding directly into width/line estimation, a regression in any of these paths would silently affect overflow accuracy.Do you want me to draft
extract_elements-focused unit tests covering each fallback (shapebold/italicattr,<strong>/<em>tags,<span bold="true">) ?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/lark-slides/scripts/xml_text_overlap_lint.py` around lines 717 - 750, Add extract_elements-focused unit coverage for bold and italic detection through the shape-level attributes, inline strong/b/em tags, and span boolean attributes, in addition to the existing content-attribute cases. Assert each fallback sets the corresponding element flags so width and line estimation receive the correct formatting state.skills/lark-slides/scripts/xml_text_overlap_lint_test.py (1)
945-1146: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCanvas-scope-restriction tests align with the intended new behavior.
The updated expectations (images/lines ignored, rotated rect uses declared bounds, whitelist test) are internally consistent with the
element_canvas_bbox/detect_elements_out_of_canvaschanges. See the related comment onxml_text_overlap_lint.py(Lines 1354-1363) about whethericon/polylineexclusion here was fully intentional, since no test in this range explicitly exercises those two kinds againstdetect_elements_out_of_canvas.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/lark-slides/scripts/xml_text_overlap_lint_test.py` around lines 945 - 1146, Review the canvas-scope whitelist in detect_elements_out_of_canvas and explicitly confirm the intended handling of icon and polyline elements alongside the tested image and line exclusions. If they should be ignored, preserve that behavior; otherwise update the whitelist and add focused coverage for both kinds without changing the existing rotated-rect and supported-element behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@skills/lark-slides/scripts/xml_text_overlap_lint_test.py`:
- Around line 945-1146: Review the canvas-scope whitelist in
detect_elements_out_of_canvas and explicitly confirm the intended handling of
icon and polyline elements alongside the tested image and line exclusions. If
they should be ignored, preserve that behavior; otherwise update the whitelist
and add focused coverage for both kinds without changing the existing
rotated-rect and supported-element behavior.
In `@skills/lark-slides/scripts/xml_text_overlap_lint.py`:
- Around line 717-750: Add extract_elements-focused unit coverage for bold and
italic detection through the shape-level attributes, inline strong/b/em tags,
and span boolean attributes, in addition to the existing content-attribute
cases. Assert each fallback sets the corresponding element flags so width and
line estimation receive the correct formatting state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 39e13736-9529-4887-97d9-c68a12bcd582
📒 Files selected for processing (2)
skills/lark-slides/scripts/xml_text_overlap_lint.pyskills/lark-slides/scripts/xml_text_overlap_lint_test.py
1 participant
Summary
Root cause
The lint estimator ignored letter spacing and applied uniform severity and canvas geometry checks to elements that do not need them, causing missed or noisy overflow findings.
Validation
Not run (not requested).
Summary by CodeRabbit
letterSpacingend-to-end, including paragraph-specific values and shaped text content.infoissues and tightened behavior for auto-wrapping and wrapping-line overflow scenarios.