Mindaugas <LA-MJ> Reporter
	Description • 9 years ago

https://developer.chrome.com/extensions/pageAction

API is listed on https://wiki.mozilla.org/WebExtensions#List_of_APIs_we_will_likely_support_in_the_future

	Dan Callahan [:callahad]
	Comment 1 • 9 years ago

Lack of pageAction blocks porting the Reddit Enhancement Suite (http://redditenhancementsuite.com/)

	Dan Callahan [:callahad]
	Comment 2 • 9 years ago

Specifically, it just requires show(), hide(), setIcon(), and the onClicked event. Title and popup functions aren't needed.

	Blake Winton (:bwinton) (:☕️)
	Comment 3 • 9 years ago

Does it need to be a pageAction, or could they use a browserAction while waiting for us to implement it?

	Dan Callahan [:callahad]
	Comment 4 • 9 years ago

It has used a pageAction in Chrome for years, so switching to a browserAction in a unified codebase wouldn't be viable, and forking the codebase at that point seems counterproductive.

Since pageActions and browserActions are defined in the manifest, we can't even use feature detection to conditionally use one over the other; we'd have to maintain two separate manifests.

	nico.schloemer
	Comment 5 • 9 years ago

Same problem here.

	andre
	Comment 6 • 9 years ago

I'm desperately waiting for pageAction! :)

	Blake Winton (:bwinton) (:☕️)
	Comment 7 • 9 years ago

Would either of you like to take a stab at helping to implement it?  It should be mostly JavaScript-based code, and I suspect it will be reasonably similar to the browserAction code at https://dxr.mozilla.org/mozilla-central/source/browser/components/extensions/ext-browserAction.js

	Kris Maglione [:kmag] Assignee
	Comment 8 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=47dbb67b0963

	Kris Maglione [:kmag] Assignee
	Comment 9 • 9 years ago

Bug 1197422 - Part 1: [webext] Use the correct URL for background page contexts. r?billm

	Kris Maglione [:kmag] Assignee
	Comment 10 • 9 years ago

Bug 1197422 - Part 2: [webext] Implement the pageAction API. r?billm

	Kris Maglione [:kmag] Assignee
	Comment 11 • 9 years ago

Bug 1197422 - Part 3: [webext] Update browserAction API to use the same context tracking code as pageAction. r?billm

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 12 • 9 years ago

Comment on attachment 8672433 [details]
MozReview Request: Bug 1197422 - Part 2: [webext] Implement the pageAction API. r=billm ui-r=bwinton

https://reviewboard.mozilla.org/r/21705/#review19671

Thanks for the attention to detail here. It really shows.

I think it would be a good idea to recursively localize all the string properties in the manifest. That would really simplify this code. That can be a follow-up though.

::: browser/components/extensions/ext-pageAction.js:228
(Diff revision 1)
> +        // Note: Chrome resolves arguments to setIcon relative to the calling
> +        // context, but resolves arguments to setPopup relative to the extension
> +        // root.

Thanks for investigating this. It sounds like Chrome's behavior here might be an accident. I think it would be fine to break compatibility here. Unconditionally resolving relative to the context seems like the right choice. People can always use "/foo" if they want it to be relative to the base URI.

::: browser/components/extensions/ext-utils.js:22
(Diff revision 1)
> +  SIZES: Object.freeze(["19", "38"]),

Why freeze this?

::: browser/components/extensions/ext-utils.js:129
(Diff revision 1)
> +  panel.setAttribute("class", "browser-action-panel");

Maybe browser-extension-panel?

::: browser/components/extensions/ext-utils.js:137
(Diff revision 1)
> +    var anchor = document.getAnonymousElementByAttribute(node, "class", "toolbarbutton-icon");

Please put |let anchor;| above the start of this |if| block. Then remove |var| here.

::: browser/components/extensions/ext-utils.js:147
(Diff revision 1)
> +    context.unload();

Maybe we should null-check context in case the popup is hidden before the panel loads.

::: browser/components/extensions/ext-utils.js:170
(Diff revision 1)
> +      browser.removeEventListener("load", contentLoadListener);

My bug, but I just noticed that we need to pass in true here for useCapture.

::: browser/components/extensions/ext-utils.js:414
(Diff revision 1)
> +      yield e.getNext();

I think the default behavior should be to skip windows that aren't yet fully loaded. It's very easy to forget that check, and I don't see any reason why we wouldn't want it everywhere.

::: browser/components/extensions/ext-utils.js:533
(Diff revision 1)
>      let e = Services.wm.getEnumerator("navigator:browser");

Can you change this one to use WindowListManager.browserWindows() too? I don't think it matters here whether the window is has finished loading.

::: browser/components/extensions/test/browser/browser_ext_pageAction_popup.js:75
(Diff revision 1)
> +            // Return to the event loop before moving to the next test, so that
> +            // our panel is actually shown before we attempt to close it. Since
> +            // the popup is not actually opened until it's fully loaded and our
> +            // scripts have run, if we don't do this, we hit a race where we
> +            // attempt to close the popup before it's actually been opened, and
> +            // it isn't removed from the document before our next test.
> +            browser.test.sendMessage("next-test");

This seems really flaky, especially once we run extensions out of process. Can we instead have the code in the test that receives next-test wait until the plugin is showing and then hide it?

::: browser/components/extensions/test/browser/browser_ext_pageAction_popup.js:105
(Diff revision 1)
> +            browser.test.notifyPass("tests-done");

Try to make this more specific to what is being tested than tests-done. Maybe "page-action-test". The thing it's designed to defeat is that we accidentially load the wrong extension but it still sends some generic "test done" message and the test passes. (This used to happen because of a weird JAR caching issue.)

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 13 • 9 years ago

Comment on attachment 8672432 [details]
MozReview Request: Bug 1197422 - Part 1: [webext] Use the correct URL for background page contexts. r=billm

https://reviewboard.mozilla.org/r/21703/#review19665

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 14 • 9 years ago

Comment on attachment 8672434 [details]
MozReview Request: Bug 1197422 - Part 3: [webext] Update browserAction API to use the same context tracking code as pageAction. r=billm

https://reviewboard.mozilla.org/r/21707/#review19673

::: browser/components/extensions/ext-browserAction.js:100
(Diff revision 1)
> -  // will be null if we don't know the current tab yet (during
> +  updateButton(node, context) {

Calling this param |context| is pretty confusing since we already have ExtensionContext, which is usually called context. I would maybe call this TabData/tabData.

	:Gijs (he/him)
	Comment 15 • 9 years ago

Coming in here via bug 1123760 and bwinton - can you ensure that when tooltiptext is set, the aria-label attribute is also set (to the same value), so that the buttons are usable by screenreader and other AT users? Thank you!

	Kris Maglione [:kmag] Assignee
	Comment 16 • 9 years ago

(In reply to Bill McCloskey (:billm) from comment #12)
> Thanks for the attention to detail here. It really shows.

Thanks

> I think it would be a good idea to recursively localize all the string
> properties in the manifest. That would really simplify this code. That can
> be a follow-up though.

I was coming to that conclusion too. I'll try to find out
exactly what manifest strings Chrome localizes.

> ::: browser/components/extensions/ext-pageAction.js:228
> (Diff revision 1)
> > +        // Note: Chrome resolves arguments to setIcon relative to the calling
> > +        // context, but resolves arguments to setPopup relative to the extension
> > +        // root.
> 
> Thanks for investigating this. It sounds like Chrome's behavior here might
> be an accident. I think it would be fine to break compatibility here.
> Unconditionally resolving relative to the context seems like the right
> choice. People can always use "/foo" if they want it to be relative to the
> base URI.

I'm not sure. I'd definitely prefer them to all be relative, but
I wonder if that will break any extensions.

> ::: browser/components/extensions/ext-utils.js:22
> (Diff revision 1)
> > +  SIZES: Object.freeze(["19", "38"]),
> 
> Why freeze this?

I can't think of a particularly good reason. I'll leave it
unfrozen.

> ::: browser/components/extensions/ext-utils.js:129
> (Diff revision 1)
> > +  panel.setAttribute("class", "browser-action-panel");
> 
> Maybe browser-extension-panel?

Yeah, I thought about changing that. browser-extension-panel
sounds good.

> ::: browser/components/extensions/ext-utils.js:147
> (Diff revision 1)
> > +    context.unload();
> 
> Maybe we should null-check context in case the popup is hidden before the
> panel loads.

I don't think that's possible. We never actually show the popup
before we've created the context, so it should never be null at
this point.

> ::: browser/components/extensions/ext-utils.js:414
> (Diff revision 1)
> > +      yield e.getNext();
> 
> I think the default behavior should be to skip windows that aren't yet fully
> loaded. It's very easy to forget that check, and I don't see any reason why
> we wouldn't want it everywhere.

Yeah, that sounds right.

> :::
> browser/components/extensions/test/browser/browser_ext_pageAction_popup.js:75
> (Diff revision 1)
> > +            // Return to the event loop before moving to the next test, so that
> > +            // our panel is actually shown before we attempt to close it. Since
> > +            // the popup is not actually opened until it's fully loaded and our
> > +            // scripts have run, if we don't do this, we hit a race where we
> > +            // attempt to close the popup before it's actually been opened, and
> > +            // it isn't removed from the document before our next test.
> > +            browser.test.sendMessage("next-test");
> 
> This seems really flaky, especially once we run extensions out of process.
> Can we instead have the code in the test that receives next-test wait until
> the plugin is showing and then hide it?

I think that with OOP extensions, it should work even without
the setTimeout. But I agree it seems flaky. I initially wrote a
popupshown handler to wait for it to open before attempting to
close it, but that felt like an even worse solution. I think the
problem is that the popup handling itself is fragile, and I
thought it made more sense to file a follow-up but to fix that
rather than do too much to work around it.

In any case, yeah, this is a bad solution. I'll replace it with
a promisePopupShown helper in the outer context, and file a
follow-up but to make the popup handling code more robust.

> :::
> browser/components/extensions/test/browser/browser_ext_pageAction_popup.js:
> 105
> (Diff revision 1)
> > +            browser.test.notifyPass("tests-done");
> 
> Try to make this more specific to what is being tested than tests-done.
> Maybe "page-action-test". The thing it's designed to defeat is that we
> accidentially load the wrong extension but it still sends some generic "test
> done" message and the test passes. (This used to happen because of a weird
> JAR caching issue.)

Makes sense. I was wondering about that.

(In reply to Bill McCloskey (:billm) from comment #14)
> ::: browser/components/extensions/ext-browserAction.js:100
> (Diff revision 1)
> > -  // will be null if we don't know the current tab yet (during
> > +  updateButton(node, context) {
> 
> Calling this param |context| is pretty confusing since we already have
> ExtensionContext, which is usually called context. I would maybe call this
> TabData/tabData.

Yeah, I'll change it elsewhere too.

(In reply to :Gijs Kruitbosch from comment #15)
> Coming in here via bug 1123760 and bwinton - can you ensure that when
> tooltiptext is set, the aria-label attribute is also set (to the same
> value), so that the buttons are usable by screenreader and other AT users?
> Thank you!

Definitely. Thanks for catching that.

	Blake Winton (:bwinton) (:☕️)
	Comment 17 • 9 years ago

https://reviewboard.mozilla.org/r/21701/#review19833

::: browser/components/extensions/ext-pageAction.js:108
(Diff revision 1)
> +    button.setAttribute("class", "urlbar-icon");

I think we probably want to add a limit on the width either here or in `browser/themes/\*/browser.css`, to avoid stuff like ![this](https://dl.dropboxusercontent.com/u/2301433/Firefox/WebExtensions/pageAction.png)  ;)

	Kris Maglione [:kmag] Assignee
	Comment 18 • 9 years ago

Comment on attachment 8672432 [details]
MozReview Request: Bug 1197422 - Part 1: [webext] Use the correct URL for background page contexts. r=billm

Bug 1197422 - Part 1: [webext] Use the correct URL for background page contexts. r=billm

	Kris Maglione [:kmag] Assignee
	Comment 19 • 9 years ago

Comment on attachment 8672432 [details]
MozReview Request: Bug 1197422 - Part 1: [webext] Use the correct URL for background page contexts. r=billm

Bug 1197422 - Part 1: [webext] Use the correct URL for background page contexts. r=billm

	Kris Maglione [:kmag] Assignee
	Comment 20 • 9 years ago

Comment on attachment 8672432 [details]
MozReview Request: Bug 1197422 - Part 1: [webext] Use the correct URL for background page contexts. r=billm

Bug 1197422 - Part 1: [webext] Use the correct URL for background page contexts. r=billm

	Kris Maglione [:kmag] Assignee
	Comment 21 • 9 years ago

Comment on attachment 8672433 [details]
MozReview Request: Bug 1197422 - Part 2: [webext] Implement the pageAction API. r=billm ui-r=bwinton

Bug 1197422 - Part 2: [webext] Implement the pageAction API. r=billm ui-r=bwinton

	Kris Maglione [:kmag] Assignee
	Comment 22 • 9 years ago

Comment on attachment 8672434 [details]
MozReview Request: Bug 1197422 - Part 3: [webext] Update browserAction API to use the same context tracking code as pageAction. r=billm

Bug 1197422 - Part 3: [webext] Update browserAction API to use the same context tracking code as pageAction. r=billm

	Kris Maglione [:kmag] Assignee
	Comment 23 • 9 years ago

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

	Kris Maglione [:kmag] Assignee
	Comment 24 • 9 years ago

(In reply to Kris Maglione [:kmag] from comment #16)
> (In reply to Bill McCloskey (:billm) from comment #12)
> > Thanks for investigating this. It sounds like Chrome's behavior here might
> > be an accident. I think it would be fine to break compatibility here.
> > Unconditionally resolving relative to the context seems like the right
> > choice. People can always use "/foo" if they want it to be relative to the
> > base URI.
> 
> I'm not sure. I'd definitely prefer them to all be relative, but
> I wonder if that will break any extensions.

Documented here:

https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Chrome_incompatibilities#browserAction

	Blake Winton (:bwinton) (:☕️)
	Comment 25 • 9 years ago

https://reviewboard.mozilla.org/r/21701/#review19963

I like how it looks from what I saw, but I can't get [this test addon](https://dl.dropboxusercontent.com/u/2301433/Firefox/WebExtensions/pageActionTest.xpi) to pop up the popup, and I'ld like to see how that looks before I give it the ui-r+…

	Blake Winton (:bwinton) (:☕️)
	Comment 26 • 9 years ago

https://reviewboard.mozilla.org/r/21701/#review19963

(It's got at least a 50% chance of being something I've done wrong, but I'm not sure what it is.  The previous version tried calling `chrome.pageAction.show()`, which totally failed.  And the next version tried using `chrome.tabs.getCurrent`, which also didn't work…  ;)

	Kris Maglione [:kmag] Assignee
	Comment 27 • 9 years ago

(In reply to Blake Winton (:bwinton) (:☕️) from comment #25)
> https://reviewboard.mozilla.org/r/21701/#review19963
> 
> I like how it looks from what I saw, but I can't get [this test
> addon](https://dl.dropboxusercontent.com/u/2301433/Firefox/WebExtensions/
> pageActionTest.xpi) to pop up the popup, and I'ld like to see how that looks
> before I give it the ui-r+…

I tried that add-on, and it worked for me, albeit with the sizing issues from bug 1199052. In any case, it's probably not a great idea to test with a remote document. We don't show the popup until after the page loads (actually, this probably isn't true. We probably show the popup as soon as any element in the page fires a load event, which may have something to do with the sizing problems, but I digress...), so using a remote document adds all sorts of non-determinancy. Also probably some races, if you click the button more than once.

Anyway, these things are all on the list of follow-up bugs I need to file, and they weren't introduced by this patch. In the mean time, this add-on uses a local file for the popup and a pre-sized img element, so it should work reliably: https://people.mozilla.com/~kmaglione/test-panel.xpi

	Blake Winton (:bwinton) (:☕️)
	Comment 28 • 9 years ago

Okay, I'll re-test with your add-on tomorrow morning and (most likely) give it a ui-r+…  ;)
Thanks!

	Blake Winton (:bwinton) (:☕️)
	Comment 29 • 9 years ago

Comment on attachment 8672433 [details]
MozReview Request: Bug 1197422 - Part 2: [webext] Implement the pageAction API. r=billm ui-r=bwinton

Looks awesome!  There's some polish work to be done on the panel itself, but those bugs are already filed, and are only tangentially related to this.  :)

Thanks for the patch and the add-on!

	Kris Maglione [:kmag] Assignee
	Comment 30 • 9 years ago

Excellent. Thanks for the reviews.

	Kris Maglione [:kmag] Assignee
	Comment 31 • 9 years ago

https://hg.mozilla.org/integration/fx-team/rev/c39ec146b6a056b15ea157b126934b69c2a6cc94
Bug 1197422 - Part 1: [webext] Use the correct URL for background page contexts. r=billm

https://hg.mozilla.org/integration/fx-team/rev/84df75fc0206c25ef794ffdebf7104c23b30d4de
Bug 1197422 - Part 2: [webext] Implement the pageAction API. r=billm ui-r=bwinton

https://hg.mozilla.org/integration/fx-team/rev/958f04b7804fa587506b35b30ec616f986b637ca
Bug 1197422 - Part 3: [webext] Update browserAction API to use the same context tracking code as pageAction. r=billm

	Kris Maglione [:kmag] Assignee
	Comment 32 • 9 years ago

Updated:

https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Chrome_incompatibilities
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json

Created:

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

	Carsten Book [:Tomcat]
	Comment 33 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/c39ec146b6a0
https://hg.mozilla.org/mozilla-central/rev/84df75fc0206
https://hg.mozilla.org/mozilla-central/rev/958f04b7804f

	Adrian Aichner [:anaran]
	Comment 34 • 9 years ago

(In reply to Kris Maglione [:kmag] from comment #27)
> (In reply to Blake Winton (:bwinton) (:☕️) from comment #25)
> > https://reviewboard.mozilla.org/r/21701/#review19963
> > 
> > I like how it looks from what I saw, but I can't get [this test
> > addon](https://dl.dropboxusercontent.com/u/2301433/Firefox/WebExtensions/
> > pageActionTest.xpi) to pop up the popup, and I'ld like to see how that looks
> > before I give it the ui-r+…
> 
> I tried that add-on, and it worked for me, albeit with the sizing issues
> from bug 1199052. In any case, it's probably not a great idea to test with a
> remote document. We don't show the popup until after the page loads
> (actually, this probably isn't true. We probably show the popup as soon as
> any element in the page fires a load event, which may have something to do
> with the sizing problems, but I digress...), so using a remote document adds
> all sorts of non-determinancy. Also probably some races, if you click the
> button more than once.
> 
> Anyway, these things are all on the list of follow-up bugs I need to file,
> and they weren't introduced by this patch. In the mean time, this add-on
> uses a local file for the popup and a pre-sized img element, so it should
> work reliably: https://people.mozilla.com/~kmaglione/test-panel.xpi

Is this bug really resolved?

Starting with your .xpi link above I see a page_action and browser_action in the manifest.

The browser_action (a clickable icon on the toolbar, displaying a shaggy image in a popup) works fine.

I do not, however, see the icon in the address bar as a page_action would do.

I also tried removing the browser_action, but that did not make the page_action work.

To me this bug looks unresolved.

	Kris Maglione [:kmag] Assignee
	Comment 35 • 9 years ago

(In reply to Adrian Aichner [:anaran] from comment #34)
> Is this bug really resolved?

Yes.

> I do not, however, see the icon in the address bar as a page_action would do.

What build are you testing on?

	Adrian Aichner [:anaran]
	Comment 36 • 9 years ago

I can confirm this fixed in 44.0a1 (2015-10-23) on Windows XP.
My testing last night was on ubuntu and I had forgotten that firefox-trunk there does not update nightly.
I was still at 2015-10-18 there, so that would explain it.
Sorry for the false alarm.

I noticed your test-panel.xpi uses the browser namespace as opposed to the chrome namespace to access webextensions APIs. Is that something people should be doing right now?

Should the page_action already be working in nightly firefox os as well?

I do have Build Identifier 20151023030241 there and have been testing https://github.com/mdn/webextensions-examples.

	Adrian Aichner [:anaran]
	Comment 37 • 9 years ago

(In reply to Kris Maglione [:kmag] from comment #27)
> (In reply to Blake Winton (:bwinton) (:☕️) from comment #25)
> > https://reviewboard.mozilla.org/r/21701/#review19963
> > 
> > I like how it looks from what I saw, but I can't get [this test
> > addon](https://dl.dropboxusercontent.com/u/2301433/Firefox/WebExtensions/
> > pageActionTest.xpi) to pop up the popup, and I'ld like to see how that looks
> > before I give it the ui-r+…
> 
> I tried that add-on, and it worked for me, albeit with the sizing issues
> from bug 1199052. In any case, it's probably not a great idea to test with a
> remote document. We don't show the popup until after the page loads
> (actually, this probably isn't true. We probably show the popup as soon as
> any element in the page fires a load event, which may have something to do
> with the sizing problems, but I digress...), so using a remote document adds
> all sorts of non-determinancy. Also probably some races, if you click the
> button more than once.
> 
> Anyway, these things are all on the list of follow-up bugs I need to file,
> and they weren't introduced by this patch. In the mean time, this add-on
> uses a local file for the popup and a pre-sized img element, so it should
> work reliably: https://people.mozilla.com/~kmaglione/test-panel.xpi

How about this, Kris?

https://github.com/mdn/webextensions-examples/pull/11

	Will Bamberg [:wbamberg]
	Comment 38 • 8 years ago

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