Skip to content
This repository has been archived by the owner. It is now read-only.

disable the pageshot button on about,data,moz-extension pages #2368

Merged
merged 2 commits into from Mar 22, 2017

Conversation

@dannycoates
Copy link
Contributor

@dannycoates dannycoates commented Mar 13, 2017

It seems like disabling the pageshot button on these pages is better than doing anything else. It needs the tabs permission (I don't know if that's ok or not) since activeTab doesn't kick in until a user action is performed.

I don't know what to do about framesets yet.

closes #2236

@ianb
Copy link
Contributor

@ianb ianb commented Mar 13, 2017

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 document.body, signal an error and tear itself down.

@@ -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)) {
Copy link
Contributor

@ianb ianb Mar 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do have behavior for about:new and about:blank (opening the shots page). So we need a match to treat those separately.

Copy link
Contributor

@ianb ianb Mar 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally that test would be in some new function here and in the onClick handler.

@dannycoates
Copy link
Contributor Author

@dannycoates dannycoates commented Mar 13, 2017

I also need to fix switching tabs

@ianb
Copy link
Contributor

@ianb ianb commented Mar 13, 2017

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.

@ianb
Copy link
Contributor

@ianb ianb commented Mar 14, 2017

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 dannycoates force-pushed the 2236-disable-on-about branch from 26e45bb to 98d237a Mar 15, 2017
@dannycoates
Copy link
Contributor Author

@dannycoates dannycoates commented Mar 16, 2017

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 dannycoates force-pushed the 2236-disable-on-about branch 2 times, most recently from 9bac6b9 to 4caf735 Mar 20, 2017
Copy link
Contributor

@ianb ianb left a comment

Just a couple minor comments, r+ with suggestions

@@ -40,8 +40,12 @@ window.main = (function () {
});
}

function opensMyShots(url) {
Copy link
Contributor

@ianb ianb Mar 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)) {
Copy link
Contributor

@ianb ianb Mar 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There might be a whitelist of WebExtension schemes, but I don't know what it is exactly

Copy link
Contributor Author

@dannycoates dannycoates Mar 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked on dxr, didn't see anything obvious

return true;
}

browser.tabs.onUpdated.addListener((id, info, tab) => {
Copy link
Contributor

@ianb ianb Mar 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

catcher.watchFunction

@SoftVision-CosminMuntean

The "Firefox Screenshots" button is now disabled on the mentioned pages, but the "Create Screenshot" option is still available in context menu.
@ianb , @dannycoates, the option from context menu should not be disabled too? I'm asking this because I am not sure what is the intended behavior.

@dannycoates
Copy link
Contributor Author

@dannycoates dannycoates commented Mar 23, 2017

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 documentUrlPatterns to the menu which would filter out most of the urls we want.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants