fix(texteditor): correctly apply bold/italic when inside styled span - #8098
fix(texteditor): correctly apply bold/italic when inside styled span#8098nineteen88 wants to merge 1 commit into
Conversation
| expect(node.getFontSize()).toBe("14px"); | ||
| expect(node.getLineHeight()).toBe("21px"); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Are we sure we handle only HTML previously exported by carbon, where parent restores bold format, so it is normalized to regular text? Test verifies weight becomes 400, but fixture has no parent and does not verify the node still has the bold format.. If the intended contract is carbon-exported HTML, could you wrap the fixture in ?
| // that genuinely uses 700 (e.g. title at 24px/30px), it was likely | ||
| // set by format (e.g. parent <strong>) so normalise to base weight. | ||
| if (fontWeight === "700" || fontWeight === "bold") { | ||
| const matchesTypography = Object.values(typographyMap).some( |
There was a problem hiding this comment.
One concern: matchesTypography infers why the source is bold, but the meaning of previosly saved HTML can change when typographyMap changes. As an example, 700/14/21 is normalized today but would be treated as 700 if a matching Typography preset were added later. Don't you think it would be better to preserve source bold signal so it does not depend on Typography preset?
There was a problem hiding this comment.
Pull request overview
This PR addresses a TextEditor serialization issue where applying bold/italic inside a default-typography styled <span> could produce incorrect/ineffective HTML output. It updates the StyledSpanNode to ensure formatting is reflected in the exported DOM and adds unit tests to cover the new behavior.
Changes:
- Update
StyledSpanNodeDOM export/DOM rendering logic to apply effective bold/italic/underline styling for styled spans. - Adjust DOM import logic to normalize bold-looking inline weights against known typography presets.
- Add Jest test coverage for the new DOM create/update/export/import behaviors.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/components/text-editor/__internal__/__nodes__/styled-span.node.ts |
Updates StyledSpanNode DOM export/import and DOM rendering logic to better preserve formatting within styled spans. |
src/components/text-editor/__internal__/__nodes__/styled-span.node.test.tsx |
Adds unit tests covering bold/italic DOM creation, DOM updates, export styling, and import normalization. |
| // If font-weight is bold/700 but doesn't match a typography preset | ||
| // that genuinely uses 700 (e.g. title at 24px/30px), it was likely | ||
| // set by format (e.g. parent <strong>) so normalise to base weight. | ||
| if (fontWeight === "700" || fontWeight === "bold") { | ||
| const matchesTypography = Object.values(typographyMap).some( |
| t.lineHeight === lineHeight, | ||
| ); | ||
| if (!matchesTypography) { | ||
| fontWeight = "400"; |
There was a problem hiding this comment.
Could we set node's bold format when change from 700 to 400? At least, it would keep it visually bold while storing 400 as base weight?
Inferring formatting form typographyMap just caught my eye)
As a long-term solution we could strong represent bold and span - base typography, but that requires round-trip tests and compatibility check.
There was a problem hiding this comment.
Nothing extra to add past the comments from @ibutakova, from me @nineteen88. Happy to approve once they're resolved or addressed👍
There was a problem hiding this comment.
The immediate loss of bold formatting (according to comments) is resolved, and tests cover the behaviour.
As non-blocking small follow-up ticket, I suggest moving typographyMap out of importDom() and pass in a function that determines whether the imported styles represent base typography. This way carbon can still use the current map by default, and custom maps would work without changing the node.
Let me know if the approach makes sense. I can add a ticket to the backlog. Or we can leave it as is.
Yeah fair enough, I agree with the direction. The immediate bug is fixed and covered by tests. But it does seem there's further scope here that could become problematic when White Labeling is being used. If you could create that, that would be great! |
when bold/italic styling is applied to a default styled span within the text-editor, we now ensure that it's correctly serialised fix #8056
Proposed behaviour
when bold/italic styling is applied to a default styled span within the text-editor, we now ensure that it's correctly serialised
fix #8056
Current behaviour
bold/italic styling within a default styled span will break the seralised html output
Checklist
QA
Additional context
Testing instructions