Closed Bug 1438666 Opened 2 years ago Closed 2 years ago

tabs.query doesn't return highlighted tabs

Categories

(WebExtensions :: Frontend, defect, P2)

57 Branch
defect

Tracking

(firefox60 verified)

VERIFIED FIXED
mozilla60
Tracking Status
firefox60 --- verified

People

(Reporter: markp.mckinney, Assigned: Oriol)

Details

(Keywords: dev-doc-complete)

Attachments

(2 files)

59 bytes, text/x-review-board-request
mixedpuppy
: review+
rpl
: review+
Details
152.03 KB, application/x-zip-compressed
Details
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.
Status: UNCONFIRMED → NEW
Component: Untriaged → WebExtensions: Frontend
Ever confirmed: true
Product: Firefox → Toolkit
Flags: needinfo?(lgreco)
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.
Assignee: nobody → oriol-bugzilla
Status: NEW → ASSIGNED
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.
Attachment #8954070 - Flags: review?(mixedpuppy) → review+
Attachment #8954070 - Flags: review?(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).
Done! How can I build and test firefox android on desktop?
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 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.
Attachment #8954070 - Flags: review?(lgreco) → review+
Flags: needinfo?(lgreco)
Keywords: checkin-needed
Priority: -- → P2
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/ff1003dc4e6e
Let tabs.query check if a tab is highlighted r=mixedpuppy,rpl
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ff1003dc4e6e
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
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.
Keywords: dev-doc-needed
Attached file 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.
Status: RESOLVED → VERIFIED
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.