disable the pageshot button on about,data,moz-extension pages #2368
Conversation
|
In response to the description, not the PR: I don't think we can proactively disable the button for framesets, but that it's acceptable once someone tries to use Page Shot to give them an error. In that case, the worker would start up, I think notice that the page has no |
| @@ -55,6 +55,17 @@ window.main = (function () { | |||
| catcher.watchPromise(loadSelector()); | |||
| })); | |||
|
|
|||
| browser.tabs.onUpdated.addListener((id, info, tab) => { | |||
| if (info.url && tab.selected) { | |||
| if (/^(?:about|data|moz-extension):/.test(info.url)) { | |||
There was a problem hiding this comment.
We do have behavior for about:new and about:blank (opening the shots page). So we need a match to treat those separately.
There was a problem hiding this comment.
Ideally that test would be in some new function here and in the onClick handler.
|
I also need to fix switching tabs |
|
The bummer is this is where we actually could affect browser performance generally, even if a user doesn't use Page Shot. I'm open to error popups in all cases to avoid the event listener, though I don't know how passionate @johngruen is about it. |
|
You might want to add a fix for #2237 to this – the error message should be slightly different, but otherwise it's the same logic. |
dannycoates
force-pushed
the
2236-disable-on-about
branch
from
26e45bb
to
98d237a
Compare
|
Since it was easy to add the tab id I went ahead and finished it this way. We can change to an error prompt if this proves too slow, but I think this is a much better UX. @ianb ready for review again. |
dannycoates
force-pushed
the
2236-disable-on-about
branch
2 times, most recently
from
9bac6b9
to
4caf735
Compare
| @@ -40,8 +40,12 @@ window.main = (function () { | |||
| }); | |||
| } | |||
|
|
|||
| function opensMyShots(url) { | |||
There was a problem hiding this comment.
That one "s" between openMyShots and opensMyShots makes quite a bit of difference – easy to misread? shouldOpenMyShots ?
| if (opensMyShots(url)) { | ||
| return true; | ||
| } | ||
| if (url.startsWith(backend) || /^(?:about|data|moz-extension):/i.test(url)) { |
There was a problem hiding this comment.
There might be a whitelist of WebExtension schemes, but I don't know what it is exactly
There was a problem hiding this comment.
I looked on dxr, didn't see anything obvious
| return true; | ||
| } | ||
|
|
||
| browser.tabs.onUpdated.addListener((id, info, tab) => { |
dannycoates
force-pushed
the
2236-disable-on-about
branch
from
4caf735
to
31349ea
Compare
|
The "Firefox Screenshots" button is now disabled on the mentioned pages, but the "Create Screenshot" option is still available in context menu. |
|
Good question @SoftVision-CosminMuntean. Ideally we should remove or disable the menu item from those pages but I don't think we can do that without more code. We could easily add |
It seems like disabling the pageshot button on these pages is better than doing anything else. It needs the
tabspermission (I don't know if that's ok or not) sinceactiveTabdoesn't kick in until a user action is performed.I don't know what to do about framesets yet.
closes #2236