Skip to content

[WC-3647] Combine seven rich text fixes - #2495

Open
samuelreichert wants to merge 12 commits into
mainfrom
fix/rich-text-combined-fixes
Open

samuelreichert wants to merge 12 commits into
mainfrom
fix/rich-text-combined-fixes

Conversation

@samuelreichert

@samuelreichert samuelreichert commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor

Pull request type

Bug fix (non-breaking change which fixes an issue)


Description

This PR combines seven Rich Text bug fixes into one branch, based on the latest main, so they can be reviewed, tested and released together. Each fix stays a separate commit.

Included PRs (this PR supersedes them):

PR Fix
#2454 Keep text color, font family and font size in class style mode
#2456 Run On change on leave only after an edit
#2458 Discard the typed value when pressing Escape in a configuration dropdown
#2459 Ignore trailing line breaks in the character count
#2460 Build a valid Google Maps embed URL
#2465 Apply the "Enable spell checking" setting
#2466 "Default font family" and "Default font size" settings (removed, see 7)

1. Class style mode loses text color, font family and size (#2454)

With Style data format = Class, these marks are saved as <span data-text-color|data-font-family|data-font-size class="has-…"> with no style attribute. Tiptap's TextStyle mark only parses a <span> that has a style attribute, so the formatting was dropped when the content was loaded again.

A new TextStyleClass extension extends TextStyle and, in class mode only, adds a parse rule for spans with these data attributes. Inline mode is unchanged.

Two follow-ups from review make the restored formatting show correctly:

  • Font size in the toolbar: class mode saves the size without a unit (data-font-size="24"). FontSize now restores the px unit when parsing, so the font size dropdown shows 24px for loaded text, the same as in inline mode. Before, it showed the "Font Size" placeholder.
  • Font family rendering: RichTextFormatStyle.scss read data-font-family with attr(… raw-string). That gives one CSS string, so a list such as impact, sans-serif was read as one family name that does not exist, and the text showed in the browser's default font. It now uses attr(… type(*)), which inserts the value as a family list. The list marker font family had the same issue and is fixed the same way. An unsafe value such as impact; color: red makes the declaration invalid, so it falls back to the inherited font. This issue existed before this PR, so it has its own changelog entry.

2. On change runs on every blur (#2456)

With On change type = When user leaves input field, On change ran on every blur, even without an edit. A dirty flag is now set by a real edit (onUpdate) and cleared on blur. External value syncs use emitUpdate: false, so they never set it. On leave still runs on every blur.

Out of scope: clicking a toolbar button also blurs the editor, so editing and then clicking the toolbar still runs On change.

3. Escape saves the typed value in configuration dropdowns (#2458)

Escape called clearDraft() and then blur() straight away, so onBlur committed the draft before the state update applied. A ref set around the Escape-triggered blur now makes onBlur skip the commit.

4. Character count includes trailing line breaks (#2459)

getCharCount now returns text.trimEnd().length, as described in the 4.12.0 changelog. Word count and HTML character count are unchanged.

5. Invalid Google Maps embed URL (#2460)

The template in videoUrlPattern.ts used msid=$2 instead of $3 and ended with a stray ". It is now maps.google.com/maps/ms?msid=$3&output=embed.

The test-only commits of #2460 are already on main, so only the fix commit is included here.

6. "Enable spell checking" has no effect (#2465)

The setting was lost in the move from Quill to Tiptap. EditorWrapper now passes spellCheck to Editor, which sets spellcheck on the editable area, as 4.x did. role: "textbox" is set explicitly in the same attributes, because useEditor replaces them on re-render. Existing snapshots only gain spellcheck="true".

7. "Default font family" and "Default font size" settings (#2466)

These settings were lost in the move to Tiptap. In 4.x they were needed because Quill's dropdowns showed their first option (such as 12px or Arial) for text without a font, which could be wrong. The Tiptap toolbar shows placeholders instead, so this PR removes the settings instead of restoring them as #2466 did.

  • The font family dropdown shows "Font Family" for text without a font family.
  • The font size dropdown now shows "Font Size" for text without a font size. Before, it showed a hard-coded 14px, which may not match the theme.
  • In apps that set these properties, Studio Pro asks to update the widget and the values are dropped.

Notes on combining

  • The CHANGELOG entries are under [Unreleased]: one ### Removed entry for the default font settings and one ### Fixed list for the rest.
  • The snapshot for "renders with both word and character count" was dropped from the spell-check commit, because that test no longer exists on main.

All fixes were written test-first. In rich-text-web: 40 test suites and 624 tests pass, tsc --noEmit is clean, and lint has 0 errors. The lint warnings are the same as on main.

What should be covered while testing?

  1. Class style mode: set Style data format to Class. Apply a text color, a font family and a font size, then save and reload the page. The formatting is still there and the saved HTML has no inline style. The text shows in the applied font, and with the cursor in it, the font family and font size dropdowns show the applied values. Repeat with Inline: nothing changed.
  2. On change on leave: configure On change (type: when user leaves input field) to show a message. Tab in and out without typing: no message. Type, then leave: the message appears once. Enter and leave again without typing: no message.
  3. Escape in a configuration dropdown: open a table configuration dropdown and type a new value in a number field. Press Escape: the value goes back and the table is unchanged. Type a value and press Enter: it is applied.
  4. Character count: enable the status bar with Character count. Type abc and press Enter twice. The count stays at 3.
  5. Google Maps embed: paste a https://maps.google.com/maps/ms?…&msid=<id> link in the video dialog. The embed URL is well-formed.
  6. Spell checking: set Enable spell checking to Yes and type a misspelled word: the browser underlines it. Set it to No: misspellings are not underlined.
  7. Default font settings removed: the Default font family and Default font size properties are no longer in Studio Pro. Place the cursor in unformatted text: the dropdowns show "Font Family" and "Font Size". Place it in text with a font family and size: the dropdowns show those values.

🤖 Generated with Claude Code

@samuelreichert
samuelreichert requested a review from a team as a code owner October 28, 2026 09:23
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

gjulivan
gjulivan previously approved these changes Oct 30, 2026
@samuelreichert
samuelreichert force-pushed the fix/rich-text-combined-fixes branch from 16d3bfa to 123a395 Compare October 30, 2026 10:21
@github-actions

This comment has been minimized.

@samuelreichert
samuelreichert force-pushed the fix/rich-text-combined-fixes branch from 123a395 to 0228fe2 Compare October 30, 2026 13:42
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

gjulivan
gjulivan previously approved these changes Oct 30, 2026
<caption>Default font size</caption>
<description />
</property>
<property key="customFonts" type="object" isList="true" required="false">

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚨 High — defaultFontFamily / defaultFontSize removed from the XML.

The PR description (section 7) says these settings are now applied as the initial value of the font dropdowns, and the test plan expects "Default font family = Arial → dropdown shows Arial". The diff does the opposite: it deletes both properties from the XML and typings, and ToolbarConfig.ts changes the font size fallback from "14px" to "". A search of src/ finds no remaining use of defaultFontFamily or defaultFontSize.

Removing XML property keys breaks existing apps that configure them (the widget definition no longer matches the page). The changelog lists it under ### Removed, but there is no migration path, and a bug-fix PR that is "non-breaking" should not remove properties.

Fix: Decide which is intended. If the description is right, restore the properties and implement the dropdown defaults. If removal is intended, update the description and PR type, and ship it as a deliberate breaking change in a major release. Also reconsider the "14px" → "" fallback, which the description says stays as the placeholder.

@github-actions

This comment has been minimized.

samuelreichert and others added 6 commits November 5, 2026 15:56
…yle mode

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
samuelreichert and others added 6 commits November 5, 2026 15:56
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Text colors stored in class style mode are now kept when content loads,
so the editor and the View/Edit Code dialog show them.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@samuelreichert
samuelreichert force-pushed the fix/rich-text-combined-fixes branch from 28c2d40 to b6c2689 Compare November 5, 2026 14:56
@github-actions

github-actions Bot commented Nov 5, 2026

Copy link
Copy Markdown
Contributor

AI Code Review

✅ Approved — no issues found


What was reviewed

File Change
rich-text-web/CHANGELOG.md Removed and Fixed entries under [Unreleased]
rich-text-web/src/RichText.xml, typings/RichTextProps.d.ts Removed defaultFontFamily and defaultFontSize. Typings are in sync, and no references remain in packages/.
src/extensions/TextStyleClass.ts, FontSize.ts, Editor.tsx New class-mode textStyle parse rule. The font size gets its px unit back on parse. TextStyle is replaced with TextStyleClass.
src/components/Editor.tsx, EditorWrapper.tsx A dirty flag so On change on leave runs only after an edit. spellCheck is passed through, and role="textbox" is kept in editorProps.
src/components/toolbars/** Escape discards the draft via a skip-commit ref. The font size dropdown shows the placeholder when no size is set.
src/components/StatusBar.tsx, src/utils/videoUrlPattern.ts, src/ui/RichTextFormatStyle.scss The character count trims trailing whitespace. The Google Maps embed URL is fixed. Font family uses attr(... type(*)).
Tests, snapshots, e2e PNGs New and updated specs for each fix.

Skipped (out of scope): the e2e snapshot PNGs, and the typings, which are generated.

CI: pending: Run code quality check, Plan E2E matrix, sigridci · unrelated failures: security/snyk (external scanner)


Questions

  • The two removed properties could be set in existing apps. The changelog says they "had no effect", so this should be safe. Could you confirm that nothing at runtime read them, including the 5.x editor preview? Grep finds no remaining references.

Positives

  • Each fix has focused unit tests, including the trailing-whitespace character count and the Google Maps embed URL.
  • The dirty flag is only set in onUpdate, and external syncs use emitUpdate: false, so loading a value never triggers On change.
  • The changelog entries are user-facing and follow the repo voice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants