1438666 - tabs.query doesn't return highlighted tabs

Status: VERIFIED FIXED

(WebExtensions :: Frontend, defect, P2)

Description

[Mark]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/64.0.3282.167 Safari/537.36

Steps to reproduce:

Run a tab query for highlighted tabs in a web extension like: browser.tabs.query({highlighted: true}, (tabs) => console.log(tabs))

Using active instead of highlighted will return the active tabs that also have highlighted set to true, but highlighted should still work.


Actual results:

An empty array is returned.


Expected results:

An array with the highlighted tabs should have been returned. This functionality works in Chrome.

Comment 1

[Oriol Brufau [:Oriol]]

Attachment: Bug 1438666 - Let tabs.query check if a tab is highlighted

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

Comment 2

[Oriol Brufau [:Oriol]]

What happens is that the highlighted property is properly checked
https://searchfox.org/mozilla-central/source/toolkit/components/extensions/ext-tabs-base.js#518-527

but the problem is that it doesn't exist, so it always returns undefined.

Therefore the provided boolean value is never matched, and no tab is returned.

Comment 3

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8954070 [details]
Bug 1438666 - Let tabs.query check if a tab is highlighted

https://reviewboard.mozilla.org/r/223218/#review229222

Android needs this fix as well in mobile/android/components/extensions//ext-utils.js.  Luca can review the android, and perhaps comment on the difference between active and selected there. r+ from me for the browser side.

Comment 4

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

Comment on attachment 8954070 [details]
Bug 1438666 - Let tabs.query check if a tab is highlighted

https://reviewboard.mozilla.org/r/223218/#review229240

::: browser/components/extensions/ext-browser.js:662
(Diff revision 1)
> +  get highlighted() {
> +    return this.nativeTab.selected;
> +  }

I confirm what Shane anticipated in comment 3:

the `highlighted` is also missing from the Firefox for Android WebExtensions API modules,
it should be defined in "mobile/android/components/extensions/ext-utils.js",
and in my opinion it should behave (and be implemented) as the `selected` getter (https://searchfox.org/mozilla-central/rev/b469db5c90d618f4b202d5ef280e1a78224eb43b/mobile/android/components/extensions/ext-utils.js#547-549):

```
  get highlighted() {
    return this.nativeTab.getActive();
  }
```

Unfortunately it doesn't seem that we have a test file which is specifically about the tabs.query API for Firefox for Android (like the browser_ext_tabs_query.js test file updated in the current version of this patch), but it looks that we could add the additional assertions to mobile/android/components/extensions/test/mochitest/test_ext_tabs_create.html test file (or create a new test file if the additions can make that test not clear enough).

About the difference between `selected` and `active` getters on Android:

The extension popups are represented by tabs on Firefox for Android and so we ensure that, while the extension popup is the currently selected tab, the active tab is actually the tab that was selected when the tab has been opened, which is needed to allow an extension popup to behave similarly to how it would behave on Desktop (e.g. if it queries the active tab it should get the tab that opened the extension popup, and if it calls tabs.executeScript or tabs.insertCSS without the tabId parameter, the API calls should target the tab that opened the extension popup).

Comment 5

[Oriol Brufau [:Oriol]]

Comment on attachment 8954070 [details]
Bug 1438666 - Let tabs.query check if a tab is highlighted

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

Comment 6

[Oriol Brufau [:Oriol]]

Done! How can I build and test firefox android on desktop?

Comment 7

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

Comment on attachment 8954070 [details]
Bug 1438666 - Let tabs.query check if a tab is highlighted

https://reviewboard.mozilla.org/r/223218/#review229342

::: mobile/android/components/extensions/test/mochitest/test_ext_tabs_query.html:39
(Diff revision 2)
> +    },
> +  });
> +
> +  let tabs = [];
> +  for (let url of ["http://example.com/", "http://example.net/", "http://test1.example.org/MochiKit/"]) {
> +    tabs.push(await BrowserTestUtils.openNewForegroundTab(gBrowser, url));

BrowserTestUtils can't be used in the android mochitest, but you can open and close Fennec tabs using BrowserApp.addTab and BrowserApp.closeTab, the BrowserApp object can be retrieved using Services.wm.getMostRecentWindow:

```
const {BrowserApp} = Services.wm.getMostRecentWindow("navigator:browser");

...
```

Here is a couple of existent tests which are opening and closing tabs using this strategy:

- https://searchfox.org/mozilla-central/source/mobile/android/components/extensions/test/mochitest/test_ext_browserAction_getPopup_setPopup.html
- https://searchfox.org/mozilla-central/source/mobile/android/components/extensions/test/mochitest/test_ext_options_ui.html

Comment 8

[Oriol Brufau [:Oriol]]

Comment on attachment 8954070 [details]
Bug 1438666 - Let tabs.query check if a tab is highlighted

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

Comment 9

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

Comment on attachment 8954070 [details]
Bug 1438666 - Let tabs.query check if a tab is highlighted

https://reviewboard.mozilla.org/r/223218/#review230066

Looks good to me.

Thanks a lot for fixing it on Firefox for Android as well.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ff1003dc4e6e
Let tabs.query check if a tab is highlighted r=mixedpuppy,rpl

Comment 11

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/ff1003dc4e6e

Comment 12

[Oriol Brufau [:Oriol]]

https://developer.mozilla.org/Add-ons/WebExtensions/API/tabs/query says that queryInfo.highlighted is supported since Firefox 45 and 54 for Android. I guess previously it might throw an error or something but I don't consider it as working until this bug landed.

Comment 13

[CosminB]

Attachment: Bug1420778.zip

This issue is verified as fixed on Firefox 60.0a1 (20180311220116) under Windows 7 64-bit and Mac OS X 10.13.2.

browser.tabs.query resolves to the expected tab details for highlighted: true (e.g. the selected tab is included in the resolved array of tab details and the highlighted and selected properties are set to true)

Please see the attached video.

Comment 14

[Will Bamberg [:wbamberg]]

Compat table is updated: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/query#Browser_compatibility