1244622 - In SyncedTabs menu, introduce some facility for showing tabs that aren't shown by default

Status: RESOLVED FIXED

(Firefox :: Sync, defect, P1)

Description

[Mark Hammond [:markh] [:mhammond]]

+++ This bug was initially created as a clone of Bug #1237941 +++

In the above bug we increased the limit on the number of tabs shown to 50 as it was felt 15 offered too much scope for the expected tab not being there.

However, the downside is that if the first device(s) shown has many tabs but the client you are looking for is at the bottom of the list, you need to scroll lots to find the tab.

We should consider dropping that limit back to 15 (or 10?) and when more tabs are available show something like "xx tabs not shown - open in sidebar", and when clicked we open the sidebar and auto-select the specific device.

Comment 1

[Edwin Wong [:edwong]]

NI: rfeeley please add details on desired UX from our meeting today.

Comment 2

[Ryan Feeley [:rfeeley]]

Stephen didn’t like the plan we discussed to have an inline "More" option, but said he had something else in mind, so I'm hoping he can follow up here.

Comment 3

[Edwin Wong [:edwong]]

Triage: rfeeley says leaving at 50 is OK for now.  bumping down to P3

Comment 4

[Edwin Wong [:edwong]]

shorelander - do you have mocks or proposal for us?

Comment 6

[Alex Davis [:adavis]]

Added a feature card in product board:
https://trello.com/c/Zy1Ie9ks/51-reduce-number-of-of-visible-tabs-per-device-in-sync-d-tabs-drop-drown

Closing bug.

Comment 7

[Mark Hammond [:markh] [:mhammond]]

WONTFIX sends the wrong message. Also, I don't think this *is* a feature request - it is a bug report that "not all tabs are shown".

Comment 9

[Mark Hammond [:markh] [:mhammond]]

rfeeley, are you able to help us get traction here?

Comment 10

[Ryan Feeley [:rfeeley]]

Luckily the Photon team are in town. I met with them just now and discussed this. They think it's worthwhile to let users know when there is overflow. Their suggestion for devices with overflow to add an option at the bottom of the menu with the label: Show all
It would have no icon, and the text would be the same font size and be left aligned with the favicons above it. When the user clicks "Show all" the menu loads all of the tabs, no matter how many there are.
The next time they restart they Sync, or start their browser, the spillover tabs are gone.
Make sense?

Comment 11

[Mark Hammond [:markh] [:mhammond]]

Thanks Ryan!

I'd have thought *some* differentiation in the text would be useful (eg, italicized) but that's clearly your side of the fence, so happy to go with it :)

(I'm changing it from "enhancement" to "normal" to get it on our triage radar; I'm hoping we can get :eoger to take this at some point)

Comment 12

[Ryan Feeley [:rfeeley]]

And to be precise, it'll need a capital A like:

Show All

Comment 13

[Ryan Feeley [:rfeeley]]

Attachment: show-all.png

Comment 14

[Edouard Oger [:eoger]]

Attachment: Bug 1244622 - Add "Show More/All" (tabs) buttons in the Synced Tabs menu.

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

Comment 15

[Edouard Oger [:eoger]]

Showed the patch to Ryan who was happy with the result :)

Comment 16

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8834138 [details]
Bug 1244622 - Add "Show More/All" (tabs) buttons in the Synced Tabs menu.

https://reviewboard.mozilla.org/r/110180/#review111326

LGTM, thanks.

::: browser/locales/en-US/chrome/browser/browser.dtd:364
(Diff revision 1)
>       the name of a device when that device has no open tabs -->
>  <!ENTITY appMenuRemoteTabs.notabs.label "No open tabs">
> +<!-- LOCALIZATION NOTE (appMenuRemoteTabs.showAll.label, appMenuRemoteTabs.showAll.tooltip):
> +     This is shown after the tabs list if we can display more tabs by clicking on the button -->
> +<!ENTITY appMenuRemoteTabs.showAll.label "Show All">
> +<!ENTITY appMenuRemoteTabs.showAll.tooltip "Show All Tabs">

Can you check with Ryan that this tooltip is OK? I'd have thought something more descriptive is better - eg, maybe something like "Show %d more tabs from this device"

::: services/sync/modules/SyncedTabs.jsm:275
(Diff revision 1)
>    // recent sync wasn't "recently".
>    syncTabs(force) {
>      return this._internal.syncTabs(force);
>    },
>  
> -  sortTabClientsByLastUsed(clients, maxTabs = Infinity) {
> +  sortTabClientsByLastUsed(clients, maxTabs = Infinity, ignoreMaxTabsForClientId) {

It seems like this code is the only caller that specifies maxTabs, and there's no real efficiency benefit in doing it here - so I think it would keep the code a little cleaner if we just remove the maxTabs param here entirely and do the slicing in the caller, thus also removing the new |ignoreMaxTabsForClientId| param and the new hasMoreTabs attribute.

Comment 17

[Edouard Oger [:eoger]]

Comment on attachment 8834138 [details]
Bug 1244622 - Add "Show More/All" (tabs) buttons in the Synced Tabs menu.

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

Comment 18

[Edouard Oger [:eoger]]

Comment on attachment 8834138 [details]
Bug 1244622 - Add "Show More/All" (tabs) buttons in the Synced Tabs menu.

Could you re-check this again Mark? After talking with rfeeley this afternoon, we decided to introduce a mechanism to avoid only displaying less than 5 more tabs when clicking on "Show All" in order to avoid user disappointment :)
For example, when we have 51 tabs, we show 46 tabs instead of 50 and "Show all" now shows 5 more tabs.

Comment 19

[Mark Hammond [:markh] [:mhammond]]

(In reply to Edouard Oger [:eoger] from comment #18)
> For example, when we have 51 tabs, we show 46 tabs instead of 50 and "Show
> all" now shows 5 more tabs.

The general idea seems reasonable, but ISTM that in that example it would be better to just show 51 in the first place.

Comment 20

[Edouard Oger [:eoger]]

But in that case you're just pushing the problem, what about 52? :)

Comment 21

[Mark Hammond [:markh] [:mhammond]]

I don't really care about this, but ISTM that with the new limit of 20 in the patch, the best behaviour would be to say that under 25 shows 25, over 25 shows 20 with "show all" - IOW, "show all" is guaranteed to show at least 5, but the number shown never falls below that limit of 20. That seems better to me than meaning we might only show 16 before "show all" is enabled.

As I said, I don't really care - I'll review the patch anyway ;)

Comment 22

[Mark Hammond [:markh] [:mhammond]]

heh - I guess that's identical to using 25 instead of 20 (or 55 instead of 50) using your technique - so let's just ignore this bike-shedding :)

Comment 23

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8834138 [details]
Bug 1244622 - Add "Show More/All" (tabs) buttons in the Synced Tabs menu.

https://reviewboard.mozilla.org/r/110180/#review111780

LGTM, thanks!

::: browser/components/customizableui/CustomizableWidgets.jsm:491
(Diff revision 2)
>        } else {
> +        let displayShowAllTabs = !ignoreTabsLimit && client.tabs.length > this.MAX_TABS_PER_CLIENT;
> +        if (displayShowAllTabs) {
> +          // When the user clicks "Show All", try to have at least SHOW_ALL_TABS_MIN_TABS more tabs
> +          // to display in order to avoid user frustration
> +          let sliceEnd = ((client.tabs.length - this.SHOW_ALL_TABS_MIN_TABS) < this.MAX_TABS_PER_CLIENT) ?

I think this can be written as `let sliceEnd = min(client.tabs.length - this.SHOW_ALL_TABS_MIN_TABS, this.MAX_TABS_PER_CLIENT);`

Comment 24

[Edouard Oger [:eoger]]

Comment on attachment 8834138 [details]
Bug 1244622 - Add "Show More/All" (tabs) buttons in the Synced Tabs menu.

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

Comment 25

[Edouard Oger [:eoger]]

Implemented bug 1337508 (pagination when too many tabs shown), if you could give it a look Mark.

Comment 26

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8834138 [details]
Bug 1244622 - Add "Show More/All" (tabs) buttons in the Synced Tabs menu.

https://reviewboard.mozilla.org/r/110180/#review113554

::: browser/components/customizableui/CustomizableWidgets.jsm:491
(Diff revision 3)
>  
>        if (client.tabs.length == 0) {
>          let label = this._appendMessageLabel("notabsforclientlabel", attachFragment);
>          label.setAttribute("class", "PanelUI-remotetabs-notabsforclient-label");
>        } else {
> +        let displayShowMoreTabs = client.tabs.length > maxTabs;

I don't think this is quite right.

At a minimum, we need to clarify what Ryan means in bug 1337508 comment 0. I'm fairly sure that in that specific example (ie, 45 tabs), he expects *both* "Show More" and "Show All" when showing the first page. It's not quite clear to me what he expects when showing the second page (ie, when 5 tabs are hidden) - ie, whether he expects both in that case too. IMO we should still show both there too, as otherwise it might confuse the user as to when each button is visible, but he may have a different opinion.

* For both buttons (ie, regardless of whether both are shown or only 1), the "overflow" should have that logic applied. IOW, neither "Show All" nor "Show More" should ever show less than 5 tabs.

I also think the names of the constants should reflect this better - eg:

MAX_TABS_PER_CLIENT -> DEFAULT_TABS_PER_PAGE
SHOW_ALL_TABS_MIN_TABS -> NEXT_PAGE_MIN_TABS (or similar)

Comment 27

[Edouard Oger [:eoger]]

Attachment: screenshots.zip

Ryan, here's what I implemented at the moment, is this what you were expecting?

Comment 28

[Mark Hammond [:markh] [:mhammond]]

(In reply to Edouard Oger [:eoger] from comment #27)
> Ryan, here's what I implemented at the moment, is this what you were
> expecting?

To be clear, what I was expecting was:

1.png - has "show all" *and* "show more" buttons (currently only "show more"). - clicking "show all" is a one-click way of getting to 4.png, whereas "Show More" takes you to 2.png

2.png - ditto - both buttons.

3.png - not clear if it should have both buttons, or only 1 (as the next page is the last).

4.png - correct - all tabs shown so no buttons.

Comment 29

[Ryan Feeley [:rfeeley]]

Perfect! As we discussed. In practice this is like "Next page", "Next page", "Next page", "Last page" UI. Just enough UI for the few who will encounter it. I don't want us to get into a "Show next 25 of 350"-type situation.

Comment 30

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8834138 [details]
Bug 1244622 - Add "Show More/All" (tabs) buttons in the Synced Tabs menu.

https://reviewboard.mozilla.org/r/110180/#review114402

Apologies for the confusion - you can tell why I'm not in UX ;)

This looks good with the below comments, but I think we need tests - browser_967000_button_sync.js seems like the obvious candidate for that, or a new test file would also work (or maybe even renaming that test now that it is no longer just testing bug 967000)

::: browser/components/customizableui/CustomizableWidgets.jsm:312
(Diff revision 3)
>        DECKINDEX_TABS: 0,
>        DECKINDEX_TABSDISABLED: 1,
>        DECKINDEX_FETCHING: 2,
>        DECKINDEX_NOCLIENTS: 3,
>      },
> +    MAX_TABS_PER_CLIENT: 25,

I still think these names should be changed - how about:

MAX_TABS_PER_CLIENT -> TABS_PER_PAGE
SHOW_ALL_TABS_MIN_TABS -> NEXT_PAGE_MIN_TABS

?

::: browser/components/customizableui/CustomizableWidgets.jsm:491
(Diff revision 3)
>  
>        if (client.tabs.length == 0) {
>          let label = this._appendMessageLabel("notabsforclientlabel", attachFragment);
>          label.setAttribute("class", "PanelUI-remotetabs-notabsforclient-label");
>        } else {
> +        let displayShowMoreTabs = client.tabs.length > maxTabs;

I think we should have a brief comment here explaining the intent - something like "if this page will be showing all tabs for the client we show no additional buttons. If the *next* page after this will be showing all tabs, we display a "Show All" button, otherwise a "Show More" button".

Also, I think the variable names should be changed as they are slightly misleading - ie, just because displayShowMoreTabs is true doesn't mean we will actually see the "Show More" button. Names like "nextPageHasMoreTabs" and "nextPageHasAllTabs" seem clearer.

::: browser/components/customizableui/CustomizableWidgets.jsm:498
(Diff revision 3)
> +        if (displayShowAllTabs) {
> +          // When the user clicks "Show All", try to have at least SHOW_ALL_TABS_MIN_TABS more tabs
> +          // to display in order to avoid user frustration
> +          maxTabs = Math.min(client.tabs.length - this.SHOW_ALL_TABS_MIN_TABS, maxTabs);
> +        }
> +        if (displayShowMoreTabs/* || displayShowAllTabs */) {

The commented code here looks like it is accidentally left behind, whereas I suspect you were just trying to reinforce that if displayShowAllTabs is true displayShowMoreTabs must also be true. I think the names I suggested above would make this clearer, so the comment should be removed.

::: browser/components/customizableui/CustomizableWidgets.jsm:537
(Diff revision 3)
> +    _createShowMoreElement(doc, clientId, showCount) {
> +      let showAllItem = doc.createElementNS(kNSXUL, "toolbarbutton");
> +      showAllItem.setAttribute("itemtype", "showmorebutton");
> +      showAllItem.setAttribute("class", "subviewbutton");
> +      let prefix = showCount === Infinity ? "showAll" : "showMore";
> +      let label = this._tabsList.getAttribute(prefix + "Label");

This is a total nit, but I think it would be better to skip "prefix" and have an if/else statement that spells the Ids out completely, otherwise DXR etc will fail to help someone find where each of these strings is actually referenced.

Comment 31

[Edouard Oger [:eoger]]

Comment on attachment 8834138 [details]
Bug 1244622 - Add "Show More/All" (tabs) buttons in the Synced Tabs menu.

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

Comment 32

[Edouard Oger [:eoger]]

Thank you Mark, comments addressed.
The synced tabs menu tests were prone to race conditions and weird errors, so I took the opportunity to fix that, they're silky smooth now :)

Comment 33

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8834138 [details]
Bug 1244622 - Add "Show More/All" (tabs) buttons in the Synced Tabs menu.

https://reviewboard.mozilla.org/r/110180/#review114660

Thanks Ed!

Comment 34

[Edouard Oger [:eoger]]

Comment on attachment 8834138 [details]
Bug 1244622 - Add "Show More/All" (tabs) buttons in the Synced Tabs menu.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/110180/diff/4-5/

Comment 35

[Edouard Oger [:eoger]]

Comment on attachment 8834138 [details]
Bug 1244622 - Add "Show More/All" (tabs) buttons in the Synced Tabs menu.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/110180/diff/5-6/

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e3fbc6096825
Add "Show More/All" (tabs) buttons in the Synced Tabs menu. r=markh

Comment 37

[Carsten Book [:Tomcat]]

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