Closed Bug 1472305 Opened 6 years ago Closed 6 years ago

browser.tabs.highlight() with single tab should not set multiselected=true

Categories

(WebExtensions :: Frontend, defect)

Product:
WebExtensions
Component:
Frontend
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox63 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: Oriol, Assigned: Oriol)

References

Details

Attachments

(1 file)

Bug 1472305 - browser.tabs.highlight() with single tab should not set multiselected=true
59 bytes, text/x-review-board-request
Gijs
: review+
mixedpuppy
: review+
Details 
It seems tabs only have multiselected===true when there are multiple ones selected. If only a single tab is selected, then multiselected is false.

But the highlight() API breaks this:

    // chrome
    gBrowser.selectedTab.multiselected === false

    // webext
    browser.tabs.highlight({tabs: 0})

    // chrome
    gBrowser.selectedTab.multiselected === true

https://searchfox.org/mozilla-central/rev/d2966246905102b36ef5221b0e3cbccf7ea15a86/browser/components/extensions/parent/ext-tabs.js#1262-1265

I don't like handling this kind of details in ext-tabs.js, maybe a new gBrowser method would be better.
I moved the multitab selection into tabbrowser.js, and made the pref check more consistent with tab hiding.

Abdoulaye, I don't really understand what updatePositionalAttributes and skipPositionalAttributes do, should I specify them?
Assignee: nobody → oriol-bugzilla
Status: NEW → ASSIGNED
Flags: needinfo?(ablayelyfondou)
(In reply to Oriol Brufau [:Oriol] from comment #2)
> Abdoulaye, I don't really understand what updatePositionalAttributes and
> skipPositionalAttributes do, should I specify them?

An attribute named "before-multiselected" is set on the tab located to the left of every multi-selected tab. 
https://searchfox.org/mozilla-central/source/browser/base/content/tabbrowser.xml#307

For exemple let consider theses tabs. 
1. Before mulitiselect operation: 
 A,B,C,D,E

2. After multiselect operation:
 A, B[multiselected], C[multiselected], D, E[multiselected]

3. After updatePositionalAttributes:
 A[before-multiselected], B[multiselected, before-multiselected], C[multiselected], D[before-multiselected], E[multiselected]

before-multiselected attribute helps us set the height of the tab line separator to 100% in css files.

Note that updatePositionalAttributes is a "heavy" process thought as it loops through all visible tabs and it also sets other attributes not related to multiselect (such as hovered or active tab). So when multiselecting a range of tabs (or multiple tabs) at once (e.g: via shift key), the updatePositionalAttributes is triggered only after all tabs are added to the multiselect collection.
 
You can avoid updating positional attributes with skipPositionalAttr = true (see https://searchfox.org/mozilla-central/source/browser/base/content/tabbrowser.js#3615-3617).
I think this can be useful in your "set selectedTabs" method as it adds multiple tabs "at once" to the multiselect collection.
Flags: needinfo?(ablayelyfondou)
OK, thanks for the explanation, I have set skipPositionalAttr to true
Comment on attachment 8989005 [details]
Bug 1472305 - browser.tabs.highlight() with single tab should not set multiselected=true

https://reviewboard.mozilla.org/r/254096/#review261112
Attachment #8989005 - Flags: review?(mixedpuppy) → review+
Comment on attachment 8989005 [details]
Bug 1472305 - browser.tabs.highlight() with single tab should not set multiselected=true

https://reviewboard.mozilla.org/r/254096/#review261130

r=me with the minor comments below addressed.

::: browser/base/content/test/tabs/browser_multiselect_tabs_using_selectedTabs.js:1
(Diff revision 3)
> +const PREF_MULTISELECT_TABS = "browser.tabs.multiselect";

Nit: public domain license header please.

::: browser/components/extensions/parent/ext-tabs.js:1242
(Diff revision 3)
>            }
>            return hidden;
>          },
>  
>          highlight(highlightInfo) {
> -          if (!Services.prefs.getBoolPref("browser.tabs.multiselect")) {
> +          if (!Services.prefs.getBoolPref(MULTISELECT_PREFNAME, false)) {

I know this precedes your patch, but please can you convert this to a cached pref with `XPCOMUtils.defineLazyPreferenceGetter`, at the top of this file, probably naming the variable something like `gMultiSelectEnabled` or something like that?
Attachment #8989005 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to :Gijs (he/him) from comment #8)
> I know this precedes your patch, but please can you convert this to a cached
> pref with `XPCOMUtils.defineLazyPreferenceGetter`, at the top of this file,
> probably naming the variable something like `gMultiSelectEnabled` or
> something like that?

But this would force people to restart the browser after enabling the pref in order to see the effects, wouldn't it?

Also, I changed this part to be more consistent with show() and hide(), and these don't cache extensions.webextensions.tabhide.enabled
(In reply to Oriol Brufau [:Oriol] from comment #9)
> (In reply to :Gijs (he/him) from comment #8)
> > I know this precedes your patch, but please can you convert this to a cached
> > pref with `XPCOMUtils.defineLazyPreferenceGetter`, at the top of this file,
> > probably naming the variable something like `gMultiSelectEnabled` or
> > something like that?
> 
> But this would force people to restart the browser after enabling the pref
> in order to see the effects, wouldn't it?

No, defineLazyPreferenceGetter observes the pref for changes with the pref service, so the variable is automatically updated when the pref changes. But it avoids asking the pref service for the value whenever the API is called.

> Also, I changed this part to be more consistent with show() and hide(), and
> these don't cache extensions.webextensions.tabhide.enabled

I think that should change too (ie that should also cache), but probably not in this bug.
Keywords: checkin-needed
Pushed by aciure@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8d97f2d05077
browser.tabs.highlight() with single tab should not set multiselected=true r=Gijs,mixedpuppy
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8d97f2d05077
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
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!
Flags: needinfo?(oriol-bugzilla)
Covered by automated test
Flags: needinfo?(oriol-bugzilla) → qe-verify-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: