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

RESOLVED FIXED in Firefox 54

Status

()

defect
P1
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: markh, Assigned: eoger)

Tracking

Trunk
Firefox 54
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(3 attachments)

Reporter

Description

3 years ago
+++ 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.
Flags: firefox-backlog+
Blocks: 1247768
NI: rfeeley please add details on desired UX from our meeting today.
Flags: needinfo?(rfeeley)
Reporter

Updated

3 years ago
Summary: In SyncedTabs menu, decrease limit on number of tabs shown and add an overflow UI to open the sidebar → In SyncedTabs menu, introduce some facility for showing tabs that aren't shown by default
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.
Flags: needinfo?(rfeeley) → needinfo?(shorlander)
Assignee: nobody → shorlander
Status: NEW → ASSIGNED
Triage: rfeeley says leaving at 50 is OK for now.  bumping down to P3
Priority: P2 → P3
Assignee: shorlander → nobody
Status: ASSIGNED → NEW
shorelander - do you have mocks or proposal for us?
Assignee: nobody → shorlander
Status: NEW → ASSIGNED
Severity: normal → enhancement
Priority: P3 → --
Reporter

Updated

3 years ago
Duplicate of this bug: 1125109
Flags: needinfo?(shorlander) → needinfo?(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.
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Flags: needinfo?(adavis)
Resolution: --- → WONTFIX
Reporter

Comment 7

3 years ago
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".
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Reporter

Updated

3 years ago
Duplicate of this bug: 1302603
Reporter

Comment 9

2 years ago
rfeeley, are you able to help us get traction here?
Flags: needinfo?(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?
Flags: needinfo?(rfeeley) → needinfo?(markh)
Reporter

Comment 11

2 years ago
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)
Assignee: shorlander → nobody
Severity: enhancement → normal
Flags: needinfo?(markh)
And to be precise, it'll need a capital A like:

Show All
Posted image show-all.png
Comment hidden (mozreview-request)
Assignee

Comment 15

2 years ago
Showed the patch to Ryan who was happy with the result :)
Reporter

Updated

2 years ago
Priority: -- → P1
Reporter

Updated

2 years ago
Assignee: nobody → eoger
Reporter

Comment 16

2 years ago
mozreview-review
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.
Attachment #8834138 - Flags: review?(markh) → review+
Comment hidden (mozreview-request)
Assignee

Comment 18

2 years ago
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.
Attachment #8834138 - Flags: review+ → review?(markh)
Assignee

Updated

2 years ago
See Also: → 1337508
Reporter

Comment 19

2 years ago
(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.
Assignee

Comment 20

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

Comment 21

2 years ago
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 ;)
Reporter

Comment 22

2 years ago
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 :)
Reporter

Comment 23

2 years ago
mozreview-review
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);`
Attachment #8834138 - Flags: review?(markh) → review+
Comment hidden (mozreview-request)
Assignee

Comment 25

2 years ago
Implemented bug 1337508 (pagination when too many tabs shown), if you could give it a look Mark.
Flags: needinfo?(markh)
Reporter

Comment 26

2 years ago
mozreview-review
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)
Attachment #8834138 - Flags: review+
Assignee

Comment 27

2 years ago
Posted file screenshots.zip
Ryan, here's what I implemented at the moment, is this what you were expecting?
Attachment #8837271 - Flags: feedback?(rfeeley)
Reporter

Comment 28

2 years ago
(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.
Flags: needinfo?(markh)
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.
Reporter

Comment 30

2 years ago
mozreview-review
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 hidden (mozreview-request)
Assignee

Comment 32

2 years ago
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 :)
Reporter

Comment 33

2 years ago
mozreview-review
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!
Attachment #8834138 - Flags: review?(markh) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 36

2 years ago
Pushed by eoger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e3fbc6096825
Add "Show More/All" (tabs) buttons in the Synced Tabs menu. r=markh

Comment 37

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e3fbc6096825
Status: REOPENED → RESOLVED
Closed: 3 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Assignee

Updated

2 years ago
Duplicate of this bug: 1337508
Attachment #8837271 - Flags: feedback?(rfeeley) → feedback+
You need to log in before you can comment on or make changes to this bug.