1264118 - [PageAction] Add support for chrome.pageAction popups on Android

Status: VERIFIED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Andy McKay]

As a first WebExtension API that impacts Android UX we'd like to add to try implementing the page action API.

https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/pageAction

We can already use the PageAction.jsm (which hopefully does the same thing):

https://dxr.mozilla.org/mozilla-central/source/mobile/android/modules/PageActions.jsm

That will allow us to add in the icon into the menu bar. However the pageAction API includes the ability to show a popup, something that I don't think is currently supported and has implications for the UX.

I've thrown together a quick demo of this might work here:

http://people.mozilla.org/~amckay/page-action-example/
http://people.mozilla.org/~amckay/page-action-example/web-ext-build/

And here's how it looks on Desktop:

https://www.dropbox.com/s/8ax7uhb5hx8ha0v/Screenshot%202016-04-12%2015.22.15.png?dl=0

What should we do here?

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

My first thoughts are about the content allowed in the popup. The API appears to support loading a URL into the popup. You will have a tough time getting that to work in Fennec. Work continues to enable using multiple GeckoViews in the same app (Jim Chen might be close), but until then you'd need to use a WebView.

Also, I notice from the Chrome API docs [1], that the Fennec pageAction is not the same as a Chrome pageAction. In the UI example image in [1], Fennec pageAction would be like the bookmark star in the URL box, not the RSS icon in the toolbar.

[1] https://developer.chrome.com/extensions/pageAction

Comment 2

[Andy McKay]

(In reply to Mark Finkle (:mfinkle) from comment #1)
> Also, I notice from the Chrome API docs [1], that the Fennec pageAction is
> not the same as a Chrome pageAction. In the UI example image in [1], Fennec
> pageAction would be like the bookmark star in the URL box, not the RSS icon
> in the toolbar.

I think its the same and those docs are a bit misleading. In Desktop a pageAction appears as icon in the URL box (see the green jigsaw in the image in comment 1), we'd like it to do the same. 

Chrome are currently making pageActions appear in the toolbar like browserActions in Canary, there's a bit of backlash against that.

Comment 3

[:Margaret Leibovic]

I think we'll need UX help to think about what to do about popup support.

As a first step, we can add support for page actions without popups (I see there is an "onClicked" handler that can fire when the page action is tapped).

As for actually supporting the popups, we could explore using a WebView in a native dialog, as mfinkle mentioned in comment 1. We could also try injecting an anonymous iframe or something into the web page, with a lightbox type of effect applied, but that seems much more fragile and prone to security/correctness problems.

I'd be interested to know what the ideal UX experience is, and then we can work backwards from there to try to figure out the best way to actually implement it.

Comment 4

[Andy McKay]

(In reply to :Margaret Leibovic from comment #3)
> As a first step, we can add support for page actions without popups (I see
> there is an "onClicked" handler that can fire when the page action is
> tapped).

That's my thought too. mattw is filing bugs for the rest of the API around pageActions on the tracker.

> I'd be interested to know what the ideal UX experience is, and then we can
> work backwards from there to try to figure out the best way to actually
> implement it.

Agreed.

Comment 5

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

I heard someone say "UX"! 

So I was looking to try the add-on in comment 0 but it doesn't work. I think this is because we're protecting users from unsigned add-ons.

(In reply to Andy McKay [:andym] from comment #4)
> (In reply to :Margaret Leibovic from comment #3)
> > As a first step, we can add support for page actions without popups (I see
> > there is an "onClicked" handler that can fire when the page action is
> > tapped).
> 
> That's my thought too. mattw is filing bugs for the rest of the API around
> pageActions on the tracker.

I like this idea. This would simplify the UX ask here while allowing early add-on integration.

> > I'd be interested to know what the ideal UX experience is, and then we can
> > work backwards from there to try to figure out the best way to actually
> > implement it.
> 
> Agreed.

Andy, (before I start getting too carried away with the UX here...) What are the most popular add-ons on Desktop that leverage this API and creates a need for pop ups? It'll help me understand the add-on developers desires a bit more.

To me the UI/UX equivalent for us on Mobile here has a couple possibilities:

 1) a dialog, (feels more appropriate (UX-wise) and creates less edge cases down the line), but is usually text/button focused
 2) our Helper UI, but is not "ideal" because we usually use this to present tips and hints
 3) a new tab, (how we handle loading URLs in a normal circumstance) but it fragments the add-on experience.

But none of these seem ideal right now. It sounds like we might have to create something entirely new here if we want to load URLs in a "pop up" like experience.

I have some ideas in that regard too.

Comment 6

[Andy McKay]

(In reply to Anthony Lam (:antlam) from comment #5)
> I heard someone say "UX"! 
> 
> So I was looking to try the add-on in comment 0 but it doesn't work. I think
> this is because we're protecting users from unsigned add-ons.

Sure, you can turn off signing in about:config and turning xpinstall.signatures.required to false.

> Andy, (before I start getting too carried away with the UX here...) What are
> the most popular add-ons on Desktop that leverage this API and creates a
> need for pop ups? It'll help me understand the add-on developers desires a
> bit more.

Erm. Here's a list of some google chrome add-ons that use it:

https://docs.google.com/document/d/1Kdlais78KKx8ParpRPx9rDmB9OIkYYt36uaDcZqnuOM/edit

But that means testing in Chrome, so I'm not sure what that's going to buy you. We have about 200 add-ons on AMO that use WebExtensions so its hard for me to tell you that any of them are popular right now.


> To me the UI/UX equivalent for us on Mobile here has a couple possibilities:
> 
>  1) a dialog, (feels more appropriate (UX-wise) and creates less edge cases
> down the line), but is usually text/button focused
>  2) our Helper UI, but is not "ideal" because we usually use this to present
> tips and hints
>  3) a new tab, (how we handle loading URLs in a normal circumstance) but it
> fragments the add-on experience.

We were hoping that anything we do here is similarish to browserAction. That can similarly define a popup when you click a button in the menu bar. Of course adding a button to menu bar is harder in Android, we thought we'd do this one as the easier route first.

Comment 7

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

All things considered, let's go with Margaret's suggestion in comment 3 for the time being. That is, having just an icon in the toolbar for now.

Scope:
 - basic support for developers to add an icon in our URL bar (like the Reader View and Open in App icons)
 - Opens a link in a new tab

hope that helps, feel free to NI me with any questions!

Comment 8

[Matthew Wein [:mattw]]

Attachment: Part 1: Implement default_popup for chrome.pageAction (Bug 1264118)

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

Comment 9

[Matthew Wein [:mattw]]

Comment on attachment 8755086 [details]
Part 1: Implement default_popup for chrome.pageAction (Bug 1264118)

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

Comment 10

[Kris Maglione [:kmag]]

Comment on attachment 8755086 [details]
Part 1: Implement default_popup for chrome.pageAction (Bug 1264118)

https://reviewboard.mozilla.org/r/54394/#review51078

::: mobile/android/components/extensions/ext-pageAction.js:13
(Diff revision 2)
>  // Import the android PageActions module.
>  XPCOMUtils.defineLazyModuleGetter(this, "PageActions",
>                                    "resource://gre/modules/PageActions.jsm");
>  
>  Cu.import("resource://gre/modules/ExtensionUtils.jsm");
> +Cu.import("resource://gre/modules/Services.jsm");

Please use `defineLazyModuleGetter`

::: mobile/android/components/extensions/ext-pageAction.js:33
(Diff revision 2)
>      title: options.default_title || extension.name,
>      icon: DEFAULT_ICON,
>      id: extension.id,
>      clickCallback: () => {
> +      if (options.default_popup) {
> +        let win = Services.wm.getMostRecentWindow('navigator:browser');

Double quotes.

::: mobile/android/components/extensions/test/mochitest/test_ext_pageAction_defaultPopup.html:21
(Diff revision 2)
> +function backgroundScript() {
> +  // TODO: Use the Tabs API to obtain the tab ids for showing pageActions.
> +  let tabId = 1;
> +  browser.test.onMessage.addListener(msg => {
> +    if (msg === "page-action-show") {
> +      browser.pageAction.show(tabId);

I think you need a `.then(() => { ...` here. After bug 1270742 lands, anyway.

::: mobile/android/components/extensions/test/mochitest/test_ext_pageAction_defaultPopup.html:41
(Diff revision 2)
> +  browser.test.sendMessage("ready");
> +}
> +
> +function popupScript() {
> +  window.onload = () => {
> +    let text = document.getElementById("test").innerText;

`innerText` is an IE-ism. Please use `textContent` instead.

::: mobile/android/components/extensions/test/mochitest/test_ext_pageAction_defaultPopup.html:42
(Diff revision 2)
> +}
> +
> +function popupScript() {
> +  window.onload = () => {
> +    let text = document.getElementById("test").innerText;
> +    browser.test.assertEq("Hello, World!", text);

I'm not sure this is really necessary.

::: mobile/android/components/extensions/test/mochitest/test_ext_pageAction_defaultPopup.html:43
(Diff revision 2)
> +
> +function popupScript() {
> +  window.onload = () => {
> +    let text = document.getElementById("test").innerText;
> +    browser.test.assertEq("Hello, World!", text);
> +    browser.runtime.sendMessage("from-page-action-popup");

We should test that `window.close()` successfully closes the tab here. Especially since I'm fairly sure we don't automatically close extension page tabs on Android at extension shutdown yet.

::: mobile/android/components/extensions/test/mochitest/test_ext_pageAction_defaultPopup.html:45
(Diff revision 2)
> +  window.onload = () => {
> +    let text = document.getElementById("test").innerText;
> +    browser.test.assertEq("Hello, World!", text);
> +    browser.runtime.sendMessage("from-page-action-popup");
> +  };
> +}

In general, the background script and popup functions should be inside the task function that they belong to.

::: mobile/android/components/extensions/test/mochitest/test_ext_pageAction_defaultPopup.html:59
(Diff revision 2)
> +        "default_popup": "popup.html"
> +      },
> +    },
> +    files: {
> +      "popup.html": `<html><head><meta charset="utf-8"><script src="popup.js"></${"script"}></head><body><div id="test">Hello, World!</div></body></html>`,
> +      "popup.js": "(" + popupScript.toString() + ")()"

No need for `.toString()`. And a template string would be better than string concatenation here.

Comment 11

[Matthew Wein [:mattw]]

Comment on attachment 8755086 [details]
Part 1: Implement default_popup for chrome.pageAction (Bug 1264118)

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

Comment 12

[Kris Maglione [:kmag]]

Comment on attachment 8755086 [details]
Part 1: Implement default_popup for chrome.pageAction (Bug 1264118)

https://reviewboard.mozilla.org/r/54394/#review51348

::: mobile/android/components/extensions/test/mochitest/test_ext_pageAction_defaultPopup.html:51
(Diff revision 3)
> +    window.onload = () => {
> +      browser.runtime.sendMessage("from-page-action-popup-shown");
> +    };
> +    browser.runtime.onMessage.addListener(msg => {
> +      if (msg == "close-popup") {
> +        browser.runtime.sendMessage("from-page-action-popup-closed");

You can use `browser.test.sendMessage` from the popup script.

::: mobile/android/components/extensions/test/mochitest/test_ext_pageAction_defaultPopup.html:83
(Diff revision 3)
> +
> +  clickPageAction(extension.id);
> +  yield extension.awaitMessage("page-action-popup-shown");
> +
> +  extension.sendMessage("page-action-close-popup");
> +  yield extension.awaitMessage("page-action-popup-closed");

Please check that the tab is actually closed.

Comment 13

[Matthew Wein [:mattw]]

https://reviewboard.mozilla.org/r/54394/#review51348

> You can use `browser.test.sendMessage` from the popup script.

Oh cool, thanks, I didn't think I had access to that.

> Please check that the tab is actually closed.

Do you know if there's a simple way to do this without access to the tabs api?  If not I'll ask #mobile to see if I can get some tips on how to do this.  I'm thinking maybe I can listen for the TabClose event that's dispatched here: https://dxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#1231.

Comment 14

[Matthew Wein [:mattw]]

Comment on attachment 8755086 [details]
Part 1: Implement default_popup for chrome.pageAction (Bug 1264118)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54394/diff/3-4/

Comment 15

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/54394/#review51348

> Do you know if there's a simple way to do this without access to the tabs api?  If not I'll ask #mobile to see if I can get some tips on how to do this.  I'm thinking maybe I can listen for the TabClose event that's dispatched here: https://dxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#1231.

You can just check `BrowserApp.tabs` after you get the message from the popup script.

Comment 16

[Matthew Wein [:mattw]]

https://reviewboard.mozilla.org/r/54394/#review51348

> You can just check `BrowserApp.tabs` after you get the message from the popup script.

Hm, I don't think BrowserApp was defined in the mochitest scope.

Comment 17

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/54394/#review51348

> Hm, I don't think BrowserApp was defined in the mochitest scope.

It's not. You need to get the browser window and use its BrowserApp object.

Comment 18

[Matthew Wein [:mattw]]

Attachment: MozReview Request: Implement default_popup for chrome.pageAction on Android r?kmag

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

Comment 19

[Matthew Wein [:mattw]]

Comment on attachment 8755086 [details]
Part 1: Implement default_popup for chrome.pageAction (Bug 1264118)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54394/diff/4-5/

Comment 20

[Matthew Wein [:mattw]]

Comment on attachment 8755086 [details]
Part 1: Implement default_popup for chrome.pageAction (Bug 1264118)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54394/diff/5-6/

Comment 21

[Matthew Wein [:mattw]]

Attachment: Part 2: Implement {set|get}Popup for chrome.pageAction (Bug 1264118)

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

Comment 22

[Matthew Wein [:mattw]]

Comment on attachment 8755086 [details]
Part 1: Implement default_popup for chrome.pageAction (Bug 1264118)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54394/diff/6-7/

Comment 23

[Matthew Wein [:mattw]]

Comment on attachment 8755086 [details]
Part 1: Implement default_popup for chrome.pageAction (Bug 1264118)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54394/diff/7-8/

Comment 24

[Matthew Wein [:mattw]]

Comment on attachment 8758431 [details]
Part 2: Implement {set|get}Popup for chrome.pageAction (Bug 1264118)

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

Comment 25

[Matthew Wein [:mattw]]

Comment on attachment 8755086 [details]
Part 1: Implement default_popup for chrome.pageAction (Bug 1264118)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54394/diff/8-9/

Comment 26

[Matthew Wein [:mattw]]

Comment on attachment 8758431 [details]
Part 2: Implement {set|get}Popup for chrome.pageAction (Bug 1264118)

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

Comment 27

[Matthew Wein [:mattw]]

Comment on attachment 8758431 [details]
Part 2: Implement {set|get}Popup for chrome.pageAction (Bug 1264118)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56652/diff/3-4/

Comment 28

[Kris Maglione [:kmag]]

Comment on attachment 8755086 [details]
Part 1: Implement default_popup for chrome.pageAction (Bug 1264118)

https://reviewboard.mozilla.org/r/54394/#review53840

::: mobile/android/components/extensions/test/mochitest/test_ext_pageAction_popup.html:92
(Diff revision 9)
> +  yield extension.awaitMessage("from-page-action-popup-shown");
> +
> +  extension.sendMessage("page-action-close-popup");
> +
> +  let url = yield tabClosedPromise;
> +  ok(url.indexOf("popup.html") > -1, "The tab for the popup should be closed");

`url.includes("popup.html")`

Comment 29

[Kris Maglione [:kmag]]

Comment on attachment 8758431 [details]
Part 2: Implement {set|get}Popup for chrome.pageAction (Bug 1264118)

https://reviewboard.mozilla.org/r/56652/#review53842

::: mobile/android/components/extensions/ext-pageAction.js:70
(Diff revision 4)
>  
> +  setPopup(tab, url) {
> +    // TODO: Only set the popup for the specified tab once we have Tabs API support.
> +    if (url) {
> +      this.popupUrl = url;
> +    }

We should set it to null if `url` is an empty string, and to `options.default_popup` if it's `null`.

We should also test both of those cases.

::: mobile/android/components/extensions/test/mochitest/test_ext_pageAction_popup.html:46
(Diff revision 4)
>      });
>  
>      browser.test.sendMessage("ready");
>    }
>  
> -  function popupScript() {
> +  function generatePopupScript(name) {

No need for this. Just use the same script for both HTML files, and send the URL of the page (`location.href`) with the message.

::: mobile/android/components/extensions/test/mochitest/test_ext_pageAction_popup.html:97
(Diff revision 4)
>    });
>  
> +  function* testPopup(name) {
> +    extension.sendMessage("page-action-get-popup");
> +    let url = yield extension.awaitMessage("page-action-got-popup");
> +    ok(url.indexOf(`${name}`) > -1, "Calling pageAction.getPopup should return the correct popup URL.");

`url.includes(name)`

::: mobile/android/components/extensions/test/mochitest/test_ext_pageAction_popup.html:104
(Diff revision 4)
> +    clickPageAction(extension.id);
> +    yield extension.awaitMessage(`from-popup-${name}`);
> +
> +    extension.sendMessage("page-action-close-popup", {name: `${name}`});
> +    url = yield tabClosedPromise;
> +    ok(url.indexOf(`${name}`) > -1, "The tab for the popup should be closed");

`url.includes(name)`

Comment 30

[Matthew Wein [:mattw]]

https://reviewboard.mozilla.org/r/56652/#review53842

> We should set it to null if `url` is an empty string, and to `options.default_popup` if it's `null`.
> 
> We should also test both of those cases.

Since `this.popupUrl` defaults to `options.default_popup`, it works just to set `this.popupUrl` to `url`, and then only emit a `click` event when `this.popupUrl` is falsy.  Does this sounds good to you, or should I still null it out when the `url` is an empty string?

Comment 31

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/56652/#review53842

> Since `this.popupUrl` defaults to `options.default_popup`, it works just to set `this.popupUrl` to `url`, and then only emit a `click` event when `this.popupUrl` is falsy.  Does this sounds good to you, or should I still null it out when the `url` is an empty string?

Setting it to an empty string has different behavior than setting it to null. Setting it to an explicit empty string means that there's no popup. Setting it to null means that the tab uses the default popup (which may itself be an empty string, but may not).

Comment 32

[Matthew Wein [:mattw]]

https://reviewboard.mozilla.org/r/56652/#review53842

> Setting it to an empty string has different behavior than setting it to null. Setting it to an explicit empty string means that there's no popup. Setting it to null means that the tab uses the default popup (which may itself be an empty string, but may not).

Are you sure that's what we want to do? We don't accept `null` on desktop nor does Chrome (it throws an error).

Right now, if I try to accept null, I get the following error: "Type error for parameter details (Error processing popup: Expected string instead of null) for pageAction.setPopup" so I think we'd need to modify the schema to make this work.  What do you think we should do?

Comment 33

[Matthew Wein [:mattw]]

Comment on attachment 8755086 [details]
Part 1: Implement default_popup for chrome.pageAction (Bug 1264118)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54394/diff/9-10/

Comment 34

[Matthew Wein [:mattw]]

Comment on attachment 8758431 [details]
Part 2: Implement {set|get}Popup for chrome.pageAction (Bug 1264118)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56652/diff/4-5/

Comment 35

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/56652/#review53842

> Are you sure that's what we want to do? We don't accept `null` on desktop nor does Chrome (it throws an error).
> 
> Right now, if I try to accept null, I get the following error: "Type error for parameter details (Error processing popup: Expected string instead of null) for pageAction.setPopup" so I think we'd need to modify the schema to make this work.  What do you think we should do?

Sorry, you're right. Title was the special case property where setting it to an empty string reverts it to the default value. Setting popup to an empty string always disables it.

I really hate this inconsistency...

Comment 36

[Kris Maglione [:kmag]]

Comment on attachment 8758431 [details]
Part 2: Implement {set|get}Popup for chrome.pageAction (Bug 1264118)

https://reviewboard.mozilla.org/r/56652/#review54600

::: mobile/android/components/extensions/test/mochitest/test_ext_pageAction_popup.html:51
(Diff revisions 4 - 5)
>  
>      browser.pageAction.onClicked.addListener(tab => {
> +      if (onClickedListenerEnabled) {
> +        browser.test.sendMessage("page-action-onClicked-fired");
> +      } else {
> -      browser.test.fail(`The onClicked listener should never fire when a popup is shown.`);
> +        browser.test.fail(`The onClicked listener should never fire when a popup is shown.`);

I'd go with `browser.test.assertTrue(onClickedListenerEnabled, ...)` instead, and then just unconditionally send the message.

::: mobile/android/components/extensions/test/mochitest/test_ext_pageAction_popup.html:62
(Diff revisions 4 - 5)
> -  function generatePopupScript(name) {
> +  function popupScript() {
> -    let popupScript = () => {
> -      window.onload = () => {
> +    window.onload = () => {
> -        browser.test.sendMessage(`from-popup-${name}`);
> +      browser.test.sendMessage("from-popup", location.href);
> -      };
> +    };
> -      browser.runtime.onMessage.addListener(msg => {
> +    browser.runtime.onMessage.addListener(msg => {

You can use `browser.test.onMessage.addListener` here rather than proxying the message in the background script.

Also, it would be nice if you sent a "close-popup" message along with a URL rather than just sending the URL.

::: mobile/android/components/extensions/test/mochitest/test_ext_pageAction_popup.html:80
(Diff revisions 4 - 5)
>          "default_popup": "default.html",
>        },
>      },
>      files: {
>        "default.html": `<html><head><meta charset="utf-8"><script src="default.js"></${"script"}></head></html>`,
> -      "default.js": generatePopupScript("default"),
> +      "default.js": `(${popupScript})()`,

No need for three separate scripts. Just create a single "popup.js" script and include it from all three popup pages.

::: mobile/android/components/extensions/test/mochitest/test_ext_pageAction_popup.html:129
(Diff revisions 4 - 5)
> -    url = yield tabClosedPromise;
> -    ok(url.indexOf(`${name}`) > -1, "The tab for the popup should be closed");
> +      yield extension.awaitMessage("page-action-onClicked-listener-disabled");
> +    } else {
> +      ok(url.includes(name), "Calling pageAction.getPopup should return the correct popup URL when the popup is set.");
> +
> +      clickPageAction(extension.id);
> +      let location = yield extension.awaitMessage("from-popup");

You should check `location.includes(name)` here.

Comment 37

[Matthew Wein [:mattw]]

https://reviewboard.mozilla.org/r/56652/#review53842

> Sorry, you're right. Title was the special case property where setting it to an empty string reverts it to the default value. Setting popup to an empty string always disables it.
> 
> I really hate this inconsistency...

Oh interesting. Yeah, they really should do the same thing.

Comment 38

[Matthew Wein [:mattw]]

Comment on attachment 8755086 [details]
Part 1: Implement default_popup for chrome.pageAction (Bug 1264118)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54394/diff/10-11/

Comment 39

[Matthew Wein [:mattw]]

Comment on attachment 8758431 [details]
Part 2: Implement {set|get}Popup for chrome.pageAction (Bug 1264118)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56652/diff/5-6/

Comment 40

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/df585c71e322
Part 1: Implement default_popup for chrome.pageAction (Bug 1264118). r=kmag
https://hg.mozilla.org/integration/fx-team/rev/b06ec8711d04
Part 2: Implement {set|get}Popup for chrome.pageAction (Bug 1264118). r=kmag

Comment 41

[Carsten Book [:Tomcat]]

backed out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=9821889&repo=fx-team

Comment 42

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/f742d75804d3
Backed out changeset b06ec8711d04 
https://hg.mozilla.org/integration/fx-team/rev/e8e5464c0f16
Backed out changeset df585c71e322 for ES test failures

Comment 43

[Matthew Wein [:mattw]]

Comment on attachment 8758431 [details]
Part 2: Implement {set|get}Popup for chrome.pageAction (Bug 1264118)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56652/diff/6-7/

Comment 44

[Matthew Wein [:mattw]]

Sorry about that. The ES test failures are fixed now.

Comment 45

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/43a83633a789
Part 1: Implement default_popup for chrome.pageAction r=kmag
https://hg.mozilla.org/integration/fx-team/rev/a80121b022f5
Part 2: Implement {set|get}Popup for chrome.pageAction r=kmag

Comment 46

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

https://hg.mozilla.org/mozilla-central/rev/43a83633a789
https://hg.mozilla.org/mozilla-central/rev/a80121b022f5

Comment 47

[CosminB]

Attachment: popupsAndroid.gif

I tested the next extensions on Firefox 56.0a1 (2017-07-24) under Android. 6.0.1:

https://addons-dev.allizom.org/en-US/firefox/addon/android-popup/
https://addons-dev.allizom.org/en-US/firefox/addon/neogaf-thread-summarizer/ 
https://addons-dev.allizom.org/en-US/firefox/addon/tweets-counter-for-twitter/ 

The pop-ups from these extensions are opened in a new tab, Matthew can you confirm that this is the expected behaviour?

Please have a look at the attached video.

Comment 48

[Luca Greco [:rpl] [:luca] [:lgreco]]

Hi Cosmin,
I confirm that this is the expected behaviour on Firefox for Android (the assigned popup url will be opened as a new tab).

Comment 49

[CosminB]

Thanks Luca!

This issue is verified as fixed on Firefox 56.0a1 under Android 6.0.1 based on comment 47 and comment 48.