Details
- Reviewers
twisniewski - Group Reviewers
Restricted Project anti-tracking-reviewers - Commits
- rMOZILLACENTRAL81954ba2ba03: Bug 1656171 - Shim requestStorageAccess calls for Kinja-powered blogs.
- Bugzilla Bug ID
- 1656171
Diff Detail
- Repository
- rMOZILLACENTRAL mozilla-central
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
D126879 fixes an issue where the popup blocker would block subsequent calls of window.open the website made after the shim requested storage access. There is still a remaining issue I need to address on the platform side: If the user gets prompted for storage access and doesn't accept the prompt within 5 seconds we will block window.open calls. However this is currently rare since we mostly auto grant storage access.
Note that when testing, you should set the pref privacy.antitracking.enableWebcompat to false. This is necessary because the sites are currently on the exception list for partitioning. The pref will also disable the auto grants and trigger a storage access prompt every time.
Good stuff.
r+ with nits addressed.
| browser/extensions/webcompat/data/shims.js | ||
|---|---|---|
| 512 | As we discussed elsewhere, this check isn't currently working for contentScripts, so until that's fixed we need to make sure that it's alright for this shim to apply even if dFPI isn't active. | |
| browser/extensions/webcompat/manifest.json | ||
| 5 | This will of course have to change when we finally land; we're up to 27.1.0 already :) | |
| 132 | Minor nit: let's keep this in alphabetized order | |
| browser/extensions/webcompat/shims/kinja.js | ||
| 86 | If I'm not misreading this, we could optionally simplify it a little to use this form: const SITE_LOGIN_BUTTON_SELECTOR = `
button svg[aria-label^="Facebook"],
button svg[aria-label^="Google"],
button svg[aria-label^="Twitter"]
`;
function getSiteLoginButtons(parentElement) {
return Array.from(
parentElement
.querySelectorAll(SITE_LOGIN_BUTTON_SELECTOR)
.map(el => el.closest("button")
);
}And if we worry that the DOM of the buttons changes, we could add the filter check as well (for null closest elements). We could of course pre-generate the string out of the array, so it doesn't have to be hard-coded: const SITE_LOGIN_BUTTON_SELECTOR =
SITE_LOGIN_BUTTON_SVG_LABELS
.map(label => `button svg[aria-label^="${label}"]`)
.join(",");But the first variant strikes me as being more readable. | |
| 137 | nit: const instead of let for these | |
| 145 | nit: should this var name not be *_ID instead of *_CLASS? | |
This revision requires a Testing Policy Project Tag to be set before landing. Please apply one of testing-approved, testing-exception-unchanged, testing-exception-ui, testing-exception-elsewhere, testing-exception-other. Tip: this Firefox add-on makes it easy!
| browser/extensions/webcompat/data/shims.js | ||
|---|---|---|
| 512 | There is a slightly higher risk of breaking the site, just because we run the shim script even if we don't strictly require it. However, requesting storage access with dFPI turned off works fine and the site still works normally. I've filed https://bugzilla.mozilla.org/show_bug.cgi?id=1733370 for adding support for the flag. | |
| browser/extensions/webcompat/shims/kinja.js | ||
| 86 | Great suggestion, thank you! | |
We don't have a testing harness for shims yet. Also, testing the actual shim with the production site isn't feasible.
New approach where the window.open call does not get blocked by the popup blocker. This is also much simpler and shouldn't break as easily as relying on specific buttons.