Closed Bug 943901 Opened 8 years ago Closed 3 years ago

(Add-On) - Long-Tap-Search conflicts with text-selection action-bar capability

Categories

(Firefox for Android Graveyard :: General, defect)

28 Branch
ARM
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: aaronmt, Assigned: wesj)

References

Details

(Keywords: addon-compat)

Attachments

(2 files)

See bug 943876. Currently when one installs Long Tap Search (https://addons.mozilla.org/en-US/android/addon/long-tap-search/?src=ss), a long-tap on any text in content will yield nothing at all.

E/GeckoConsole( 9024): [JavaScript Error: "TypeError: aWindow.SelectionHandler.shouldShowContextMenu is not a function" {file: "resource://gre/modules/XPIProvider.jsm -> jar:file:///data/data/org.mozilla.fennec/files/mozilla/zzjfpwt7.default/extensions/%7Ba7cbc53c-aeec-4474-92a1-6d0921e499d6%7D.xpi!/bootstrap.js" line: 82}]
We could stub this method for them if you want. Do you want us to do hold on to legacy code for add-on compatibility?
Flags: needinfo?(mark.finkle)
First, we should see if stubbing that function just leads to more issues. Second, we should see if the add-on could be reimplemented using the new UI approach. If it can, we should contact the author (and search for other affected add-ons) and suggest a way to support both modes.

If the add-on can't be supported using the new UI approach, then we need to find a way to make it work so we give add-on devs a migration path.
Flags: needinfo?(mark.finkle)
Attached patch Patch v1Splinter Review
This stubs and provides a little bit of backwards compatibility for add-ons that previously matched on text selection. For this one, it will cause all of the "Search with..." menu items to be added to the overflow menu. I had to use a bit of a hack. What used to happen was:

1.) First long tap, add-on doesn't match selector. Text is selected instead.
2.) Second long tap, add-on DOES match. Context menu is shown.

Without the __calledFromSelectionHandler__ stuff that's in this patch, that turns into

1.) First long tap, add-on doesn't match selector. Text is selected instead. 1a.) Action mode is shown and DOES match selector. Context menu items are added.
2.) Second long tap, add-on DOES match. Context menu is shown.

The hack lets us have:

1.) First long tap, add-on doesn't match selector. Text is selected instead. 1a.) Action mode is shown and DOES match selector. Context menu items are added
2.) Second long tap, add-on doesn't match selector. Selection is updated.
2a.) Action mode is updated and DOES match selector. Context menu items are added

Not sure how much work we want to do for backward compat addons. I'll also try to advertise the better way to do this for new addons/updated ones.
Attachment #8347413 - Flags: review?(mark.finkle)
Blocks: 950593
Since we've seen addon authors start to use this, we should add an api. This just adds some methods in SelectionHandler, addAction and removeAction. addAction has some logic to automagically generate an id for you if you don't pass one in.
Attachment #8348990 - Flags: review?(mark.finkle)
Comment on attachment 8348990 [details] [diff] [review]
Patch 2/2 - Add add-on api

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>-    SelectionHandler.actions.SEARCH_ADD = {
>+    SelectionHandler.addAction({
>       id: "add_search_action",

I like to reorder based on "base-verb" style, so how about "search-add"
Attachment #8348990 - Flags: review?(mark.finkle) → review+
Comment on attachment 8347413 [details] [diff] [review]
Patch v1

>+  // Not used by us. Provides backward compatibility for addons.
>+  shouldShowContextMenu: function shouldShowContextMenu() {
>+    return (this._activeType === this.TYPE_SELECTION) && this.__calledFromSelectionHandler__;
>+  },

How is this even called? I don't see any code that uses it.
(In reply to Mark Finkle (:mfinkle) from comment #6)
> Comment on attachment 8347413 [details] [diff] [review]
> Patch v1
> 
> >+  // Not used by us. Provides backward compatibility for addons.
> >+  shouldShowContextMenu: function shouldShowContextMenu() {
> >+    return (this._activeType === this.TYPE_SELECTION) && this.__calledFromSelectionHandler__;
> >+  },
> 
> How is this even called? I don't see any code that uses it.

Two addons call it: https://mxr.mozilla.org/addons/search?string=SelectionHandler.shouldShowContextMenu&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=addons
https://hg.mozilla.org/integration/fx-team/rev/704e9626f012

Part 2 goes in. This won't fix the addon, so leave-opening the bug. We can close if we don't want that fix though. I'll try to blog/email them soon.
Whiteboard: [leave-open]
Comment on attachment 8347413 [details] [diff] [review]
Patch v1

Let's try to get add-ons to convert to the new API instead of adding too much compatibility code to the app.
Attachment #8347413 - Flags: review?(mark.finkle)
Component: General → Text Selection
Bah, didn't mean to change that on second consideration.
Component: Text Selection → General
I don't think it makes sense to keep this bug as it has been 5 years without activities.
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Whiteboard: [leave-open]
Assignee: nobody → wjohnston2000
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.