pixiebrix-extension icon indicating copy to clipboard operation
pixiebrix-extension copied to clipboard
verified publisher icon pixiebrix

Widgets are inserted into the user's content when `body.isContentEditable`

Open fregante opened this issue 1 year ago • 3 comments

Describe the bug

Our widgets will be injected into this page and, if the content editable field/frame is read by a controller (as one would expect), it will include our widgets

<!doctype html>
<body contenteditable>
   Hello user, type something here
</body>

To Reproduce

  1. Visit https://contenteditable-body.tiiny.site

Actual behavior

  • See our widgets end up inside the contenteditable field, in the DOM, and are potentially saved as part of the user input
  • Our widgets (including the notifications) are selectable/editable

Expected behavior

I'm not sure of how to deal with this. At at very least we can add a contenteditable=false attribute to all of our widgets (part of #7670)

Demo

Screenshot 23

https://github.com/pixiebrix/pixiebrix-extension/assets/1402241/3311dcf2-eb18-497c-bde6-ea7e937ab2ff

Context

Seen in https://github.com/pixiebrix/pixiebrix-extension/issues/8071

fregante avatar Mar 29 '24 13:03 fregante

Thoughts

  • We cannot hide the elements from the reader, they can use .innerHTML at any time to "save" all of the elements
  • Our shadow DOM already hides the actual content, providing that we stop using mode=open on the shadow DOM

Provisional resolutions

  • [ ] ~~Use contenteditable=false on every shadow root~~
  • [ ] ~~Use mode=closed on every shadow root~~
  • [ ] ~~Introduce lint rule to avoid mode=open~~
  • [ ] Move notifications to shadow DOM
    • it requires vendoring react-hot-toast https://github.com/timolins/react-hot-toast/issues/189
  • [ ] Proactively remove elements from the DOM when not in use, rather than leaving them behind
  • [ ] Keep in mind that some of our elements may already exist on the page, and our code will reuse them: https://github.com/pixiebrix/pixiebrix-extension/blob/58ae23e997d43011220c74f23792b1b129e7f22b/src/contentScript/tooltipDom.ts#L31-L33

Also

  • [ ] Add these to the list of things to include in https://github.com/pixiebrix/pixiebrix-extension/issues/7670

fregante avatar Mar 29 '24 13:03 fregante

The toast is not in a shadow dom. We don't have this problem with the Snippet Shortcut Menu, and it's in a mode=open shadow dom. I would be against requiring mode=closed. I'm not convinced it does anything for us, and closed shadow doms are harder to write tests for.

grahamlangford avatar Mar 29 '24 15:03 grahamlangford

The toast is not in a shadow dom

Indeed it looks like the shadow dom already does not inherit contenteditable, so this specific issue does not apply. However the content is still accessible and editable by the host website's JavaScript

I'm not convinced it does anything for us

It does what it says on the tin, and it says isolation is good for safety and reliability. Content scripts are specifically run in an isolated context to avoid conflicts and increase security, this continues on that line.

closed shadow doms are harder to write tests for

I mentioned this before, all we need is something like mode={process.env.SHADOW_MODE}, also easily enforceable via lint; we already have a custom ENV specific to E2E so it's a solved problem.

Anyway for this issue the mode isn't relevant so I crossed those off.

fregante avatar Mar 29 '24 15:03 fregante

I'm not sure of how to deal with this. At at very least we can add a contenteditable=false attribute to all of our widgets

Given that this case arises with certain editors, IMO the correct long-term fix is to render the elements on the on the parent frame (which IIRC is what Grammarly does).

To solve/prevent the immediate problem of data corruption in editors, I would recommend the following immediate approach for body[contenteditable] (just body[contenteditable] for now, but we might consider :

Show in top-level frame:

  • Toast
  • Open sidebar dialog: see bug: https://github.com/pixiebrix/pixiebrix-extension/issues/8141
  • Integration configuration banner
  • Quick Bar: IIRC already only shows in top-level frame
  • Sidebar: IIRC already only shows in top-level frame

Throw an error (mod developer should just have these run in the top-level frame):

  • Modal
  • Popover (display temporary information with "popover" location)

Ensure PixieBrix always cleans up the container:

  • Snippet shortcut menu
  • Text selection menu

Editors Impacted (in playground, we should include runtime tests):

  • TinyMCE 6
  • CKEditor 4
  • CKEditor 5

~~For Salesforce CKEditor 4, I'm seeing some weird behavior where the toast is showing in the editor before I think the context menu action is even run: https://github.com/pixiebrix/pixiebrix-extension/issues/8071#issuecomment-2041639163~~. Update: that alert had gotten saved in the editor

twschiller avatar Apr 08 '24 10:04 twschiller