YUKI "Piro" Hiroshi Reporter
	Description • 6 years ago

By the bug 1458010 and the bug 1458013, now tabs can have multiselected status. On the other hand, there is no way to know the status via WebExtensions APIs, and it is also impossible to change the status via APIs. The status should be exposed to WE APIs for tab related addons.

	Geoffrey De Belie (Smile4ever)
	Comment 1 • 6 years ago

I believe this is achieved in Chrome by setting highlighted to true on the tabs that are selected.

https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/highlight
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/Tab

So the title of this bug could be "Support browser.tabs.highlight".

	YUKI "Piro" Hiroshi Reporter
	Comment 3 • 6 years ago

I've looked the patch and I think something mechanism to notify updated "highlighted" status to listeners of tabs.onUpdated looks necessary.

	Oriol Brufau [:Oriol] Assignee
	Comment 4 • 6 years ago

Not sure what to do if the pref is disabled. Then a webextension may highlight tabs, and the user won't be able to clear them by clicking a tab.

Also, some things I did for compatibility with Chrome, but I'm not sure if they are good ideas:
 - The schema enforces a minimum index of 0 when using an array, but not when using a single integer, then you only get a rejection. So it's not consistent.
 - The method rejects if the array is empty, but this could be enforced in the schema.
 - The returned window is populated with all its tabs (even if you only highlighted one). This will be slow if there are lots of tabs, IMO seems unnecessary, and the documentation doesn't mention it.


(In reply to YUKI "Piro" Hiroshi from comment #3)
> I've looked the patch and I think something mechanism to notify updated
> "highlighted" status to listeners of tabs.onUpdated looks necessary.

This can be a different bug, together with onHighlighted, which should be moved outside of bug 1465170 anyways, because it's not related at all to tabs.query

	Jared Wein [:jaws] (please needinfo? me)
	Comment 5 • 6 years ago

(In reply to Oriol Brufau [:Oriol] from comment #4)
> Not sure what to do if the pref is disabled. Then a webextension may
> highlight tabs, and the user won't be able to clear them by clicking a tab.

If the pref is disabled then we should just do nothing I would propose. Extensions could check if the `tabs.Tab.highlighted` to see if it was effective.

	Oriol Brufau [:Oriol] Assignee
	Comment 6 • 6 years ago

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #5)
> If the pref is disabled then we should just do nothing I would propose.
> Extensions could check if the `tabs.Tab.highlighted` to see if it was
> effective.

Do nothing and throw an error or just fail silently?

Ideally I would like browser.tabs.highlight to become undefined when the pref is false, but it seems there is no easy way to do this.

	Shane Caraveo (:mixedpuppy)
	Comment 7 • 6 years ago

If the pref is false, highlight should behave however it currently behaves.

	Oriol Brufau [:Oriol] Assignee
	Comment 8 • 6 years ago

(In reply to Shane Caraveo (:mixedpuppy) from comment #7)
> If the pref is false, highlight should behave however it currently behaves.

Currently it's not supported:

  browser.tabs.highlight; // undefined

How can I make this depend on a pref?

	Shane Caraveo (:mixedpuppy)
	Comment 9 • 6 years ago
mozreview-review

Comment on attachment 8982916 [details]
Bug 1464862 - Expose multiselected status to "tabs.Tab.highlighted", and allow to change it via "browser.tabs.highlight()"

https://reviewboard.mozilla.org/r/248742/#review258510

::: browser/components/extensions/parent/ext-tabs.js:1240
(Diff revision 1)
>            }
>            return hidden;
>          },
> +
> +        highlight(highlightInfo) {
> +          let {windowId, tabs} = highlightInfo;

Check the multiselect pref here and throw an error if it is false.

::: browser/components/extensions/test/browser/browser_ext_tabs_highlight.js:7
(Diff revision 1)
> +/* vim: set sts=2 sw=2 et tw=80: */
> +/* global gBrowser SessionStore */
> +"use strict";
> +
> +add_task(async function test_highlighted() {
> +  let extension = ExtensionTestUtils.loadExtension({

You need to use specialpowers.pushprefenv to ensure the multiselect pref is true.

::: mobile/android/components/extensions/ext-utils.js:523
(Diff revision 1)
> +  get highlighted() {
> +    return this.active;
> +  }

be sure this platform difference is documented on mdn correctly.

	Oriol Brufau [:Oriol] Assignee
	Comment 13 • 6 years ago

The selected tab should be part of the multi-selection (bug 1468443).

	Pulsebot
	Comment 14 • 6 years ago

Pushed by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4e78d806cb63
Expose multiselected status to "tabs.Tab.highlighted", and allow to change it via "browser.tabs.highlight()" r=mixedpuppy

	Raul Gurzau (:RaulG)
	Comment 15 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/4e78d806cb63

	Vlad Jiman
	Comment 16 • 6 years ago

Is manual testing required on this bug? If yes, please provide some STR and the proper extension(if required) or set the “qe-verify -“ flag.

Thanks!

	Oriol Brufau [:Oriol] Assignee
	Comment 17 • 6 years ago

Covered by automated tests

	Irene Smith
	Comment 18 • 6 years ago

Created a pull request to update the compatibility data and added the following to the release notes:

tabs.Tab property properly reflects which tabs in a browser window are selected (highlighted) and tabs.highlight supports changing the highlighted status of multiple tabs (bug 1464862).

	Oriol Brufau [:Oriol] Assignee
	Comment 19 • 6 years ago

Looks good, thanks