Fix toggling styles on collapsed selections with $patchStyleText #3922
Conversation
birtles
requested review from
trueadm,
zurfyx,
fantactuka,
acywatson,
tylerjbainbridge and
thegreatercurve
as code owners
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
facebook-github-bot
added
the
CLA Signed
| @@ -281,13 +281,16 @@ export function $patchStyleText( | |||
| if (selection.isCollapsed()) { | |||
| const styles = getStyleObjectFromCSS(selection.style); | |||
| Object.entries(patch).forEach(([key, value]) => { | |||
| if (value !== null) { | |||
| if (value === null) { | |||
| delete styles[key]; | |||
There was a problem hiding this comment.
It might be best to avoid the delete keyword here, as there are some cases where it causes V8 to switch away from optimized paths. Could we just set it to undefined?
There was a problem hiding this comment.
I didn't know that. I think it might be better to be consistent with $patchNodeStyle though? Also the type of styles is Record<string, string> so we can't use undefined here without changing the type. (And I don't suppose a helper like $patchStyleText is likely to be on the critical path anyway.)
There was a problem hiding this comment.
I didn't know that. I think it might be better to be consistent with
$patchNodeStylethough? Also the type ofstylesisRecord<string, string>so we can't use undefined here without changing the type. (And I don't suppose a helper like$patchStyleTextis likely to be on the critical path anyway.)
Yea, I think it's fine in this context, given the other considerations. We generally try to avoid it where possible because the manifestations of the performance impact aren't always obvious. IIRC, delete sometimes causes V8 to switch to a less-optimal read structure for that object, which makes future lookups on the same object slower. Admittedly, I'm just using a mental heuristic here - I don't have perfect recall of all the ways the delete keyword can impact performance.
@trueadm am I wrong here?
|
Thank you for this! If you pull the latest from main and rebase, the e2e tests should pass. |
birtles
force-pushed
the
patch-collapsed-styles
branch
from
dd3d2be
to
5e630fb
Compare
Thanks! Rebased. |
Fixes #3921