[PageAction] Support show/hide on a per tab basis on Android.

VERIFIED FIXED in Firefox 56

Status

P2
normal
VERIFIED FIXED
2 years ago
7 months ago

People

(Reporter: mattw, Assigned: mattw)

Tracking

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

unspecified
mozilla56
Unspecified
Android
dev-doc-complete
Dependency tree / graph

Firefox Tracking Flags

(firefox56 verified, firefox57 verified)

Details

(Whiteboard: [pageAction]triaged)

Attachments

(7 attachments)

(Assignee)

Description

2 years ago
Currently, show and hide work globally across all tabs, ignoring the tab ID argument passed in.  Once basic support for the Tabs API is added, we can start supporting show and hide on a per tab basis.

Comment 1

2 years ago
Would you be able to take a look Matt now that bug 1260548 has landed?
Assignee: nobody → mwein

Updated

2 years ago
Priority: -- → P2
(Assignee)

Updated

2 years ago
webextensions: --- → ?

Updated

2 years ago
webextensions: ? → +
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 13

2 years ago
mozreview-review
Comment on attachment 8875597 [details]
Bug 1300811 - Part 2 - Add support for TabContext to android

https://reviewboard.mozilla.org/r/147018/#review152478
Attachment #8875597 - Flags: review?(mixedpuppy) → review+

Comment 14

2 years ago
mozreview-review
Comment on attachment 8875598 [details]
Bug 1300811 - Part 4 - Support show and hide on a per tab basis

https://reviewboard.mozilla.org/r/147020/#review152480

::: mobile/android/components/extensions/ext-pageAction.js:34
(Diff revisions 2 - 3)
>      this.icons = IconDetails.normalize({path: options.default_icon}, extension);
>  
>      this.popup = options.default_popup;
>  
> -    // Map[TabID -> Object]
> -    this.tabIdToPropertyMap = new DefaultMap(() => ({}));
> +    // Object[TabID -> Object]
> +    this.tabIdToPropertyMap = {};

Couldn't a weakmap be used here?  I'm guessing we don't have TabContext on android.  I wonder if it should be.
(Assignee)

Comment 15

2 years ago
mozreview-review-reply
Comment on attachment 8875598 [details]
Bug 1300811 - Part 4 - Support show and hide on a per tab basis

https://reviewboard.mozilla.org/r/147020/#review152480

> Couldn't a weakmap be used here?  I'm guessing we don't have TabContext on android.  I wonder if it should be.

Yeah, I think now would be a good time to add a TabContext to android.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 22

2 years ago
mozreview-review
Comment on attachment 8875597 [details]
Bug 1300811 - Part 2 - Add support for TabContext to android

https://reviewboard.mozilla.org/r/147018/#review153026

::: mobile/android/components/extensions/ext-utils.js:438
(Diff revision 3)
> +// Manages tab-specific context data and dispatches tab select and close events.
> +class TabContext {
> +  constructor(getDefaults, extension) {
> +    this.extension = extension;
> +    this.getDefaults = getDefaults;
> +    this.tabData = new Map();

Can weakmap be used here?

Comment 23

2 years ago
mozreview-review
Comment on attachment 8876976 [details]
Bug 1300811 - Part 1 - Convert PageAction to a class

https://reviewboard.mozilla.org/r/148288/#review153028
Attachment #8876976 - Flags: review?(mixedpuppy) → review+

Comment 24

2 years ago
mozreview-review
Comment on attachment 8876977 [details]
Bug 1300811 - Part 3 - Update the browser action API to use TabContext

https://reviewboard.mozilla.org/r/148290/#review153034

::: mobile/android/components/extensions/ext-browserAction.js:121
(Diff revision 1)
>  
>    /**
>     * Unregister the browser action from the BrowserActions module.
>     */
>    shutdown() {
>      BrowserActions.unregister(this.uuid);

Do you need to call tabContext.shutdown here?

::: mobile/android/components/extensions/test/mochitest/test_ext_browserAction_getTitle_setTitle.html:31
(Diff revision 1)
>      });
>  
>      async function createAndTestNewTab(title, url) {
>        // First make sure the default title is correct.
>        let defaultTitle = await browser.browserAction.getTitle({});
> -      browser.test.assertEq("Browser Action", defaultTitle, `Expected the default title to be ${defaultTitle}`);
> +      browser.test.assertEq("Browser Action", defaultTitle, `Expected the default title to be "Browser Action"`);

nit: A failure should output expected and got values, no need to say it again.  Just "Expected default title" is enough.

::: mobile/android/components/extensions/test/mochitest/test_ext_browserAction_getTitle_setTitle.html:115
(Diff revision 1)
>  
>    async function checkTab(tab, name) {
>      extension.sendMessage("select-tab", tab.id);
>      await extension.awaitMessage("tab-selected");
> +    // Give the browser action a chance to update the title.
> +    await new Promise(resolve => setTimeout(resolve, 10));

intermittent waiting to happen. Is there any way to use something like BrowserTestUtils.waitForCondition?
Attachment #8876977 - Flags: review?(mixedpuppy)

Comment 25

2 years ago
mozreview-review
Comment on attachment 8875598 [details]
Bug 1300811 - Part 4 - Support show and hide on a per tab basis

https://reviewboard.mozilla.org/r/147020/#review153046

::: mobile/android/components/extensions/ext-pageAction.js:161
(Diff revision 4)
>        return Promise.resolve();
>      }
>  
>      this.shouldShow = true;
>  
>      // TODO(robwu): Remove dependency on contentWindow from this file. It should

Unrelated, but can you add bugs for these TODO items, and put the bug #'s here?
Attachment #8875598 - Flags: review?(mixedpuppy) → review+

Comment 26

2 years ago
mozreview-review
Comment on attachment 8875599 [details]
Bug 1300811 - Part 5 - test show and hide with multiple tabs

https://reviewboard.mozilla.org/r/147022/#review153048

Try to use ContentTaskUtils here.

::: mobile/android/components/extensions/test/mochitest/test_ext_pageAction_show_hide.html:109
(Diff revision 4)
> +      await extension.awaitMessage("page-action-shown");
> +      ok(!PageActions.isShown(uuid), "The PageAction should still not be shown");
> +      extension.sendMessage("select-tab", tabId);
> +      await extension.awaitMessage("tab-selected");
> +      // Give the page action a chance to be shown.
> +      await new Promise(resolve => setTimeout(resolve, 10));

waitForCondition
Attachment #8875599 - Flags: review?(mixedpuppy) → review+

Comment 27

2 years ago
mozreview-review
Comment on attachment 8875600 [details]
Bug 1300811 - Part 6 - test setPopup and getPopup with multiple tabs

https://reviewboard.mozilla.org/r/147024/#review153052

::: mobile/android/components/extensions/test/mochitest/test_ext_pageAction_getPopup_setPopup.html:181
(Diff revision 4)
> +    PageActions.synthesizeClick(uuid);
> +    let location = await extension.awaitMessage("page-action-from-popup");
> +    ok(location.includes(expectedPopup), "The popup with the correct URL should be shown.");
> +
> +    extension.sendMessage("page-action-close-popup", {location});
> +    location = await tabClosedPromise();

unused location?  remove if not needed.
Attachment #8875600 - Flags: review?(mixedpuppy) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 34

2 years ago
mozreview-review-reply
Comment on attachment 8875597 [details]
Bug 1300811 - Part 2 - Add support for TabContext to android

https://reviewboard.mozilla.org/r/147018/#review153026

> Can weakmap be used here?

WeakMaps require the keys to be objects, and I'd like to use the tab Id as the key, since that's all that is provided to "Tab:Selected" and "Tab:Closed" and it's all that is needed for the implementation.
(Assignee)

Comment 35

2 years ago
mozreview-review-reply
Comment on attachment 8875599 [details]
Bug 1300811 - Part 5 - test show and hide with multiple tabs

https://reviewboard.mozilla.org/r/147022/#review153048

Thanks, using ContentTaskUtils worked fine

> waitForCondition

Done.
(Assignee)

Comment 36

2 years ago
mozreview-review-reply
Comment on attachment 8875600 [details]
Bug 1300811 - Part 6 - test setPopup and getPopup with multiple tabs

https://reviewboard.mozilla.org/r/147024/#review153052

> unused location?  remove if not needed.

It's used on the following line to confirm that the correct tab was closed.
(Assignee)

Comment 37

2 years ago
mozreview-review-reply
Comment on attachment 8876977 [details]
Bug 1300811 - Part 3 - Update the browser action API to use TabContext

https://reviewboard.mozilla.org/r/148290/#review153034

> Do you need to call tabContext.shutdown here?

Yes, thanks, same for ext-pageAction.js

> nit: A failure should output expected and got values, no need to say it again.  Just "Expected default title" is enough.

Done.

> intermittent waiting to happen. Is there any way to use something like BrowserTestUtils.waitForCondition?

Updated to use ContentTaskUtils.waitForCondition
(Assignee)

Comment 38

2 years ago
mozreview-review-reply
Comment on attachment 8875598 [details]
Bug 1300811 - Part 4 - Support show and hide on a per tab basis

https://reviewboard.mozilla.org/r/147020/#review153046

> Unrelated, but can you add bugs for these TODO items, and put the bug #'s here?

Sure - I filed bug 1372782 and bug 1372783.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 42

2 years ago
mozreview-review
Comment on attachment 8876977 [details]
Bug 1300811 - Part 3 - Update the browser action API to use TabContext

https://reviewboard.mozilla.org/r/148290/#review153676
Attachment #8876977 - Flags: review?(mixedpuppy) → review+

Comment 43

2 years ago
Pushed by mwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b1a39016988e
Part 1 - Convert PageAction to a class r=mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/ab64e2df938f
Part 2 - Add support for TabContext to android r=mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/b8834309f85b
Part 3 - Update the browser action API to use TabContext r=mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/76b7cff0e3b2
Part 4 - Support show and hide on a per tab basis r=mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/58da9860201b
Part 5 - test show and hide with multiple tabs r=mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/625f79374b4d
Part 6 - test setPopup and getPopup with multiple tabs r=mixedpuppy
Depends on: 1373170

Comment 45

2 years ago
Is there necessary any manual testing around this bug? If yes, could you please provide some reliable steps?
Flags: needinfo?(mwein)
Keywords: dev-doc-needed

Comment 46

a year ago
Created attachment 8898727 [details]
ShowHide.gif

This issue is verified as fixed on Fennec 56.0b3 and Fennec 57.0a1(2017-08-17) under Android 6.0.1

The show and hide is supported per tab basis, as presented in the video.
Flags: needinfo?(matthewjwein)

Updated

a year ago
Status: RESOLVED → VERIFIED
status-firefox56: fixed → verified
status-firefox57: --- → verified
I've updated the compat data for:
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/pageAction/show#Browser_compatibility
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/pageAction/hide#Browser_compatibility

Setting this dev-doc-complete, but let me know if we need anything else.
Keywords: dev-doc-needed → dev-doc-complete

Updated

7 months ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.