Closed Bug 1419842 Opened 7 years ago Closed 7 years ago

Add option to show pageActions by default

Categories

(WebExtensions :: General, enhancement, P3)

Product:
WebExtensions
Component:
General
Type:
enhancement

Tracking

(firefox59 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: ianbicking, Assigned: mixedpuppy)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [design-decision-approved])

Attachments

(1 file)

Bug 1419842 support pattern matching to show/hide pageActions,
59 bytes, text/x-review-board-request
aswan
: review+
Details 
Currently a declared pageAction is only shown when you call pageAction.show(). To add a pageAction to all/most pages this requires adding a tabs.onUpdated event that shows the pageAction. This event has a notable effect on overall Firefox performance.

I would propose that there be a new option in the page_action portion of the manifest, that would ask that the pageAction to be shown on all matching pages.
With respect to the importance of this: in Screenshots we currently use the Photon page action API in bootstrap.js, and do not use the WebExtension API. We would like to change this (issue https://github.com/mozilla-services/screenshots/issues/3756) but in our performance testing we see performance regressions that would not be acceptable: ~4% on the "tart opt" tests (https://screenshots.firefox.com/B2bhjgiflgLvCd43/treeherder.mozilla.org).
Severity: normal → enhancement
Priority: -- → P3
Whiteboard: [design-decision-needed]
Amending Comment 1: I tested the wrong code for performance, I switched tabs.onCreated to tabs.onUpdated and re-ran the test: https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=62068b674a563c78052c6f0cf4a16133a81a1ace&framework=1&showOnlyImportant=1&selectedTimeRange=172800 – the results are worse (5-10% across a dozen tests), so no change in conclusion.
A page action is explicitly designed not to appear all the time, but be relevant to the context of the page. In our documentation we say "Use this button when a feature is only relevant for some web pages"

https://developer.mozilla.org/en-US/Add-ons/WebExtensions/user_interface/Page_actions

On this we were following Chrome because that's what they did. However there's a couple of things to note:
* an extension author could call pageAction.show on every page and make it appear all the time
* Firefox already has the pocket and bookmarks icon appear pretty much all the time (Chrome shows the bookmark API all the time)
* in Photon its being changed so that if the pageAction is set to hide() it will still appear but be disabled (I think) bug 1395387

Given the above, I think this makes sense to do. Although I'm cc'ing product and UX to ask about this.
Flags: needinfo?(mjaritz)
Flags: needinfo?(mconca)
(In reply to Andy McKay [:andym] from comment #3)
> * in Photon its being changed so that if the pageAction is set to hide() it
> will still appear but be disabled (I think) bug 1395387

That was the original behavior implemented in the bug you linked, but we later changed it in bug 1413574 so that hide() really does hide actions in the urlbar.  (Both bugs landed in 58.  Hidden actions still appear in the menu though.)  So Ian is right that if you want to show an action in the urlbar for all or most pages, you have to use a tabs.onUpdated listener and show() it when your listener is called.
(In reply to Andy McKay [:andym] from comment #3)
> A page action is explicitly designed not to appear all the time, but be
> relevant to the context of the page. In our documentation we say "Use this
> button when a feature is only relevant for some web pages"
> 
> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/user_interface/
> Page_actions
> 
> On this we were following Chrome because that's what they did. However
> there's a couple of things to note:
> * an extension author could call pageAction.show on every page and make it
> appear all the time
> * Firefox already has the pocket and bookmarks icon appear pretty much all
> the time (Chrome shows the bookmark API all the time)
> * in Photon its being changed so that if the pageAction is set to hide() it
> will still appear but be disabled (I think) bug 1395387
> 
> Given the above, I think this makes sense to do. Although I'm cc'ing product
> and UX to ask about this.

As Andy points out, some page actions appear on every page anyway. I'm okay proceeding with this.
Flags: needinfo?(mconca)
This makes sense. 
Screenshots, Send Tab, Bookmark, all are visible on every page, but located in the page action menu, as the are closely related to the shown page. 

This however blurs an already quite blurry separation of page and browser action.
So I think we should update the documentation to better define when to use what:
page_action    -> if action is related to that specific page (no matter how often that is the case)
browser_action -> if action is related to the browser, or many open pages in parallel

examples would be:

type         bookmarks              content            tabs
――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――
page     ||  bookmark this page  |  enhance reddit  |  send tab              |
――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――
browser  ||  show all bookmarks  |  block ads       |  show all synced tabs  |
――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――
Flags: needinfo?(mjaritz)
straw man proposal.

manifest entry for page_action gets a new item, default_show.  It defaults to false.  Setting to true, will make it appear on each tab by default.  You will still be able to hide.
Assignee: nobody → mixedpuppy
For Screenshots default_show would work. I'd add that we want to match the current behavior, which is that this only shows the page action on pages accessible with the <all_urls> permission (e.g., not about: pages). If it also removes the page action for addons.mozilla.org (a blacklisted URL) that would be preferable but not necessary.

This isn't important for Screenshots' use case, but as a general comment: we found that listening for tabs.onUpdated has a substantial effect on overall Firefox performance. If this manifest entry allowed a list of match patterns (instead of just true/false) then something like the Reddit Enhancement Suite (which adds a page action just on reddit.com pages) could handle that declaratively and potentially avoid the performance overhead.

We won't land any changes to Screenshots page action code until this has landed, so we don't require any uplift of a patch.
If we want to avoid extensions having to use something like webNavigation, this seems like a limited solution, why not add something like `"matches": "(match pattern here)"`, then many extensions that use page actions can just ditch webNavigation completely.
Regardless, seems like there should be wider design input on this before we land something and commit to supporting it.
Flags: needinfo?(mixedpuppy)
I think there is a use case for both.  However, right now our common use case is for always show, it's a simple change with zero overhead.  

The alternative is to match against all_urls, which if handled how we generally handle matching, will still require the infrastructure of watching tab/nav changes.  While we would do better than extensions can, we'd still be introducing unnecessary overhead for the majority use case (which IMO is always show).

Match patterns can fit into another bug.
Flags: needinfo?(mixedpuppy)
My concern is that we shouldn't have two different ways to do the same thing, if we do matches, then we don't need default_show since that would be written as `matches: ["<all_urls>"]`.  If we land this then we have to document, test, etc. the behavior when both default_show and matches are present in the manifest.

Also, this patch doesn't appear to handle the request from comment 8.  That particular feature probably does warrant some further thought/discussion.
(In reply to Andrew Swan [:aswan] from comment #12)
> Also, this patch doesn't appear to handle the request from comment 8.  That
> particular feature probably does warrant some further thought/discussion.

(In reply to Ian Bicking (:ianbicking) from comment #8)
> For Screenshots default_show would work. I'd add that we want to match the
> current behavior, which is that this only shows the page action on pages
> accessible with the <all_urls> permission (e.g., not about: pages).

The browser hides all page actions except the bookmark star on about pages that show the branded identity box.  That's these about pages: https://hg.mozilla.org/mozilla-central/file/c2248f853469/browser/base/content/browser.js#l7160
(In reply to Ian Bicking (:ianbicking) from comment #8)
> If it also removes the page action for addons.mozilla.org (a blacklisted URL) that
> would be preferable but not necessary.

addons.mozilla.org prevents content scripts, not page actions. I don't think we should remove page actions on that page.
Screenshots pops up an error if it is activated on an unsupported page, which is fine, so if the action is shown on pages we can't support it's only a minor issue.
I added the [design-decision-needed] flag, but seeing how it was important for screenshots kinda fast tracked it through some decision makers. So moved it to approved, although it was never talked about in a meeting.
Whiteboard: [design-decision-needed] → [design-decision-approved]
Latest patch:

- show_matches and hide_matches added to page_action in the manifest.  These are arrays of match patterns.  
- Matches against hide_matches takes precedence over matches against show_matches.
ok, I'm happy with the test now, try initiated.
Comment on attachment 8932625 [details]
Bug 1419842 support pattern matching to show/hide pageActions,

https://reviewboard.mozilla.org/r/203678/#review210278

::: commit-message-c2248:1
(Diff revision 4)
> +Bug 1419842 add default_show option to page actions manifest, r?aswan

Update the description please

::: browser/components/extensions/ext-pageAction.js:43
(Diff revision 4)
> +    // If <all_urls> is present, the default is to show the page action.  In
> +    // this case we also ignore show_matches since the default show setting
> +    // will be faster.

citation needed?  (about the extra logic here being faster)
This nags at me a little bit since I've seen lots of extensions where the authors apparently don't know about `"<all_urls>"` so they use `"*://*/*"` or even `["http://*/*", "https://*/*"]`, meaning that even if this is measurably faster, it won't actually apply for many extensions.
But the bigger motivation is just having less code...

::: browser/components/extensions/ext-pageAction.js:237
(Diff revision 4)
> +    if (!context.show && context.showMatches && context.showMatches.matches(uri)) {
> +      context.show = true;
> +    }

If you just always create `showMatches` and let it be an empty set if necessary, you can just write this whole clause as
```
context.show ||= context.showMatches(uri);
```
Similarly for the clause below.
Attachment #8932625 - Flags: review?(aswan) → review+
(In reply to Andrew Swan [:aswan] from comment #22)
> Comment on attachment 8932625 [details]
> Bug 1419842 add default_show option to page actions manifest,
> 
> https://reviewboard.mozilla.org/r/203678/#review210278
> 
> ::: commit-message-c2248:1
> (Diff revision 4)
> > +Bug 1419842 add default_show option to page actions manifest, r?aswan
> 
> Update the description please
> 
> ::: browser/components/extensions/ext-pageAction.js:43
> (Diff revision 4)
> > +    // If <all_urls> is present, the default is to show the page action.  In
> > +    // this case we also ignore show_matches since the default show setting
> > +    // will be faster.
> 
> citation needed?  (about the extra logic here being faster)

The extra logic is only in creation of the extension and it's a small amount.  My presumption is that avoiding a pattern match on location change is indeed faster than doing it regardless how small it may be.  Add a half dozen page actions and whatever else is done on location change, it all adds up.
Keywords: dev-doc-needed
Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ece8a84edfa7
support pattern matching to show/hide pageActions, r=aswan
Blocks: 1423396
https://hg.mozilla.org/mozilla-central/rev/ece8a84edfa7
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Is manual testing required on this bug? If Yes, please provide some STR and the proper webextension(if required), if No set the “qe-verify-“ flag.
Flags: needinfo?(mixedpuppy)
Flags: needinfo?(mixedpuppy) → qe-verify-
I've written some docs on show_matches and hide_matches: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/page_action.

Also included something on the difference between page actions and browser actions, based on comment 6: /en-US/Add-ons/WebExtensions/user_interface/Page_actions#Page_actions_and_browser_actions.

Please let me know if this covers it.
Flags: needinfo?(mixedpuppy)
lgtm
Flags: needinfo?(mixedpuppy)
Depends on: 1437178
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: