Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Blocking import/export compatibility issues and regression-test gaps remain.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
src/components/text-editor/internal/nodes/styled-span.node.ts:211
- issue (blocking): This updated fallback also handles legacy
fontWeight === "bold"markup, but every added legacy test uses the numeric"700"form. Add a focused keyword-form import test so the changed size/line-height recovery is covered for both accepted inputs.
// No preset matches 700 at this size/line-height, so 700 was
// likely applied formatting, not the base weight. Recover the
// true base weight from size + line-height, falling back to
// paragraph weight if nothing matches.
const matchedBySizeOnly = Object.values(typographyMap).find(
(t) => t.size === fontSize && t.lineHeight === lineHeight,
);
fontWeight = matchedBySizeOnly ? matchedBySizeOnly.weight : "400";
src/components/text-editor/internal/nodes/styled-span.node.ts:212
- issue (blocking):
font-weight: boldenters this fallback even when the size/line-height is the title preset, because the preset check only matches the literal"700". As a result, equivalent legacy/foreign markup such as<span style="font-weight: bold; font-size: 24px; line-height: 30px">is reconstructed as a title withboldformat enabled, unlike the same markup using700. Normalizeboldto700before the preset check (and add a regression case) so a natural title is not marked as user-applied bold.
const matchedBySizeOnly = Object.values(typographyMap).find(
(t) => t.size === fontSize && t.lineHeight === lineHeight,
);
fontWeight = matchedBySizeOnly ? matchedBySizeOnly.weight : "400";
shouldApplyBoldFormat = !domNode.closest("strong, b");
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
8869468 to
ac53dea
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Two moderate blocking findings remain in typography import and serialization compatibility.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
src/components/text-editor/internal/nodes/styled-span.node.ts:167
- issue: (blocking): Adding this marker to every exported span changes the existing HTML serialization contract. The current exact Playwright assertion at
src/components/text-editor/text-editor.pw.tsx:477-479and the inline-style assertion atsrc/components/text-editor/text-editor.test.tsx:787-789will fail becausedata-carbon-styled-spanis inserted into the<span>. Either update those affected contract tests and document the new attribute, or restrict the marker to the ambiguous custom-700 cases so ordinary exports remain unchanged.
// marks element as a current carbon export.
element.setAttribute(CARBON_STYLED_SPAN_ATTRIBUTE, "true");
src/components/text-editor/internal/nodes/styled-span.node.ts:166
- nitpick: (non-blocking): This new comment should use a sentence-style comment and capitalize the Carbon name.
// marks element as a current carbon export.
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
ac53dea to
69c079b
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Exported bold formatting is incorrect when the span retains a base weight below 700.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
69c079b to
983d1b6
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Blocking issues remain in explicit bold-title export and legacy font-weight: bold import handling.
Get a fresh assessment by requesting another Copilot review.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Review details
Files excluded by content exclusion policy (1)
- skills/carbon-react/components/text-editor.md
Suppressed comments (3)
src/components/text-editor/internal/nodes/styled-span.node.ts:220
- issue: (blocking): When legacy or foreign HTML uses the CSS keyword
font-weight: boldwith title dimensions (24px/30px),matchesTypographyis true, so this branch leavesfontWeightas"bold".getTypographyKey()only recognizes"700", so the imported node falls back to paragraph typography and loses the title style. Normalize the keyword before the match and add a regression test.
if (
!isCurrentCarbonExport &&
(fontWeight === "700" || fontWeight === "bold")
) {
src/components/text-editor/internal/nodes/styled-span.node.ts:163
- issue: (blocking): The PR's explicit-bold-title requirement says the exported span retains its base
font-weight: 700, but this condition suppressesfontWeightfor every bold node. The new test consequently asserts the opposite of that requirement. Keep the 700 base weight on the span while still omitting non-700 base weights so thestrongwrapper can render those cases.
if (!isBold) {
element.style.fontWeight = this.__fontWeight;
}
src/components/text-editor/internal/utils/helpers.ts:24
- issue: (non-blocking): The new
BandItag mappings are not covered by the parameterized test, which only exercisesstrong,em, andu. Regressions in the aliases could therefore pass CI; addbandicases to the same test.
STRONG: "font-weight: bold;",
B: "font-weight: bold;",
EM: "font-style: italic;",
I: "font-style: italic;",
U: "text-decoration: underline;",
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Lite
08f7a47 to
9f7ae71
Compare
Proposed behaviour
Separates typography from user formatting in Text Editor HTML exports, making save/import round trips reliable.
StyledSpanNode now exports user formatting with semantic wrappers:
<strong><span data-carbon-base-weight="500" style="font-size: 21px; line-height: 26.25px">Bold subtitle</span></strong>StyledSpanNode.exportDOM()no longer writes effective bold, italic, or underline styles directly to the<span>. User-applied formatting is represented by surroundingstrong,em, anduwrappers. The span keeps its base size and line height; for bold content, its base weight is stored indata-carbon-base-weightso<strong>cancontrol the rendered weight.
StyledSpanNode.importDOM()'slegacy fallback handles older Carbon and foreign HTML that encoded bold as a literal font-weight on the span. When the weight does not match a 700 typography preset, it recovers the matching preset weight from the size and line height instead of always defaulting to 400. This preserves legacy bold headings and subtitles more accurately after import.Solution does:
For HTML stored and later re-imported, data-carbon-base-weight is retained as part of the exported content.
Current behaviour
StyledSpanNodecurrently writes bold formatting in two places:spanstrongelementThis makes HTML import difficult because font-weight: 700 could mean either a naturally bold typography style, such as a title, or bold formatting applied by the user.
Checklist
d.tsfile added or updated if requiredQA
Additional context
Testing instructions
strong.strongwrapper.strong, span still 700.emonly.uonly.u``em``strong``span.Paste/import HTML exported by an older Carbon version (or any HTML with font-weight:700 directly on a span, no
strong) → text should still import as bold correctly.