Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Implement pageAction API for open extension API

RESOLVED FIXED in Firefox 44

Status

()

Toolkit
WebExtensions: Untriaged
RESOLVED FIXED
2 years ago
11 months ago

People

(Reporter: Mindaugas <LA-MJ>, Assigned: kmag)

Tracking

(Blocks: 1 bug, {dev-doc-complete, DevAdvocacy})

unspecified
mozilla44
dev-doc-complete, DevAdvocacy
Points:
---
Dependency tree / graph
Bug Flags:
blocking-webextensions +

Firefox Tracking Flags

(firefox44 fixed)

Details

(Whiteboard: [pageAction])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Reporter)

Description

2 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

Updated

2 years ago
Blocks: 1161828
Status: UNCONFIRMED → NEW
Ever confirmed: true
Lack of pageAction blocks porting the Reddit Enhancement Suite (http://redditenhancementsuite.com/)
Keywords: DevAdvocacy
Specifically, it just requires show(), hide(), setIcon(), and the onClicked event. Title and popup functions aren't needed.
Does it need to be a pageAction, or could they use a browserAction while waiting for us to implement it?
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.
Blocks: 1208765

Comment 5

2 years ago
Same problem here.

Comment 6

2 years ago
I'm desperately waiting for pageAction! :)
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
(Assignee)

Updated

2 years ago
Assignee: nobody → kmaglione+bmo
Keywords: dev-doc-needed

Updated

2 years ago
Iteration: --- → 44.2 - Oct 19
(Assignee)

Comment 8

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

Updated

2 years ago
Whiteboard: [pageAction]
(Assignee)

Comment 9

2 years ago
Created 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
Attachment #8672432 - Flags: review?(wmccloskey)
(Assignee)

Comment 10

2 years ago
Created 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
Attachment #8672433 - Flags: review?(wmccloskey)
(Assignee)

Comment 11

2 years ago
Created 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
Attachment #8672434 - Flags: review?(wmccloskey)
(Assignee)

Updated

2 years ago
Blocks: 1203738
(Assignee)

Updated

2 years ago
Blocks: 1190321
(Assignee)

Updated

2 years ago
Blocks: 1200674
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.)
Attachment #8672433 - Flags: review?(wmccloskey) → review+
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
Attachment #8672432 - Flags: review?(wmccloskey) → review+
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.
Attachment #8672434 - Flags: review?(wmccloskey) → review+

Comment 15

2 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!
(Assignee)

Comment 16

2 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.

Updated

2 years ago
Blocks: 1214433
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)  ;)
(Assignee)

Comment 18

2 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
Attachment #8672432 - Attachment description: MozReview Request: Bug 1197422 - Part 1: [webext] Use the correct URL for background page contexts. r?billm → MozReview Request: Bug 1197422 - Part 1: [webext] Use the correct URL for background page contexts. r=billm
(Assignee)

Comment 19

2 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
(Assignee)

Comment 20

2 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
(Assignee)

Comment 21

2 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
Attachment #8672433 - Attachment description: MozReview Request: Bug 1197422 - Part 2: [webext] Implement the pageAction API. r?billm → MozReview Request: Bug 1197422 - Part 2: [webext] Implement the pageAction API. r=billm ui-r=bwinton
(Assignee)

Updated

2 years ago
Attachment #8672434 - Attachment description: MozReview Request: Bug 1197422 - Part 3: [webext] Update browserAction API to use the same context tracking code as pageAction. r?billm → MozReview Request: Bug 1197422 - Part 3: [webext] Update browserAction API to use the same context tracking code as pageAction. r=billm
(Assignee)

Comment 22

2 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
(Assignee)

Updated

2 years ago
Attachment #8672433 - Flags: ui-review?(bwinton)
(Assignee)

Comment 23

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0c86ec80720
(Assignee)

Comment 24

2 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
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+…
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…  ;)
(Assignee)

Comment 27

2 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
Okay, I'll re-test with your add-on tomorrow morning and (most likely) give it a ui-r+…  ;)
Thanks!
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!
Attachment #8672433 - Flags: ui-review?(bwinton) → ui-review+

Updated

2 years ago
Iteration: 44.2 - Oct 19 → 44.3 - Nov 2
(Assignee)

Comment 30

2 years ago
Excellent. Thanks for the reviews.
Iteration: 44.3 - Nov 2 → 44.2 - Oct 19
(Assignee)

Comment 31

2 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
(Assignee)

Comment 32

2 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
https://hg.mozilla.org/mozilla-central/rev/c39ec146b6a0
https://hg.mozilla.org/mozilla-central/rev/84df75fc0206
https://hg.mozilla.org/mozilla-central/rev/958f04b7804f
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44

Updated

2 years ago
Flags: blocking-webextensions+
(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.
(Assignee)

Comment 35

2 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?
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.
Blocks: 1218175
No longer blocks: 1218175
(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
-> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/pageAction
Keywords: dev-doc-needed → dev-doc-complete

Updated

11 months ago
See Also: → bug 1294920
You need to log in before you can comment on or make changes to this bug.