Closed Bug 1006989 Opened 8 years ago Closed 7 years ago

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

Categories

(Firefox :: Menus, defect)

defect
Not set
normal
Points:
3

Tracking

()

VERIFIED FIXED
Firefox 33
Iteration:
33.3

People

(Reporter: jaws, Assigned: Gijs)

References

Details

Attachments

(2 files, 2 obsolete files)

No description provided.
Attached patch WIP Patch (obsolete) — Splinter Review
Attached patch Patch (rebased)Splinter Review
Rebased this patch on top of the patch in bug 1003364.
Attachment #8418583 - Attachment is obsolete: true
Attachment #8419196 - Flags: review?(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.
Attachment #8419196 - Flags: review?(mconley) → review?(mak77)
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.
Attachment #8419196 - Flags: feedback?(mano)
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 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.
Attachment #8419196 - Flags: feedback?(mano)
(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?
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.
Flags: needinfo?(mmucci)
Flags: firefox-backlog+
Added to Iteration 32.3.  Please provide a point estimate.
Flags: needinfo?(mmucci)
Whiteboard: p=0 s=it-32c-31a-30b.3 [qa?]
Whiteboard: p=0 s=it-32c-31a-30b.3 [qa?] → p=3 s=it-32c-31a-30b.3 [qa?]
(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 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.
Attachment #8419196 - Flags: review?(mak77)
Hi Jared, can you recommend if this bug should be [qa+] or [qa-] for QA verification.
Flags: needinfo?(jaws)
Flags: needinfo?(jaws)
Whiteboard: p=3 s=it-32c-31a-30b.3 [qa?] → p=3 s=it-32c-31a-30b.3 [qa+]
Removed from Iteration 32.3
Assignee: jaws → nobody
Status: ASSIGNED → NEW
Whiteboard: p=3 s=it-32c-31a-30b.3 [qa+] → p=3 [qa+]
Added to Iteration 32.3
Assignee: nobody → florian
Status: NEW → ASSIGNED
Whiteboard: p=3 [qa+] → p=3 s=it-32c-31a-30b.3 [qa+]
Apologies, I modified the wrong bug.
Assignee: florian → nobody
Status: ASSIGNED → NEW
Whiteboard: p=3 s=it-32c-31a-30b.3 [qa+] → p=3 [qa+]
Marco, can you add this back in my name? :-)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Flags: needinfo?(mmucci)
Added to Iteration 32.3
Flags: needinfo?(mmucci)
Whiteboard: p=3 [qa+] → p=3 s=it-32c-31a-30b.3 [qa+]
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.
Attachment #8434978 - Flags: review?(mak77)
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
Attachment #8434978 - Flags: review?(mak77) → review?(mano)
Whiteboard: p=3 s=it-32c-31a-30b.3 [qa+] → p=3 s=33.1 [qa+]
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).
Attachment #8434978 - Flags: review?(mano) → feedback+
(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.
Needinfo for comment 22...
Flags: needinfo?(mano)
(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.
Flags: needinfo?(mano)
Blocks: 1003364
(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.)
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.
Attachment #8441312 - Flags: review?(mano)
Attachment #8434978 - Attachment is obsolete: true
(the removal of rootIsSelected is also cleanup - it was unused already, in fact, but now it can never even be true, so...)
Iteration: --- → 33.2
Points: --- → 3
QA Whiteboard: [qa+]
Whiteboard: p=3 s=33.1 [qa+]
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
Attachment #8441312 - Flags: review?(mano) → review+
w/ nits:

remote:   https://hg.mozilla.org/integration/fx-team/rev/5d467596b176
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/5d467596b176
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 33
Hi Florin, can a QA contact be assigned for verification of this bug.
Flags: needinfo?(florin.mezei)
Iteration: 33.2 → 33.3
Flags: needinfo?(florin.mezei)
QA Contact: cornel.ionce
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.
Status: RESOLVED → VERIFIED
QA Whiteboard: [qa+] → [qa!]
Depends on: 1090609
Depends on: 1177639
You need to log in before you can comment on or make changes to this bug.