Closed Bug 1419426 Opened 5 years ago Closed 5 years ago

Expose ui.context_menus.after_mouseup to webextension preferences so mouse gesture add-ons can modify it

Categories

(WebExtensions :: Untriaged, defect)

Product:
WebExtensions
Component:
Untriaged
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox58 fixed, firefox59 fixed)

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

People

(Reporter: Gijs, Assigned: bsilverberg)

References

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 2 obsolete files)

Bug 1419426 - Implement browserSettings.contextMenuShowEvent,
59 bytes, text/x-review-board-request
kmag
: review+
Details
Patch for uplift to beta.
10.18 KB, patch
gchang
: approval-mozilla-beta+
Details | Diff | Splinter Review 
Apparently mouse gesture add-ons that use the right mouse button need to delay the context menu on macOS/Linux so it doesn't interfere with the gesture. We added a hidden about:config pref for this. It would potentially be nice if add-ons could change this pref -- at least it would provide a more seamless UX for people who used such add-ons. From the bug that added the pref:

(In reply to Andrew Swan [:aswan] from bug 1360278 comment #36)
> :smaug was asking about exposing
> preferences to webextensions.  We do this for things like the
> browserSettings [1] and privacy [2] apis.  For a simple preference, the code
> to implement one of these for webextensions is pretty simple, see something
> like [3] for an example.
> 
> [1]
> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/browserSettings
> [2] https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/privacy
> [3]
> http://searchfox.org/mozilla-central/rev/
> 7ba03cebf1dcf3b4c40c85a4efad42a5022cf1ec/toolkit/components/extensions/ext-
> privacy.js#63-71

I don't have a strong opinion either way, because I don't use this type of add-on. I assume the webextensions team can make an appropriate decision here.

Note that the preference does nothing on Windows right now, and I don't know of any plans to change this.
Bob, since bug 1360278 got uplifted, if this is simple, it be nice to land and uplift too.
Assignee: nobody → bob.silverberg
(In reply to Andy McKay [:andym] from comment #1)
> Bob, since bug 1360278 got uplifted, if this is simple, it be nice to land
> and uplift too.

Yes, this is very simple to do. I'll get right on it. The only question is what we should name the setting. I'm thinking browser.browserSettings.showContextMenuAfterMouseUp, as that's a name being used in our code base already [1] and it seems decent to me. What do you think, Mike?

[1] https://hg.mozilla.org/mozilla-central/file/f540f9e801cb/widget/nsBaseWidget.cpp#l1223
Flags: needinfo?(mconca)
The ui.context_menus.after_mouseup pref doesn't really provide absolute control over whether the menu pops up on mouseup or mousedown. I.e., you cannot force it to mousedown, only request that it be mouseup, on platforms where mousedown is the default. This is why I felt our setting should simply be about forcing it to mouseup - either true or false. 

Kris, how do you think we could make this better?
Flags: needinfo?(kmaglione+bmo)
Based on Kris' feedback in IRC, I'm going to change it to browserSettings.contextMenuShowEvent, which accepts an enum of either `mouseup` or `mousedown`. We'll have to take the value passed by the extension and interpret whether it can be set or not. My understanding from bug 1360278 is that the following will be possible:

Platform   Pref value   When context menu is shown
--------   ----------   --------------------------
Linux      false        mousedown
Linux      true         mouseup
OS X       false        mousedown
OS X       true         mouseup
Windows    false        mouseup
Windows    true         mouseup

Gijs, does that look accurate?
Flags: needinfo?(mconca)
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Bob Silverberg [:bsilverberg] from comment #4)
> Based on Kris' feedback in IRC, I'm going to change it to
> browserSettings.contextMenuShowEvent, which accepts an enum of either
> `mouseup` or `mousedown`. We'll have to take the value passed by the
> extension and interpret whether it can be set or not. My understanding from
> bug 1360278 is that the following will be possible:
> 
> Platform   Pref value   When context menu is shown
> --------   ----------   --------------------------
> Linux      false        mousedown
> Linux      true         mouseup
> OS X       false        mousedown
> OS X       true         mouseup
> Windows    false        mouseup
> Windows    true         mouseup
> 
> Gijs, does that look accurate?

Yes. Basically, setting the pref is a no-op on Windows, but it should otherwise work.
Flags: needinfo?(gijskruitbosch+bugs)
Tiny issue with setting description - copied, pasted but not updated.

browser_settings.json:35
  "description": "How images should be animated in the browser."
(In reply to richardaburton from comment #8)
> Tiny issue with setting description - copied, pasted but not updated.
> 
> browser_settings.json:35
>   "description": "How images should be animated in the browser."

Thanks for spotting that!
Comment on attachment 8930670 [details]
Bug 1419426 - Implement browserSettings.contextMenuShowEvent,

https://reviewboard.mozilla.org/r/201802/#review207558

::: toolkit/components/extensions/ext-browserSettings.js:143
(Diff revision 3)
>          newTabPageOverride: getSettingsAPI(extension,
>            NEW_TAB_OVERRIDE_SETTING,
>            () => {
>              return aboutNewTabService.newTabURL;
>            }, URL_STORE_TYPE, true),
> +        contextMenuShowEvent: Object.assign(getSettingsAPI(extension,

Nit: `getSettingsAPI(...` should start on a new line. And `extension` should either go on its own line, or subsequent arguments should be aligned with it. We really need to fix this ESLint rule...

::: toolkit/components/extensions/ext-browserSettings.js:158
(Diff revision 3)
> +              if (AppConstants.platform === "android"
> +                  || (AppConstants.platform === "win"
> +                      && details.value === "mousedown")) {

Nit: Binary operators go at the end of the previous line.
Attachment #8930670 - Flags: review?(kmaglione+bmo) → review+
Pushed by bsilverberg@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e8d475fed206
Implement browserSettings.contextMenuShowEvent, r=kmag
Attached patch Patch for uplift to beta. (obsolete) — — Splinter Review 
Approval Request Comment
[Feature/Bug causing the regression]: 1360278
[User impact if declined]: WebExtensions will not be able to set the pref introduced in bug 1360278, which was one of the reasons for introducing the pref in the first place.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: No
[Needs manual test from QE? If yes, steps to reproduce]: No 
[List of other uplifts needed for the feature/fix]: Bug 1360278, which has already been uplifted
[Is the change risky?]: No
[Why is the change risky/not risky?]: This introduces a single API method, browserSettings.contextMenuShowEvent, which involves changes to a single file, and it is covered by automated tests. A try run has been submitted at https://treeherder.mozilla.org/#/jobs?repo=try&revision=96fa78856ebd1305530e648de0befe921b822870
[String changes made/needed]: None
Attachment #8931348 - Flags: approval-mozilla-beta?
Comment on attachment 8931348 [details] [diff] [review]
Patch for uplift to beta.

Oops, there seems to be a problem with the patch.
Attachment #8931348 - Flags: approval-mozilla-beta?
Pushed by bsilverberg@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d4779e0fdbbb
Implement browserSettings.contextMenuShowEvent, r=kmag
https://hg.mozilla.org/mozilla-central/rev/d4779e0fdbbb
Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox59: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Attached patch Patch for uplift to beta. (obsolete) — — Splinter Review 
Approval Request Comment
[Feature/Bug causing the regression]: 1360278
[User impact if declined]: WebExtensions will not be able to set the pref introduced in bug 1360278, which was one of the reasons for introducing the pref in the first place.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: No
[Needs manual test from QE? If yes, steps to reproduce]: No 
[List of other uplifts needed for the feature/fix]: Bug 1360278, which has already been uplifted
[Is the change risky?]: No
[Why is the change risky/not risky?]: This introduces a single API method, browserSettings.contextMenuShowEvent, which involves changes to a single file, and it is covered by automated tests. A try run has been submitted at https://treeherder.mozilla.org/#/jobs?repo=try&revision=8532753db360a1b1b1a90ab7c8ded7e2b1762e37
[String changes made/needed]: None
Attachment #8931348 - Attachment is obsolete: true
Flags: needinfo?(bob.silverberg)
Attachment #8931661 - Flags: approval-mozilla-beta?
Tested and working on v59. Uplift for v58 would be appreciated.

Updated https://addons.mozilla.org/addon/tabscroller/ with browserSettings permission and the following trivial code (which first checks for the existence of the new setting so it still runs fine on older versions of Firefox):

if (browser.browserSettings.contextMenuShowEvent) {
	browser.browserSettings.contextMenuShowEvent.set({value: 'mouseup'});
}
Comment on attachment 8931661 [details] [diff] [review]
Patch for uplift to beta.

Since bug 1360278 is in 58, we need to provide webextension a way to set the pref. Beta58+.
Attachment #8931661 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Sorry if I'm being thick, but attachment #8931661 [details] [diff] [review] doesn't look like it has anything to do with this bug/feature.
Richarda is correct, attachment 8931661 [details] [diff] [review] in this bug is a duplicate of attachment 8931616 [details] [diff] [review] in bug 1417869. Typo when the attachment number got entered manually? Bob, please upload the correct patch.
Flags: needinfo?(bob.silverberg)
This has already been approved for uplift. See comment #24. Somehow the wrong patch got attached to the bug, so this is the correct patch.
Attachment #8931661 - Attachment is obsolete: true
Flags: needinfo?(bob.silverberg)
Attachment #8932077 - Flags: approval-mozilla-beta?
Attachment #8932077 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Verified working in v58.b7
Is manual testing required on this bug? If yes, please provide some STR and the proper extension(if required) or set the “qe-verify -“ flag.

Thanks!
Flags: needinfo?(bob.silverberg)
Flags: needinfo?(bob.silverberg) → qe-verify-
Flags: needinfo?(wbamberg)
Keywords: dev-doc-needed
Flags: needinfo?(wbamberg)
-> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/browserSettings/contextMenuShowEvent

Please let me know if this covers it (note that the compat data is not deployed to MDN yet, but will be soon).
Flags: needinfo?(bob.silverberg)
Looks good, thanks Will.
Flags: needinfo?(bob.silverberg)
(In reply to Will Bamberg [:wbamberg] from comment #31)
> ->
> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/
> browserSettings/contextMenuShowEvent
> 
> Please let me know if this covers it (note that the compat data is not
> deployed to MDN yet, but will be soon).

Just one more thing that may be useful to mention is, that calling this preference with “mousedown” is a no-op on Windows.
> Just one more thing that may be useful to mention is, that calling this preference with “mousedown” is a no-op on Windows.

I did not know this. Thanks! Then I guess this:

> The default value is "mousedown".

...is also not true on Windows?
(In reply to Will Bamberg [:wbamberg] from comment #34)
> > Just one more thing that may be useful to mention is, that calling this preference with “mousedown” is a no-op on Windows.
> 
> I did not know this. Thanks! Then I guess this:
> 
> > The default value is "mousedown".
> 
> ...is also not true on Windows?

Correct. I've updated the doc ( https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/browserSettings/contextMenuShowEvent$compare?locale=en-US&to=1358194&from=1357521 )
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.