1006989 - The Bookmarks Toolbar Items context shouldn't have "Cut", Copy", "Delete", or "Properties" when right-clicking on a blank area

Status: VERIFIED FIXED

(Firefox :: Menus, defect)

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: WIP Patch

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch (rebased)

Rebased this patch on top of the patch in bug 1003364.

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8419196 [details] [diff] [review]
Patch (rebased)

Redirecting to mak, simply because I'm not at all familiar with this Places stuff.

If mak is overloaded, I can come back, learn it, and check it out.

Comment 4

[Marco Bonardo [:mak]]

Comment on attachment 8419196 [details] [diff] [review]
Patch (rebased)

I think in the past we have a discussion regarding whether hide or disable context menu options and the decision was to disable to avoid breaking users muscle memory. Though I may be wrong, so setting a feedback to Mano, he may remember.

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

These are the simple rules for determining disabling vs. hiding:

- Disable the menuitem if the current state of the item being clicked on provides that this option is not available at the moment.
- Hide the menuitem if item being clicked on will never provide this ability.

In this case, right-clicking on a blank area never will provide the ability to cut, copy, or delete. Therefore, we should hide these menuitems.

Comment 6

[Asaf Romano (gone)]

Comment on attachment 8419196 [details] [diff] [review]
Patch (rebased)

I think that the no-selection case is special enough for a "different" context menu. So, in that regard, feedback+. However, my feedback for ux stuff is all but meaningless...

I do, however, have some concerns:
1) We should make sure that when all commands are disabled and/or hidden for the current selection, the menu doesn't show up at all. A completely disabled menu is bad. A little empty popup is terrible. For instance, with no History available for Today, open Today's history query in the library and right click the "root". You'll notice: (a) there's a bug that keeps "Open All in Tabs" enabled for some reason; (b) if that bug wasn't there, you'd get a completely disabled menu. That's not nice, I think. With this patch, we can get quite close to an *empty* menu (Open In Tabs is still un-hidden, but we shouldn't rely on this).
2) Anything that's hidden should be disabled. Otherwise keyboard shortcuts may still work.
3) Because all of Firefox's own places views hide the root node, this patch severs its purpose. However, it'd be nice to rely on something else. I think that it'd be better to pass no selection when nothing is selected rather than the root. This may require some extra work, so I'm not going to insist on this.

Comment 7

[Marco Bonardo [:mak]]

(In reply to Mano from comment #6)
> However, it'd be nice to rely on something else. I think
> that it'd be better to pass no selection when nothing is selected rather
> than the root. This may require some extra work, so I'm not going to insist
> on this.

Regarding this, we have bug 958848 that is somehow related, there again the only selected item is the root node of the popup, if I read the bug correctly.
So the solution here (and there) may be to make the selection return nothing if the root node is selected and when there is no selection we could disable and hide cut and copy.
Do I get correctly what you are suggesting?

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Marco, can you please add this to the backlog for this iteration as it is carry-over from before we started the new process but is close to completion.

Comment 9

[Marco Mucci [:MarcoM]]

Added to Iteration 32.3.  Please provide a point estimate.

Comment 10

[Asaf Romano (gone)]

(In reply to Marco Bonardo [:mak] from comment #7)
> (In reply to Mano from comment #6)
> > However, it'd be nice to rely on something else. I think
> > that it'd be better to pass no selection when nothing is selected rather
> > than the root. This may require some extra work, so I'm not going to insist
> > on this.
> 
> Regarding this, we have bug 958848 that is somehow related, there again the
> only selected item is the root node of the popup, if I read the bug
> correctly.
> So the solution here (and there) may be to make the selection return nothing
> if the root node is selected and when there is no selection we could disable
> and hide cut and copy.
> Do I get correctly what you are suggesting?

Yep.

Comment 11

[Marco Bonardo [:mak]]

Comment on attachment 8419196 [details] [diff] [review]
Patch (rebased)

Review of attachment 8419196 [details] [diff] [review]:
-----------------------------------------------------------------

I think Mano's idea is much better as it's a little bit more general than a new special annotation and will serve us better in future. Should be matter of changing the views to return an empty selection when the root node is selected and change the context menu when the selection is empty.

Comment 12

[Marco Mucci [:MarcoM]]

Hi Jared, can you recommend if this bug should be [qa+] or [qa-] for QA verification.

Comment 13

[Asaf Romano (gone)]

qa+

Comment 14

[Marco Mucci [:MarcoM]]

Removed from Iteration 32.3

Comment 15

[Marco Mucci [:MarcoM]]

Added to Iteration 32.3

Comment 16

[Marco Mucci [:MarcoM]]

Apologies, I modified the wrong bug.

Comment 17

[:Gijs (he/him)]

Marco, can you add this back in my name? :-)

Comment 18

[Marco Mucci [:MarcoM]]

Added to Iteration 32.3

Comment 19

[:Gijs (he/him)]

Attachment: hide and disable cut/copy/delete/properties when opening bookmarks context menu with no selection,

AIUI this is what is desired here. Fixing up the 'open all in tabs' item to be disabled if the container has no openable children should be a separate issue, IMO. Until that time, AFAICT there is no way currently for the menu to be literally empty.

Comment 20

[Marco Bonardo [:mak]]

Comment on attachment 8434978 [details] [diff] [review]
hide and disable cut/copy/delete/properties when opening bookmarks context menu with no selection,

Review of attachment 8434978 [details] [diff] [review]:
-----------------------------------------------------------------

I'm forwarding this to Mano cause I can't handle it very shortly

Comment 21

[Asaf Romano (gone)]

Comment on attachment 8434978 [details] [diff] [review]
hide and disable cut/copy/delete/properties when opening bookmarks context menu with no selection,

Review of attachment 8434978 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/places/content/browserPlacesViews.js
@@ +122,5 @@
>    selectAll: function() { },
>  
>    get selectedNode() {
>      if (this._contextMenuShown) {
> +      let anchor = this._contextMenuShown.triggerNode;

This by itself is just cleanup, right?

@@ +124,5 @@
>    get selectedNode() {
>      if (this._contextMenuShown) {
> +      let anchor = this._contextMenuShown.triggerNode;
> +      if (anchor._placesNode) {
> +        return this._rootElt == anchor ? null : anchor._placesNode;

It'd be better to avoid all the _rootElt mess altogether. If the root isn't selectable, there should be not root that has it set in _placesNode.

::: browser/components/places/content/controller.js
@@ +538,5 @@
> +        return false;
> +      if (count == 1 && selectionTypes.indexOf("single") == -1)
> +        return false;
> +      // NB: if there is no selection, we show the item if and only if
> +      // the selectiontype includes 'none' - the metadata list will be

The comment is somewhat confusing, because the item is also shown when the selectiontype attribute is not set at all. I wonder if we should keep the "default" as "single|multiple", and force an opt-in of the "none" case, because it always requires special care.

On top of that, |selection="any"| should probably ensure there's something is selected. Currently it returns true, that's why this patch does not hide the "New *" items. They should be non-hidden indeed, but not for this reason (you probably want to remove the "selection" attribute of these items).

::: browser/components/places/content/placesOverlay.xul
@@ +117,1 @@
>                selection="folder|host|query"/>

Open In Tabs does not actually work with this patch. The implementation expects a selection.

This one is a little bit tricky. First, because it's unclear which "Open In Tabs" should be used for the no-selection case (there's one for multiple selection here). And, secondly, because it should only show up if there is more than one item to open.

This also means that in some cases the menu is going to be empty (e.g. in a History container that contains has a single url item). Please verify that in these cases the menu doesn't open at all (popupshowing should prevent-default/return false).

Comment 22

[:Gijs (he/him)]

(In reply to Mano (needinfo? for any questions; not reading general bugmail) from comment #21)
> Comment on attachment 8434978 [details] [diff] [review]
> hide and disable cut/copy/delete/properties when opening bookmarks context
> menu with no selection,
> 
> Review of attachment 8434978 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/places/content/browserPlacesViews.js
> @@ +122,5 @@
> >    selectAll: function() { },
> >  
> >    get selectedNode() {
> >      if (this._contextMenuShown) {
> > +      let anchor = this._contextMenuShown.triggerNode;
> 
> This by itself is just cleanup, right?

Yes.

> @@ +124,5 @@
> >    get selectedNode() {
> >      if (this._contextMenuShown) {
> > +      let anchor = this._contextMenuShown.triggerNode;
> > +      if (anchor._placesNode) {
> > +        return this._rootElt == anchor ? null : anchor._placesNode;
> 
> It'd be better to avoid all the _rootElt mess altogether. If the root isn't
> selectable, there should be not root that has it set in _placesNode.

I don't follow what you mean here.

> ::: browser/components/places/content/controller.js
> @@ +538,5 @@
> > +        return false;
> > +      if (count == 1 && selectionTypes.indexOf("single") == -1)
> > +        return false;
> > +      // NB: if there is no selection, we show the item if and only if
> > +      // the selectiontype includes 'none' - the metadata list will be
> 
> The comment is somewhat confusing, because the item is also shown when the
> selectiontype attribute is not set at all. I wonder if we should keep the
> "default" as "single|multiple", and force an opt-in of the "none" case,
> because it always requires special care.
> 
> On top of that, |selection="any"| should probably ensure there's something
> is selected. Currently it returns true, that's why this patch does not hide
> the "New *" items. They should be non-hidden indeed, but not for this reason
> (you probably want to remove the "selection" attribute of these items).
> 
> ::: browser/components/places/content/placesOverlay.xul
> @@ +117,1 @@
> >                selection="folder|host|query"/>
> 
> Open In Tabs does not actually work with this patch. The implementation
> expects a selection.

That's odd, I remember testing this and it working. I'll have another look.

> This one is a little bit tricky. First, because it's unclear which "Open In
> Tabs" should be used for the no-selection case (there's one for multiple
> selection here). And, secondly, because it should only show up if there is
> more than one item to open.

This already doesn't work before the patch (e.g. right clicking on a folder with 1 bookmark in it shows it enabled, and clicking it opens that 1 item in a tab). As I noted in comment 19, I think this is a separate issue and would prefer not to try to fix that in this patch.

> This also means that in some cases the menu is going to be empty (e.g. in a
> History container that contains has a single url item). Please verify that
> in these cases the menu doesn't open at all (popupshowing should
> prevent-default/return false).

Ditto.

Comment 23

[:Gijs (he/him)]

Needinfo for comment 22...

Comment 24

[Asaf Romano (gone)]

(In reply to :Gijs Kruitbosch from comment #22)

> > >    get selectedNode() {
> > >      if (this._contextMenuShown) {
> > > +      let anchor = this._contextMenuShown.triggerNode;
> > > +      if (anchor._placesNode) {
> > > +        return this._rootElt == anchor ? null : anchor._placesNode;
> > 
> > It'd be better to avoid all the _rootElt mess altogether. If the root isn't
> > selectable, there should be not root that has it set in _placesNode.
> 
> I don't follow what you mean here.
> 

_rootElt could be either the bookmarks toolbar or a <menupoup>. This was done in order to support selection of the root node. Now that it's unsupported, _placesNode shouldn't be set for these nodes. Then, anchor._placesNode would be unset and you wouldn't have to do:

return this._rootElt == anchor ? null : anchor._placesNode;

> > 
> > ::: browser/components/places/content/placesOverlay.xul
> > @@ +117,1 @@
> > >                selection="folder|host|query"/>
> > 
> > Open In Tabs does not actually work with this patch. The implementation
> > expects a selection.
> 
> That's odd, I remember testing this and it working. I'll have another look.
> 

Not for me, and the code suggests it wouldn't.

> > This one is a little bit tricky. First, because it's unclear which "Open In
> > Tabs" should be used for the no-selection case (there's one for multiple
> > selection here). And, secondly, because it should only show up if there is
> > more than one item to open.
> 
> This already doesn't work before the patch (e.g. right clicking on a folder
> with 1 bookmark in it shows it enabled, and clicking it opens that 1 item in
> a tab). As I noted in comment 19, I think this is a separate issue and would
> prefer not to try to fix that in this patch.
> 

I would consider the no-selection case more similar to the the "select all" case, which works as it should. The issues are in fact, independent, because in your case you've the data handy. I would thus prefer not to introduce another bug here, even though there are similar bugs.

> > This also means that in some cases the menu is going to be empty (e.g. in a
> > History container that contains has a single url item). Please verify that
> > in these cases the menu doesn't open at all (popupshowing should
> > prevent-default/return false).
> 
> Ditto.

How so? If Open in Tabs is hidden, the menu could be empty. This situation wasn't possible until now.

Comment 25

[:Gijs (he/him)]

(In reply to Mano (needinfo? for any questions; not reading general bugmail) from comment #24)
> (In reply to :Gijs Kruitbosch from comment #22)
> 
> > > >    get selectedNode() {
> > > >      if (this._contextMenuShown) {
> > > > +      let anchor = this._contextMenuShown.triggerNode;
> > > > +      if (anchor._placesNode) {
> > > > +        return this._rootElt == anchor ? null : anchor._placesNode;
> > > 
> > > It'd be better to avoid all the _rootElt mess altogether. If the root isn't
> > > selectable, there should be not root that has it set in _placesNode.
> > 
> > I don't follow what you mean here.
> > 
> 
> _rootElt could be either the bookmarks toolbar or a <menupoup>. This was
> done in order to support selection of the root node. Now that it's
> unsupported, _placesNode shouldn't be set for these nodes. Then,
> anchor._placesNode would be unset and you wouldn't have to do:
> 
> return this._rootElt == anchor ? null : anchor._placesNode;

Not setting _rootElt._placesNode or popup._placesNode breaks the world. (e.g. folders and live bookmarks on the bookmarks toolbar don't open, etc.)

Comment 26

[:Gijs (he/him)]

Attachment: hide and disable cut/copy/delete/properties when opening bookmarks context menu with no selection,

So as noted, removing the root element's placesnode breaks lots of things (also some automated tests) and so I've left that as-is. I believe all the other issues you highlighted should be fixed in this version of the patch.

Comment 27

[:Gijs (he/him)]

(the removal of rootIsSelected is also cleanup - it was unused already, in fact, but now it can never even be true, so...)

Comment 28

[Asaf Romano (gone)]

Comment on attachment 8441312 [details] [diff] [review]
hide and disable cut/copy/delete/properties when opening bookmarks context menu with no selection,

Review of attachment 8441312 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/places/content/browserPlacesViews.js
@@ +125,5 @@
>      if (this._contextMenuShown) {
> +      let anchor = this._contextMenuShown.triggerNode;
> +      if (!anchor) {
> +        return null;
> +      }

nit: in this file, don't wrap single-line blocks in brackets

Comment 29

[:Gijs (he/him)]

w/ nits:

remote:   https://hg.mozilla.org/integration/fx-team/rev/5d467596b176

Comment 30

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5d467596b176

Comment 31

[Marco Mucci [:MarcoM]]

Hi Florin, can a QA contact be assigned for verification of this bug.

Comment 32

[Cornel Ionce [:noni] [Hubs QA]]

User Agents:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:33.0) Gecko/20100101 Firefox/33.0
Mozilla/5.0 (X11; Linux i686; rv:33.0) Gecko/20100101 Firefox/33.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:33.0) Gecko/20100101 Firefox/33.0

"Cut", Copy", "Delete", or "Properties" are no longer displayed when right-clicking on a blank area (Bookmarks toolbar) using latest Nightly, build ID: 20140713030204 on Windows 7 64bit, Ubuntu 14.04 and Mac OS X 10.9.3.