#7300: Enable contextMenus on srcdoc iframes
#7305
Conversation
fregante
changed the title
srcdoc iframes
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7305 +/- ##
==========================================
- Coverage 72.43% 72.43% -0.01%
==========================================
Files 1201 1201
Lines 37389 37389
Branches 7018 7018
==========================================
- Hits 27083 27082 -1
- Misses 10306 10307 +1 ☔ View full report in Codecov by Sentry. |
package.json
Outdated
| @@ -165,7 +165,7 @@ | |||
| "use-sync-external-store": "^1.2.0", | |||
| "uuid": "^9.0.1", | |||
| "webext-base-css": "^1.4.4", | |||
| "webext-content-scripts": "^2.6.0", | |||
| "webext-content-scripts": "^2.6.1", | |||
There was a problem hiding this comment.
| @@ -22,7 +22,6 @@ import { getReloadOnNextNavigate } from "@/contentScript/ready"; | |||
|
|
|||
| type MenuHandler = (args: Menus.OnClickData) => Promise<void>; | |||
|
|
|||
| // TODO: Replace with `SimpleEventTarget` (e.g. target.emit(extensionId)) | |||
| // eslint-disable-next-line local-rules/persistBackgroundData -- Functions | |||
| const handlers = new Map<UUID, MenuHandler>(); | |||
There was a problem hiding this comment.
I looked at the code and SimpleEventTarget isn't enough here
src/bricks/available.ts
Outdated
| @@ -28,7 +28,12 @@ export function testMatchPatterns( | |||
| ): boolean { | |||
| for (const pattern of patterns) { | |||
| try { | |||
| if (patternToRegex(pattern).test(url)) { | |||
| if ( | |||
There was a problem hiding this comment.
This would work in this specific case?
But for isAvailable checks from the content script, I think we want to introduce a version of this that returns true if <all_urls> is the pattern regardless of protocol. Because if the content script is running in the context, it means that chrome thinks the content script can run there
Investigating some 1.8.6 issues, but will take a look after
There was a problem hiding this comment.
That makes sense, but I think it would introduce some mismatches in some cases when this function is called from two contexts, for example specifically in contextMenus.
I don't know if that happens and if that is troublesome though, just worth mentioning.
There was a problem hiding this comment.
How do you want to proceed here?
There was a problem hiding this comment.
I'm going to close this out for now until it comes up again (doesn't seem to be requirement for client). We'll want to add some regression tests/etc. around it.
There was a problem hiding this comment.
Given the fix is very specific and it adds (at least partial) support to srcdoc frames, wouldn't it be easier to merge it? So we don't have to track it anymore, especially if it's low priority.
The webpack side is already being taken care of by the plugin developer apparently, so there's a good chance this will add full support with near-zero effort.
There was a problem hiding this comment.
Sure, I can re-open. Could you add a test case(s) to the method?
| expect(test(patterns, "https://www.example.comunication")).toBeFalse(); | ||
| expect(test(patterns, "https://www.pixiebrix.com/")).toBeFalse(); | ||
| expect(test(patterns, "about:srcdoc")).toBeFalse(); | ||
| }); |
There was a problem hiding this comment.
Here I just improved the readability + added one srcdoc assertion
|
|
||
| expect(test(["<all_urls>"], "https://www.example.com")).toBeTrue(); | ||
| expect(test(["<all_urls>"], "about:srcdoc")).toBeTrue(); | ||
| expect(test(["<all_urls>"], "chrome://extensions")).toBeFalse(); |
There was a problem hiding this comment.
Tested all_urls and srcdoc here specifically
| } | ||
| if (url === "about:srcdoc") { | ||
| // <all_urls> doesn't officially include about:srcdoc, but it works in some cases | ||
| return patterns.includes("<all_urls>"); |
There was a problem hiding this comment.
Instead of testing this for every pattern, I just do a single check. There's only a case where about:srcdoc is a match.
| } | ||
|
|
||
| return false; | ||
| try { | ||
| return doesUrlMatchPatterns(url, ...patterns); |
There was a problem hiding this comment.
I moved the loop to this function to optimize for the common scenarios and to include it in test suite of webext-patterns itself:
https://github.com/fregante/webext-patterns/blob/main/test/patterns.js
| } catch { | ||
| const invalidPattern = patterns.find((pattern) => !isValidPattern(pattern)); | ||
| throw new BusinessError( | ||
| `Pattern not recognized as valid match pattern: ${invalidPattern}`, |
There was a problem hiding this comment.
We could improve the error message in webext-patterns and just expose the error thrown by doesUrlMatchPatterns (by just wrapping it in a BusinessError)
|
No loom links were found in the first post. Please add one there if you'd like to it to appear on Slack. Do not edit this comment manually. |
What does this PR do?
Demo
You can also test it on https://ephiframe.vercel.app/?srcdoc=%3Ch1%3Ehello-srcdoc
Future Work
allow-scriptsor figure out what's up with webpack injecting scriptsChecklist