Repository navigation
[WC-3647] Combine seven rich text fixes - #2495
samuelreichert wants to merge 12 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
16d3bfa to
123a395
Compare
This comment has been minimized.
This comment has been minimized.
123a395 to
0228fe2
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| <caption>Default font size</caption> | ||
| <description /> | ||
| </property> | ||
| <property key="customFonts" type="object" isList="true" required="false"> |
There was a problem hiding this comment.
🚨 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.
This comment has been minimized.
This comment has been minimized.
…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>
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>
28c2d40 to
b6c2689
Compare
AI Code Review
What was reviewed
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
Positives
|
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):
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 nostyleattribute. Tiptap'sTextStylemark only parses a<span>that has astyleattribute, so the formatting was dropped when the content was loaded again.A new
TextStyleClassextension extendsTextStyleand, 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:
data-font-size="24").FontSizenow restores thepxunit when parsing, so the font size dropdown shows24pxfor loaded text, the same as in inline mode. Before, it showed the "Font Size" placeholder.RichTextFormatStyle.scssreaddata-font-familywithattr(… raw-string). That gives one CSS string, so a list such asimpact, sans-serifwas read as one family name that does not exist, and the text showed in the browser's default font. It now usesattr(… 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 asimpact; color: redmakes 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 useemitUpdate: 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 thenblur()straight away, soonBlurcommitted the draft before the state update applied. A ref set around the Escape-triggered blur now makesonBlurskip the commit.4. Character count includes trailing line breaks (#2459)
getCharCountnow returnstext.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.tsusedmsid=$2instead of$3and ended with a stray". It is nowmaps.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.
EditorWrappernow passesspellChecktoEditor, which setsspellcheckon the editable area, as 4.x did.role: "textbox"is set explicitly in the same attributes, becauseuseEditorreplaces them on re-render. Existing snapshots only gainspellcheck="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
12pxorArial) 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.14px, which may not match the theme.Notes on combining
[Unreleased]: one### Removedentry for the default font settings and one### Fixedlist for the rest.main.All fixes were written test-first. In
rich-text-web: 40 test suites and 624 tests pass,tsc --noEmitis clean, and lint has 0 errors. The lint warnings are the same as onmain.What should be covered while testing?
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.abcand press Enter twice. The count stays at 3.https://maps.google.com/maps/ms?…&msid=<id>link in the video dialog. The embed URL is well-formed.🤖 Generated with Claude Code