search

pixiebrix / pixiebrix-extension

PixieBrix browser extension
https://www.pixiebrix.com
GNU Affero General Public License v3.0
83 stars 22 forks source link

Content script not loading in Salesforce CKEditor 4 #8071

Open twschiller opened 6 months ago

twschiller commented 6 months ago

Describe the bug

Selection popover not displayed in Salesforce CKEditor

To Reproduce

Steps to reproduce the behavior:

  1. Define a context menu
  2. Try to select some text in the email editor: https://pixiebrix3-dev-ed.lightning.force.com/lightning/r/Case/5008X00002YDcUtQAL/view

Actual behavior

The tooltip container exists in the DOM, but is not visible. So the contentScript appears to be partially loaded

Expected behavior

User can see/click on the a selection menu item

Screenshots/Loom

image

Desktop (please complete the following information):

Discussion/Investigation

If you try the context menu, you get the following error: image

Additional context

Add any other context about the problem here.

twschiller commented 6 months ago

@fregante any ideas what might cause this?

fregante commented 6 months ago

Could be some unrelated error crashing the content script. The console for the specific frame should have loading information. I don’t think the nesting causes issues when loading, but we likely do not support inserting into a double-nested iframe unless you use target: all frames

twschiller commented 6 months ago

but we likely do not support inserting into a double-nested iframe unless you use target: all frames

This is issue is not referring to the "Insert at Cursor" brick. The content script does not appear to be initializing properly. If you define an "alert" brick on a load trigger to run on <all_urls> you won't see it in the editable area

fregante commented 6 months ago

This is issue is not referring to the "Insert at Cursor" brick

Yeah I was answering to the message on Slack that pointed here as well.

The console for the specific frame should have loading information

The content script is reportedly loaded and ready in every context:

Screenshot 9 Screenshot 8

The tooltips container is present, but nothing visible is in it:

Screenshot 10

Notable:

My guess:

  1. the iframe loads the current page
  2. the content script runs and sets the attribute
  3. iframe.contentDocument.write overrides the content of the iframe, losing the "ready" attribute
  4. a followup "is content script ready" call can't find the attribute and times out

I'm not entirely sure of how to deal with the loading of content scripts in such frames, I can take a look tomorrow.

twschiller commented 6 months ago

I'm not entirely sure of how to deal with the loading of content scripts in such frames, I can take a look tomorrow.

Thanks for investigating! We're grabbing some time with the client to see if it will come up in their use case. If it's not we can potentially deprioritize this quirk

fregante commented 6 months ago

Removing the attribute and the check would probably fix this specific issue

Related:

fregante commented 6 months ago

I'm thinking if we really need the concept of "content script ready". Maybe it can be simplified to:

And then in the page editor we keep a "connected / not connected" state, which could also be visible to the user. This would also be useful for the MV3 sidebar. Also discussed in https://github.com/pixiebrix/pixiebrix-extension/issues/7271

twschiller commented 6 months ago

Customer is using that email field (it appears to be the default for Salesforce lightening). Thanks for investigating/coming up with ideas. I'll take a look this afternoon

twschiller commented 6 months ago

And then in the page editor we keep a "connected / not connected" state

I'm not sure what the connection to the Page Editor is? The bug is also happening without the Page Editor open. Or are you saying that would just be an added benefit?

Is there are reason why we're using the symbol for "installed" but the attribute for "ready"? https://github.com/pixiebrix/pixiebrix-extension/blob/60500955ec2135875331f92425ec934f302dd474/src/contentScript/ready.ts#L115-L115

It seems to me we should be using the JS environment for both since it's protected from the host page? In general, the DOM is hostile ground because the host page can tamper with it. (Which is also why I'm wary of using an iframe for proxying API calls https://github.com/pixiebrix/pixiebrix-extension/issues/7182#issuecomment-1868349878)

On my local main, it seems like the import isn't finishing so the document is never marked as ready but doesn't appear to be erroring?

image

The logs don't make sense to me - I'm never seeing the contentScriptCore: init log message but that frame somehow has the pixiebrix-tooltips-container. Perhaps the document.write is copying that over (could always add a nonce to the div to confirm)

I gave this a try to no avail:

  // Keeping the import separate ensures that no side effects are run until this point
  let contentScriptPromise = import(
    /* webpackChunkName: "contentScriptCore" */ "./contentScriptCore"
  );

  contentScriptPromise = pTimeout(contentScriptPromise, {
    milliseconds: 10_000,
    async fallback() {
      console.debug("Retrying import");
      return import(
        /* webpackChunkName: "contentScriptCore" */ "./contentScriptCore"
      );
    }
});

The frame must be messing with webpack-target-webextension's ability to dynamically import

fregante commented 6 months ago

Some frame behaviors described here might be useful to know at some point: https://togithub.com/w3c/webextensions/issues/575

I'm not sure what the connection to the Page Editor is? The bug is also happening without the Page Editor open. Or are you saying that would just be an added benefit?

Yeah it's a semi-related feature request. The page editor is just a reader of the content script readiness state, it could fail under similar circumstances (content script loads, host website clears attributes on <html>).

reason why we're using the symbol for "installed" but the attribute for "ready"

There's a file header adding some context. In short it's for duplicate detection across extension reloads. Symbols don't match in that case, we need a document-level marker.

I can check what else we can use for this. Judging by the comment, it's possible that after the work done in https://github.com/pixiebrix/pixiebrix-extension/issues/4258, we don't really need to detect this, so we can stop using the attribute altogether.

When checking for readiness, we can just message the content script and store the state in a variable.

The logs don't make sense to me - I'm never seeing the contentScriptCore: init log message but that frame somehow has the pixiebrix-tooltips-container.

I can look into it today

twschiller commented 6 months ago

There's a file header adding some context. In short it's for duplicate detection across extension reloads. Symbols don't match in that case, we need a document-level marker.

Seems like we could also consider using both a symbol and an HTML attribute to be resilient to extension reloads and hostile pages?

Judging by the comment, it's possible that after the work done in #4258, we don't really need to detect this, so we can stop using the attribute altogether.

You're suggesting that the only conflict between content scripts is duplicate triggers/etc. that have been added to the page? That seems correct. There will be some things (e.g., you were using the highlighting brick) we can't undo, but that's probably OK?

I can look into it today

Thanks!

fregante commented 6 months ago

Seems like we could also consider using both a symbol and an HTML attribute to be resilient to extension reloads and hostile pages?

Either we need the attribute or we don't. If we don't require it to exist, then we don't need it. The symbol cannot be used across extension restarts.

conflict between content scripts

Yeah the "conflicts" across restarts should just be widgets and event listeners left behind. Every single content script previously injected will still be "running" on the page, so we only need to ensure that it stops reacting to anything.

There will be some things (e.g., you were using the highlighting brick) we can't undo, but that's probably OK?

Yeah user changes to the DOM/content can't be undone, it's just the active parts (buttons, listeners, modals) that must be removed.

The frame must be messing with webpack-target-webextension's ability to dynamically import

That seems to be the case. This is the correct log:

Screenshot 17

But it stops here:

Screenshot 18

I'm trying to isolate the issue and report it.

It seems that the content scripts are loaded for the frames. These are the requests after clicking the "Email" tab:

Screenshot 20

And here you can see the non-messenger WHO_AM_I messages made by contentScript.ts and received by the background:

Screenshot 19

Notable:

  1. frame 49 messages from about:blank, then messages again from another URL, which suggests that the frame is reloaded
  2. frame 50 sends the message from about:blank
  3. only the message from http is followed by a READY message
fregante commented 6 months ago

This appears to be a race condition, there's no specific issue with loading, probably. The content script is able to run after an extension reload (via https://github.com/fregante/webext-inject-on-install) because the iframe is ready and static by that time.

Screenshot 22

Also there doesn't seem to be any issue related to about:blank, PB loads fully in this iframe: https://more-ephiframe.vercel.app/Outer?iframe=about:blank

Screenshot

that frame somehow has the pixiebrix-tooltips-container

This is funny… the element is still there if you disable the extension.

  1. The body is contenteditable
  2. The extension injects the widgets after a reload (as seen above)
  3. The body is saved in the editor, including our own widgets

So now the issues are two:

fregante commented 6 months ago

why isn't the content script fully loading?

  • this issue

My best guess is that document.write or the quick reload is breaking Chrome's own injection mechanism: the first content script load fails because the document is unloaded, but then Chrome no longer natively injects the content scripts.

Now, how do we deal with this issue? Maybe

  1. Isolate/replicate issue, then report to Chrome
  2. Manually inject in this specific Salesforce frame when detected, likely via webNavigation events
twschiller commented 6 months ago

@grahamlangford I'm removing from 1.8.12 scope because this will take more investigation/experimentation

twschiller commented 6 months ago

how do we exclude our widgets from being injected into the user content?

Agree, that's an interesting question. Seeing the notification toast run and get stuck in there automatically. Not sure why that's happening now for me (on 1.8.12-mv3), but hadn't occurred previously. Most likely it's because the content script can make it farther now that it doesn't depend on the pb-ready attribute

image

And the notification is editable because the whole body is contenteditable:

image

If we want to support, we'd need to avoid adding UI elements if the body is contenteditable. And instead show UI affordances the affordance on the parent frame. Toast can just go in their normal spot. Text selection/snippet menus would we'd likely want to translate the x/y values

Also, potentially some context here: https://github.com/ckeditor/ckeditor4/issues/4325. But I couldn't replicate on the linked example page

twschiller commented 6 months ago

😮‍💨🤦: it looks like the alert had gotten saved in the email editor field. So more evidence/cause to prioritize: https://github.com/pixiebrix/pixiebrix-extension/issues/8094

github-actions[bot] commented 3 months ago

This issue will be closed in 7 days unless the stale label is removed, or a comment is added to the issue.