Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED in Firefox 54

Status

()

Firefox
Sync
P1
normal
RESOLVED FIXED
2 years ago
2 months ago

People

(Reporter: markh, Assigned: eoger)

Tracking

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

Firefox Tracking Flags

(firefox54 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Reporter)

Description

2 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

Comment 1

2 years ago
NI: rfeeley please add details on desired UX from our meeting today.
Flags: needinfo?(rfeeley)
(Reporter)

Updated

2 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

Updated

a year ago
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

a year ago
Duplicate of this bug: 1125109

Updated

11 months ago
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
Last Resolved: 11 months ago
Flags: needinfo?(adavis)
Resolution: --- → WONTFIX
(Reporter)

Comment 7

10 months 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

10 months ago
Duplicate of this bug: 1302603
(Reporter)

Comment 9

6 months 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

6 months 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
Created attachment 8834070 [details]
show-all.png
Comment hidden (mozreview-request)
(Assignee)

Comment 15

5 months ago
Showed the patch to Ryan who was happy with the result :)
(Reporter)

Updated

5 months ago
Priority: -- → P1
(Reporter)

Updated

5 months ago
Assignee: nobody → eoger
(Reporter)

Comment 16

5 months 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

5 months 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

5 months ago
See Also: → bug 1337508
(Reporter)

Comment 19

5 months 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

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

Comment 21

5 months 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

5 months 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

5 months 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

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

Comment 26

5 months 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

5 months ago
Created attachment 8837271 [details]
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

5 months 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

5 months 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

5 months 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

5 months 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

5 months 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
https://hg.mozilla.org/mozilla-central/rev/e3fbc6096825
Status: REOPENED → RESOLVED
Last Resolved: 11 months ago5 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
(Assignee)

Updated

5 months ago
Duplicate of this bug: 1337508

Updated

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