1244462 - "Save Page to Pocket" menu should not appear at the top of selected text menu

Status: RESOLVED FIXED

(Firefox :: Pocket, defect)

Description

[Jack M]

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
Build ID: 20160128004012

Steps to reproduce:

Right click after selecting some text on any webpage. The context menu opens, with the 'Save Page to Pocket' button at the top, which breaks my workflow.


Actual results:

The 'Save Page to Pocket' option appeared at the top of the menu.


Expected results:

The 'Copy' button should be on top, and the 'Save Page To Pocket' button should either be lower down, or not appear at all (I'm copying text, I doubt I'll want to save a webpage at that point).

Comment 1

[Jack M]

Attachment: e4579799557137a949cb004a6ad7f0b6.gif

GIF Demonstration of issue

Comment 2

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking since this is a regression likely in 46.

Comment 3

[Shane Caraveo (:mixedpuppy)]

This is a result of using the "on-build-contextmenu" observer to add/update menu items for the addon.  It relies on this.isContentSelected, which is used before it is set.  Patch coming.

Comment 4

[Shane Caraveo (:mixedpuppy)]

Attachment: MozReview Request: Bug 1244462 set isContentSelected before using it for the on-build-contextmenu notification, r=gijs

Review commit: https://reviewboard.mozilla.org/r/34237/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/34237/

Comment 5

[Shane Caraveo (:mixedpuppy)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7f85be635f15

Comment 6

[:Gijs (he/him)]

This should probably not remove the current location of the setter because the observers might change the selection.

Comment 7

[Shane Caraveo (:mixedpuppy)]

good point.  though it would be PITA to have your selection change in the midst of a context click.

Comment 8

[:Gijs (he/him)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #7)
> good point.  though it would be PITA to have your selection change in the
> midst of a context click.

I agree that doing it intentionally would be broken. I would just not be surprised if add-ons did it unintentionally and also if code did not like it if that flag was set but there was no selection.

Could even just define it as a getter, tbh... so it always accurately reflects what's going on with the document.

Comment 9

[:Gijs (he/him)]

... though the document itself may in some cases also go away. For more such fun see bug 1025568.

Comment 10

[:Gijs (he/him)]

Comment on attachment 8717598 [details]
MozReview Request: Bug 1244462 set isContentSelected before using it for the on-build-contextmenu notification, r=gijs

https://reviewboard.mozilla.org/r/34237/#review31033

::: browser/base/content/nsContextMenu.js
(Diff revision 1)
> -    this.isContentSelected = !this.selectionInfo.docSelectionIsCollapsed;

r=me with this left in.

Comment 11

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8717598 [details]
MozReview Request: Bug 1244462 set isContentSelected before using it for the on-build-contextmenu notification, r=gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34237/diff/1-2/

Comment 12

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8717598 [details]
MozReview Request: Bug 1244462 set isContentSelected before using it for the on-build-contextmenu notification, r=gijs

https://reviewboard.mozilla.org/r/34237/#review31093

Comment 13

[Shane Caraveo (:mixedpuppy)]

https://hg.mozilla.org/integration/fx-team/rev/e0409d9febb6cfb7bbaca11563ce96bde6e38c64
Bug 1244462 set isContentSelected before using it for the on-build-contextmenu notification, r=gijs

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e0409d9febb6

Comment 15

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8717598 [details]
MozReview Request: Bug 1244462 set isContentSelected before using it for the on-build-contextmenu notification, r=gijs

Approval Request Comment
[Feature/regressing bug #]: 1190667
[User impact if declined]: users could get context menus that may not work
[Describe test coverage new/current, TreeHerder]: manual
[Risks and why]: low
[String/UUID change made/needed]: none

Comment 16

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8717598 [details]
MozReview Request: Bug 1244462 set isContentSelected before using it for the on-build-contextmenu notification, r=gijs

Recent regression, fix for menus. Please uplift to aurora.

Comment 17

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/de3216301e20

Comment 18

[Mayur Patil]

[bugday-20160323]

Status: RESOLVED,FIXED -> VERIFIED

Comments:
Test Successful

Component: 
Name			Firefox
Version			46.0b9
Build ID		20160322075646
Update Channel          beta
User Agent		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS			Windows 7 SP1 x86_64

Expected Results: 
Yes

Actual Results: 
As expected