Closed Bug 1413574 Opened 7 years ago Closed 7 years ago

Disabled page actions should be hidden

Categories

(Firefox :: Toolbars and Customization, defect, P1)

Product:
Firefox
Component:
Toolbars and Customization
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 58
Tracking Flags:
Tracking Status
firefox57 --- unaffected
firefox58 --- fixed

People

(Reporter: ntim, Assigned: adw)

References

Details

(Whiteboard: [reserve-photon-structure])

Attachments

(2 files)

Bug 1413574 - Hide disabled page actions and offer a "Manage Extension" command for actions in extensions.
59 bytes, text/x-review-board-request
Gijs
: review+
Details
Request for Data Collection Review
152.47 KB, application/pdf
rweiss
: review+
Details 
This matches the old behaviour. Most add-ons were designed with that old behaviour in mind, so it makes sense to keep it IMO. An example of such add-on is "Extension source viewer" [0], which only shows its button on extension download pages.


[0]: https://addons.mozilla.org/en-US/firefox/addon/crxviewer/
We decided in bug 1395387 though to disable them instead of hiding them.  It matches what Chrome does, but more importantly, the user can hide/show actions in the urlbar as they'd like (via the context menu), so it would be confusing for the user if actions came and went by themselves too.  So this is almost certainly a wontfix.
... but I just realized that disabled icons look the same as enabled icons for non-svg non-Mozilla-signed extensions... which is a big problem.  Hmm.
(In reply to Drew Willcoxon :adw from comment #1)
> It matches what Chrome does

That's chrome's behaviour after Chrome 48... when Google decided to deprecate page actions by making them behave exactly like browser actions (there was a blog post about this, but I can't seem to find it anymore). I did find this though: https://productforums.google.com/forum/#!msg/chrome/wOUFbsKqPg0/f4jJ2K9bCgAJ . I do remember a blog post mentioning the deprecation though.

Before Chrome 48, disabled page actions were simply hidden.


So a lot of extensions are designed with that in mind, I can cite a few examples out of many:
- Ink for Google (initially a chrome extension!) is the extension that themes Google services and it provides a page action that only showed on the themed pages to disable the theming on a specific service if needed. With the new behaviour, the page action button will always display, which is a waste of space most of the time.

- Extension source viewer used to only show its page actions on extension download pages, now it shows all time, which is quite annoying.

Maybe the old behaviour could be preserved on urlbar buttons, and then the menu item could always be visible ? Space in the urlbar is quite valuable, and it's the behaviour users are used to.
Aaron, any thoughts on this bug?  Please see Tim's comments.
Flags: needinfo?(abenson)
* Contextual visibility is the main feature of page actions, removing it makes them functionally identical to browser actions which undermines the work done in Photon to better handle them.

* Showing irrelevant page actions in the address bar is confusing and clutters the browser chrome. eg Reddit Enhancement Suite has a page action that disables custom CSS on reddit.com that serves no purpose being visible anywhere else.

* The built-in reader mode and page zoom page actions appear only when needed, this should be possible with extensions.

* Page actions in the address bar indicate an action is available, if the user removes it then this is no longer possible without first opening the menu. CanvasBlocker uses its page action as a notification when the current page has done something.

* The user could remove all page actions but this makes them harder to access when they are relevant for the current site.

* Disabled page action icons don't look disabled aside from the hover state, using opacity can still be ambiguous and the extension's context menu items still appear.

* If installed page actions must all be shown somewhere, they can be shown in the menu where clutter is less of an issue.
Extension source viewer has a "Hide this button" context menu item on the page action and a setting that hides the button.

Now, when you click the "Hide this button" context menu item on the page action, the page action simply gets disabled which is very confusing.
( also reported at https://github.com/Rob--W/crxviewer/issues/51 )

Disabled page actions should be hidden.

I haven't read through all comments in bug 1395387, but as a user and add-on dev I don't want to see page actions when they are not activated. Some page actions are only useful on a very limited set of websites, and showing the pageaction on other websites makes no sense and only clutters the view.

The "new" design (and also Chrome's design) suffers from a usability issue, namely that a user cannot choose to only show the page action on relevant websites. They either have to completely hide the button, or always show it.

When Chrome started treating page actions visually identical to browser actions, there was much outcry among users and extension developers.
Chromium's dev team even acknowledges that the UX design is suboptimal and needs to be revisited: https://bugs.chromium.org/p/chromium/issues/detail?id=597657
Opera is a fork of Chromium, but they still maintain code to render page actions inside the location bar.

Can the page action be hidden again if when the page action is hidden?
Flags: needinfo?(abenson)
Aaron, I think we could go back to the old behavior if we simply change the wording of the context menu, and possibly have different context menus for the panel and for the urlbar.  Currently we have:

(1) Add to Address Bar
(2) Remove from Address Bar

We could replace the idea of "adding" with "showing" or "pinning":

(1) Show in Address Bar When Active
    Show in Address Bar When Relevant
    Pin When Active
    Pin When Relevant
(2) Never Show in Address Bar
    Never Pin

Or instead of having a "remove" at all, we could just use a checkmark menu item.

The problem with (2) though is that you want it to read as an affirmative action when you invoke it in the urlbar.  So for the urlbar, we could still say "remove" or "hide" or "unpin".

Here's what I like.  For the panel, a single "Pin When Relevant" checkmark item.  For the urlbar, a single "Unpin", and that's all you need there.

"Pin" is more concise than "add to address bar", and it's analogous to "pinning" tabs.  We already use that word in the product for that concept.  (But that's not the hill I want to die on.  I more like the idea of two different context menus and a checkmark item for the panel one.)
Flags: needinfo?(abenson)
I think we may still want "show" to map to "disable" in the panel though, so that the items in the panel do not change and only become disabled.
Assignee: nobody → adw
Status: NEW → ASSIGNED
Priority: -- → P1
Whiteboard: [photon-structure][triage] → [reserve-photon-structure]
Work in progress.  I kept the concept of disabled actions but changed the implementation so that they're hidden in the urlbar.  They remain disabled (grayed out) in the panel.  I also renamed the "shown in urlbar" concept to "pin to urlbar."  I used the "Pin When Relevant" and "Unpin" strings that I mentioned in my previous comment, but I'll wait for Aaron on wording, and also the checkmark context menu item in the panel.  Need to update tests still and figure out one or two better ways to do things.
Here are try builds in case anyone wants: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bafd4c420abd

It works afaict, but there may be bugs.
This is ready to review, but I still need Aaron's comments.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd438ef8ef01
Note to self, I'll need to update the test I'm adding in bug 1413692, assuming that lands first.
This rebases on the current tree and bug 1413692, including updating the test I mentioned in comment 16.  It should also fix the test failures on try.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d02125c0365b
I just talked with Aaron and Bryan.  We decided to treat built-in actions a little differently from extension actions, while preserving the old behavior of extension actions.

Built-in actions will behave just like they do now: "Add to Address Bar" and "Remove from Address Bar."  The built-in actions are mostly not transient, meaning they're either all shown (for most pages) or all hidden (e.g., on about: pages) in the urlbar.

Extension actions will behave just like they used to.  When an extension show()s itself, it's shown in the urlbar, and when it hide()s itself, it's hidden in the urlbar.  To stop the extension from ever showing itself in the urlbar again, the user uninstalls it.

Extension actions will still appear in the panel.  When an extension hide()s itself, its menu item in the panel still appears, but it's disabled.

The context menu for an extension action takes the user to the extension's info in about:addons.
Flags: needinfo?(abenson)
Gijs, please see comment 20 for a high-level explanation of the main goal of this change.  I'll try to explain how this works in the patch.

To recap from bug 1395387, there are two different ways for actions to be "shown" and "hidden" in the urlbar.  (1) The user can show/hide them (currently only via the context menu), and (2) webextension actions can show/hide themselves via the webextension API.  To make this easier to talk about, I've been referring to (1) as "pinning" and to (2) as "disabling."

This patch renames the shownInUrlbar property to pinnedToUrlbar, and it changes what both it and `disabled` mean.  An action is truly shown in the urlbar only if it's both pinned and not disabled.  If the user hasn't pinned it, then it's never shown; if the extension disabled it, then it's not shown.

Now, Bryan pointed out that we can simplify the UX around this.  Extension actions should just always be pinned.  The user made a choice to install them, effectively pinning them.  And built-in actions are mostly always enabled or disabled together.  (e.g., they're all hidden on about: pages, mostly.)  So really, disabled state only pertains to extension actions, and pinned state only pertains to built-in actions.  Therefore we can keep the "Add/Remove to/from Address Bar" context menu for built-in actions, and there's no context menu needed at all for extension actions since disabling happens via the webextension API -- except we provide a "Manage Extension" context menu, which takes you to the about:addons section for the extension.

But I've kept the ability in the PageActions API to separately pin and disable any action.

Other changes this makes:

* Changed PageActions.actionsInUrlbar from a simple getter to a method that takes a browser window, since the actions in the urlbar depend on their disabled state, and their disabled state can be per-window

* Added panelButtonNodeForActionID and urlbarButtonNodeForActionID methods to BrowserPageActions so that you can get a node in one call instead of first getting its DOM ID and then getting the node

* Simplified context-menu handling by removing the `contextmenu` listener and instead doing what it did directly inside the context menu popupshowing listener

* Added an `extensionID` property to actions so I can open their about:addon pages

* Added a `_isBuiltIn` property to actions so I can determine which context menu to show for them

* Added a new FX_PAGE_ACTION_MANAGED telemetry probe, to go along with the existing ADDED and REMOVED probes
> The user made a choice to install them, effectively pinning them.  

I think it's nice to be able to remove them from the Address bar. Sometimes I need the page action, but I don't want it to appear inside the addressbar. Also, some add-ons come with a page action that you may not need.
(In reply to Tim Nguyen :ntim from comment #25)
> > The user made a choice to install them, effectively pinning them.  
> Also, some add-ons come with a page action that you may not need.

I agree with that. Extensions like RES (Reddit Enhancement Suite) do a lot of things, but the page action only controls one little feature, and the functionality of the page action is also available in-content. For users with mutliple extension provided page actions (and maybe even small screens) being able to unpin page actions could increase the efficiency of using them a lot (like it does for the built in ones, where you can remove and add things for quick access, but you may still use the other features occasionally).
I tried out the latest patch (the build from comment 24) and observed that the page action is visible in the triple-dot menu next to the locationbar, even if the add-on has hidden the page action.

Please do not show the pageaction (in the location bar / the triple-dot menu) unless the add-on has requested the page action to be shown.
Otherwise if a user has many page_action add-ons, then that menu will be cluttered with page actions that are not relevant for the given context (page).
(In reply to Rob Wu [:robwu] from comment #27)
> Please do not show the pageaction (in the location bar / the triple-dot
> menu) unless the add-on has requested the page action to be shown.

We're currently re-evaluating the newest patch in light of comment 25 and comment 26, but I do think we're all pretty firm on showing all actions in the menu.
(In reply to Drew Willcoxon :adw from comment #28)
> (In reply to Rob Wu [:robwu] from comment #27)
> > Please do not show the pageaction (in the location bar / the triple-dot
> > menu) unless the add-on has requested the page action to be shown.
> 
> We're currently re-evaluating the newest patch in light of comment 25 and
> comment 26, but I do think we're all pretty firm on showing all actions in
> the menu.

For users and devs who value minimal clutter, showing all actions is a negative incentive to build or install add-ons with page actions:

Page actions are opt-in UI elements. If you decide to unconditionally show page actions, then there is an associated cost with using the page action. If the potential benefit of showing the menu (e.g. good UX in an obscure situation) does not outweigh the perceived negative effects (clutter), then an add-on would not ship with a page action, and the resulting add-on is not as good as it could have been.

What makes page actions so important that they have to be shown unconditionally if an add-on declares one, even when it is not shown 99.99999% of the time?
But they're not shown unconditionally, at least not after this bug is fixed.  You have to click the "..." button to show them.

(In reply to Rob Wu [:robwu] from comment #29)
> What makes page actions so important that they have to be shown
> unconditionally if an add-on declares one, even when it is not shown
> 99.99999% of the time?

The "..." button is *the page action button*.

Do other people in this bug agree with Rob?
(In reply to Drew Willcoxon :adw from comment #30)
> But they're not shown unconditionally, at least not after this bug is fixed.
> You have to click the "..." button to show them.
> 
> (In reply to Rob Wu [:robwu] from comment #29)
> > What makes page actions so important that they have to be shown
> > unconditionally if an add-on declares one, even when it is not shown
> > 99.99999% of the time?
> 
> The "..." button is *the page action button*.

Thanks for this statement, I think that we misunderstood each other here.

The "..." is indeed the page action button (of Firefox).
But when "..." is clicked, the "page action" from an extension (declared through page_action in the extension's manifest.json) shows up, even if pageAction.show() was NOT called. Hence, hidden page actions are shown unconditionally in the UI (not "in the location bar", but in "the triple-dot menu").

With this clarification, is my remark "Page actions are opt-in UI elements. (...)" from comment 29 more clear?


> Do other people in this bug agree with Rob?

To be explicit, the requested change of the patch that we'd be voting on is:

"When an extension hides its "page_action", the extension's item should not appear in the triple-dot menu next to the location bar."



To test:
1. Download and run a Firefox build with the patch from comment 24.
2. Install from AMO: https://addons.mozilla.org/en-US/firefox/addon/crxviewer/
3. On AMO a yellow CRX button should be seen next to the location bar.
4. Switch to a different tab (e.g. mozilla.org)
5. In that different tab, the CRX button should be hidden.
6. In the triple-dot "page action" button, the extension's action item should not appear.

With the current patch, step 6 fails, a menu item from the extension appears (greyed out, but still visible!).
> Do other people in this bug agree with Rob?

I agree. IMO only one place where inactive page actions can be showed without negative attitude is "Customize Firefox".
The Page Action Menu was meant to serve as a directory of the page action extensions that are currently installed. We should make sure to show disabled states as clearly as possible, however, *not* showing extensions in the menu makes little sense because there's no other more obvious place to look for them. I'd argue that it is clearer to show a disabled extension than none at all and risk people being confused about where to look. 

The attached modification makes it easy for people to remove a page action extension if they don't want it in the Address Bar but optimizes for the extensions usage by placing it in the Address Bar by default. 

https://mozilla.invisionapp.com/share/4VEDARP7H#/screens/262895766_Explainer_-_Page_Action_Items
OK, this is ready for review again and implements the UI in the mockup in comment 33.   The only changes from the previous patch have to do with the context menu.  Now that the context menu has three items for extension actions (including the separator), it seems better to define all the menu items in markup and then hide/show them in CSS, instead of building the menu in JS.  (It was probably better to do that anyway.)
We have a slight change in wording for the context menu of extension actions.  That's all the latest changeset is.
Forgot to update the test for the wording change.
Comment on attachment 8925162 [details]
Bug 1413574 - Hide disabled page actions and offer a "Manage Extension" command for actions in extensions.

https://reviewboard.mozilla.org/r/196406/#review203688

Looks good to me, thanks!

::: browser/base/content/browser-pageActions.js:98
(Diff revision 8)
>      let id = this.panelButtonNodeIDForActionID(action.id);
>      let node = document.getElementById(id);

This can also use `panelButtonNodeForActionID()`.

::: browser/base/content/browser-pageActions.js:353
(Diff revision 8)
>          }
>        }
>        return;
>      }
>  
> +    let insertBeforeID = PageActions.nextActionIDInUrlbar(window, action);

Here and in `placeActionInPanel`, we can fetch the insertBeforeID a bit later still (that is, right before using it).

::: browser/base/content/browser-pageActions.js:486
(Diff revision 8)
> -    let nodeIDs = [
> -      this.panelButtonNodeIDForActionID(action.id),
> -      this.urlbarButtonNodeIDForActionID(action.id),
> +    let nodes = [
> +      this.panelButtonNodeForActionID(action.id),
> +      this.urlbarButtonNodeForActionID(action.id),
>      ];
> -    for (let nodeID of nodeIDs) {
> +    for (let node of nodes) {

Nice. Might as well do:

```js
let nodes [
...
].filter(n => !!n);
```

and then you can reduce the nesting in the for loop because you don't need to have the individual if statement checking if node is non-null anymore.

::: browser/base/content/browser-pageActions.js:543
(Diff revision 8)
>        return;
>      }
>      PageActions.logTelemetry("used", action, buttonNode);
>      // If we're in the panel, open a subview inside the panel:
>      // Note that we can't use this.panelNode.contains(buttonNode) here
>      // because of XBL boundaries breaking ELement.contains.

Drive-by nit: `Element`

::: browser/modules/PageActions.jsm:254
(Diff revision 8)
> +          action.id == ACTION_ID_BOOKMARK ? -1 :
> +          this._persistedActions.idsInUrlbar.indexOf(ACTION_ID_BOOKMARK);

Nit: Indenting here feels strange to me. Not sure how best to fix...
Attachment #8925162 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to Aaron Benson from comment #33)
> The Page Action Menu was meant to serve as a directory of the page action
> extensions that are currently installed. We should make sure to show
> disabled states as clearly as possible,

"disabled" as in disabled by the user, or disabled by an extension?

If disabled by the user (="Don't Show in Address bar"), I agree with showing the item, because the menu item is relevant for the context.

If disabled by the extension, then I disagree with prominently showing disabled extension action items, because of previously mentioned reasons (clutter in page action menu by non-relevant extension action items).

If you insist on featuring the non-relevant menu items in the page action menu, how about moving these non-relevant items to a separate menu that can be accessed from the main page action menu? This would address both my concern of clutter, and your wish to show as much information as possible.


> however, *not* showing extensions in
> the menu makes little sense because there's no other more obvious place to
> look for them. I'd argue that it is clearer to show a disabled extension
> than none at all and risk people being confused about where to look.

Under which scenario does a user want to see a page action that is not relevant for the context...?
You seem to think that there is value in showing as many extension action items as possible, as if the mere act of declaring page_action in an extension's manifest.json implies that the extension always has a relevant action for the Page action menu.
I disagree with that view, because most (if not all) page action extensions that I use/build are only relevant for specific contexts (often sites), and showing them in the menu on another site does not add any value; it only increases the (mental) efforts to find the relevant action items.

Here is a similar example from Chrome, which shows icons for all extensions. Consequently, finding the relevant page action amidst 23 icons is unnecessarily complicated (find the icon with the blue dot): https://imgur.com/a/sjqAx
Thanks Gijs!

(In reply to :Gijs (slow, PTO recovery mode) from comment #41)
> ::: browser/base/content/browser-pageActions.js:98
> (Diff revision 8)
> >      let id = this.panelButtonNodeIDForActionID(action.id);
> >      let node = document.getElementById(id);
> 
> This can also use `panelButtonNodeForActionID()`.

`id` is used below in this chunk, to give the newly created node an ID.  So there isn't any benefit to using panelButtonNodeForActionID, so I left this one as is.

> ::: browser/base/content/browser-pageActions.js:353
> (Diff revision 8)
> >          }
> >        }
> >        return;
> >      }
> >  
> > +    let insertBeforeID = PageActions.nextActionIDInUrlbar(window, action);
> 
> Here and in `placeActionInPanel`, we can fetch the insertBeforeID a bit
> later still (that is, right before using it).

Yes, I think you're right.

> ::: browser/base/content/browser-pageActions.js:486
> (Diff revision 8)
> > -    let nodeIDs = [
> > -      this.panelButtonNodeIDForActionID(action.id),
> > -      this.urlbarButtonNodeIDForActionID(action.id),
> > +    let nodes = [
> > +      this.panelButtonNodeForActionID(action.id),
> > +      this.urlbarButtonNodeForActionID(action.id),
> >      ];
> > -    for (let nodeID of nodeIDs) {
> > +    for (let node of nodes) {
> 
> Nice. Might as well do:
> 
> ```js
> let nodes [
> ...
> ].filter(n => !!n);
> ```

Done

> ::: browser/base/content/browser-pageActions.js:543
> (Diff revision 8)
> >        return;
> >      }
> >      PageActions.logTelemetry("used", action, buttonNode);
> >      // If we're in the panel, open a subview inside the panel:
> >      // Note that we can't use this.panelNode.contains(buttonNode) here
> >      // because of XBL boundaries breaking ELement.contains.
> 
> Drive-by nit: `Element`

Fixed

> ::: browser/modules/PageActions.jsm:254
> (Diff revision 8)
> > +          action.id == ACTION_ID_BOOKMARK ? -1 :
> > +          this._persistedActions.idsInUrlbar.indexOf(ACTION_ID_BOOKMARK);
> 
> Nit: Indenting here feels strange to me. Not sure how best to fix...

I changed this to:

        index = action.id == ACTION_ID_BOOKMARK ? -1 :
                this._persistedActions.idsInUrlbar.indexOf(ACTION_ID_BOOKMARK);
Pushed by dwillcoxon@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/777cfa8b3fd9
Hide disabled page actions and offer a "Manage Extension" command for actions in extensions. r=Gijs
Rob, thanks for your comments in this bug.  We're rushing a bit to fix this bug by the 58 merge day, this Monday.  Especially since 58 is the first release where Photon page actions and webextension page actions are integrated.  We just don't have time right now to evaluate, fix, and get reviewed another change like the one you're suggesting.

The UX team seems pretty firm right now on showing all actions in the menu.  But again, this will only be the first release where extension actions are even in the menu at all.  Things can change, as this bug shows.
https://hg.mozilla.org/mozilla-central/rev/777cfa8b3fd9
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox58: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
In rushing to have this merged, it appears as though you may have forgotten to request Data Collection Review[1] for the new Telemetry probe you added (FX_PAGE_ACTION_MANAGED).

I am not a Data Peer, but this collection looks like it ought to be fine. Please gain data collection review for this change ASAP.

[1]: https://wiki.mozilla.org/Firefox/Data_Collection
Flags: needinfo?(adw)
Thanks Chris.
Rebecca, this histogram is an extension of the histograms added in bug 1393843.
Flags: needinfo?(adw)
Attachment #8928012 - Flags: review?(rweiss)
Comment on attachment 8928012 [details]
Request for Data Collection Review

1. This is a telemetry histogram, and so will have standard documentation included.
2. Yes, it will obey data preferences for telemetry.
3. Not permanent data collection
4. These are type 2 measurements (measuring interaction with the browser)
5. Unclear, but since this is type 2, this measurement can be default on in release if the requesters desire.
6. No new identifiers
7. Covered by the privacy policy
8. Yes, this is a renewing probe.  This will require additional signoff if we want to make this measurement permanent, but renewal should not require secondary review.
Attachment #8928012 - Flags: review?(rweiss) → review+
Depends on: 1435992
No longer depends on: 1435992
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: