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

fregante · 2024-03-29T13:31:18Z

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

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

Screen.Recording.3.mov

Context

Seen in https://github.com/pixiebrix/pixiebrix-source/issues/322

The text was updated successfully, but these errors were encountered:

fregante · 2024-03-29T13:53:56Z

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 Is it possible to change where the style sheets are injected? timolins/react-hot-toast#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:

pixiebrix-extension/src/contentScript/tooltipDom.ts

Lines 31 to 33 in 58ae23e

    
           let container = document.querySelector( 
        
             `.${PIXIEBRIX_TOOLTIPS_CONTAINER_CLASS}`, 
        
           );

Also

Add these to the list of things to include in Figure out ergonomics of CSS modules in ShadowDOM components #7670

grahamlangford · 2024-03-29T15:03:12Z

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.

fregante · 2024-03-29T15:32:58Z

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.

twschiller · 2024-04-08T10:48:59Z

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: MV3 open sidebar message dialog appears in frames when using Page Editor #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-source/issues/322. Update: that alert had gotten saved in the editor

BLoe · 2024-05-13T20:11:25Z

The alert popups are no longer rendered inside the <body/> element in the page, so they seem to no longer suffer from this issue? I can't produce the behavior from the initial video anymore. Is this issue still a real problem that users suffer from?

twschiller · 2024-05-13T21:59:24Z

Moving to "Next" - I'm also having trouble reproducing on the Salesforce edit field right now: https://pixiebrix3-dev-ed.lightning.force.com/lightning/r/Case/5008X00002YDcUtQAL/view. But I'm not aware of what would have changed

BLoe · 2024-05-13T22:19:54Z

The "widget" used to be appended to body here:

pixiebrix-extension/src/contentScript/modalDom.tsx

Line 64 in abfecaf

document.body.append(container, style);

Federico fixed the issue in this PR

Now, the widget is added directly to document instead, see here

BLoe · 2024-05-13T22:20:13Z

@twschiller I think we should close this issue unless you see a reason otherwise, given the above change

twschiller · 2024-05-13T22:41:43Z

Thanks for clarifying - will close and we can reopen as necessary

fregante added the bug Something isn't working label Mar 29, 2024

fregante added the triage label Mar 29, 2024

grahamlangford added the specification required label Mar 29, 2024

twschiller added customer Required for a customer projct and removed triage labels Apr 8, 2024

twschiller self-assigned this May 13, 2024

twschiller closed this as completed May 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

fregante commented Mar 29, 2024 •

edited

Loading

fregante commented Mar 29, 2024 •

edited

Loading

grahamlangford commented Mar 29, 2024 •

edited

Loading

fregante commented Mar 29, 2024

twschiller commented Apr 8, 2024 •

edited

Loading

BLoe commented May 13, 2024 •

edited

Loading

twschiller commented May 13, 2024

BLoe commented May 13, 2024

BLoe commented May 13, 2024

twschiller commented May 13, 2024 •

edited

Loading

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

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

Comments

fregante commented Mar 29, 2024 • edited Loading

Describe the bug

To Reproduce

Actual behavior

Expected behavior

Demo

Context

fregante commented Mar 29, 2024 • edited Loading

Thoughts

Provisional resolutions

grahamlangford commented Mar 29, 2024 • edited Loading

fregante commented Mar 29, 2024

twschiller commented Apr 8, 2024 • edited Loading

BLoe commented May 13, 2024 • edited Loading

twschiller commented May 13, 2024

BLoe commented May 13, 2024

BLoe commented May 13, 2024

twschiller commented May 13, 2024 • edited Loading

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

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

fregante commented Mar 29, 2024 •

edited

Loading

fregante commented Mar 29, 2024 •

edited

Loading

grahamlangford commented Mar 29, 2024 •

edited

Loading

twschiller commented Apr 8, 2024 •

edited

Loading

BLoe commented May 13, 2024 •

edited

Loading

twschiller commented May 13, 2024 •

edited

Loading