Convert all tabs popup to photon panel

VERIFIED FIXED in Firefox 62

Status

()

P1
enhancement
VERIFIED FIXED
a year ago
2 months ago

People

(Reporter: mstriemer, Assigned: mstriemer)

Tracking

(Depends on: 5 bugs, Blocks: 2 bugs)

61 Branch
Firefox 62
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61- wontfix, firefox62 verified, firefox63 verified)

Details

Attachments

(7 attachments)

(Assignee)

Description

a year ago
The all tabs dropdown (the down arrow on overflow/hidden tabs) is currently a native popup. This should be updated to use the photon style panel.
(Assignee)

Updated

a year ago
Depends on: 1446114
Here is a direct link to this menu UI:
https://mozilla.invisionapp.com/share/KXEUSPHHN#/screens/266892148

And a link to an extended version of it containing a scrollbar and undo:
https://mozilla.invisionapp.com/share/KXEUSPHHN#/screens/279576474
Mark, you're actively working on this right?
Priority: -- → P1
(Assignee)

Updated

a year ago
status-firefox61: --- → fix-optional
tracking-firefox61: --- → ?
Nice to have, but not required for the new tab hiding api work. Tracking61-
tracking-firefox61: ? → -
(Assignee)

Comment 5

11 months ago
Dropping to P3 since this isn't needed for flipping the tab hiding pref and will likely target 62 now.
Priority: P1 → P2

Comment 6

11 months ago
You mentioned "Dropping to P3" but you set it to P2. Typo?
Flags: needinfo?(mstriemer)
(Assignee)

Comment 7

11 months ago
Yeah, this is in progress but no longer targeting 61, so I believe it should be P2.
Flags: needinfo?(mstriemer)
(Assignee)

Updated

11 months ago
Severity: normal → enhancement
Iteration: --- → 62.3 - Jun 18
(Assignee)

Updated

10 months ago
Blocks: 1465171
(Assignee)

Updated

10 months ago
Blocks: 1465119
(Assignee)

Updated

10 months ago
Blocks: 1466813
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 15

10 months ago
mozreview-review
Comment on attachment 8984351 [details]
Bug 1446101 - Part 3: Make History > Hidden Tabs deep link to Hidden Tabs menu

https://reviewboard.mozilla.org/r/250166/#review256558
Attachment #8984351 - Flags: review?(mak77) → review+
status-firefox61: fix-optional → wontfix
status-firefox62: --- → affected
(Assignee)

Comment 16

9 months ago
Would be nice to get this into 62, there were some minor regressions with the history changes that this should fix.
Component: WebExtensions: Frontend → Tabbed Browser
Priority: P2 → P1
Product: Toolkit → Firefox
Target Milestone: --- → Firefox 62
Version: unspecified → 61 Branch

Comment 17

9 months ago
mozreview-review
Comment on attachment 8984349 [details]
bug 1446101 - Part 1: Extract a common TabsListBase class from TabsPopup

https://reviewboard.mozilla.org/r/250162/#review258740
Attachment #8984349 - Flags: review?(dao+bmo) → review+

Comment 18

9 months ago
mozreview-review
Comment on attachment 8984350 [details]
Bug 1446101 - Part 2: Convert the all tabs panel to photon styles

https://reviewboard.mozilla.org/r/250164/#review258738

::: browser/base/content/browser.js:89
(Diff revision 2)
>  XPCOMUtils.defineLazyScriptGetter(this, "PanelUI",
>                                    "chrome://browser/content/customizableui/panelUI.js");
>  XPCOMUtils.defineLazyScriptGetter(this, "gViewSourceUtils",
>                                    "chrome://global/content/viewSourceUtils.js");
> +XPCOMUtils.defineLazyScriptGetter(this, "gTabsPanel",
> +                                  "chrome://browser/content/browser-all-tabs.js");

Can we rename this to browser-allTabsMenu.js?

::: browser/base/content/browser.xul:518
(Diff revision 2)
>  #include popup-notifications.inc
>  
>  #include ../../components/customizableui/content/panelUI.inc.xul
>  #include ../../components/controlcenter/content/panel.inc.xul
>  #include ../../components/downloads/content/downloadsPanel.inc.xul
> +#include browser-all-tabs.inc.xul

And browser-allTabsMenu.inc.xul?

::: browser/base/content/browser.xul:670
(Diff revision 2)
>                       cui-areatype="toolbar"
>                       removable="true"/>
>  
>        <toolbarbutton id="alltabs-button"
>                       class="toolbarbutton-1 chromeclass-toolbar-additional tabs-alltabs-button badged-button"
> -                     type="menu"
> +                     oncommand="gTabsPanel.showAllTabsPanel()"

nit: add a semicolon :)
Attachment #8984350 - Flags: review?(dao+bmo) → review+

Comment 19

9 months ago
mozreview-review
Comment on attachment 8984352 [details]
Bug 1446101 - Part 4: Remove old alltabs code, rename alltabs-popup to newtab-popup

https://reviewboard.mozilla.org/r/250168/#review258742
Attachment #8984352 - Flags: review?(dao+bmo) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 24

9 months ago
Pushed by mstriemer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/399aa4954743
Part 1: Extract a common TabsListBase class from TabsPopup r=dao
https://hg.mozilla.org/integration/autoland/rev/b5c365e6f133
Part 2: Convert the all tabs panel to photon styles r=dao
https://hg.mozilla.org/integration/autoland/rev/acbf751ec508
Part 3: Make History > Hidden Tabs deep link to Hidden Tabs menu r=mak
https://hg.mozilla.org/integration/autoland/rev/0d4eda5546dd
Part 4: Remove old alltabs code, rename alltabs-popup to newtab-popup r=dao
(Assignee)

Comment 25

9 months ago
Please verify that there are no regressions with the updates to the all tabs panel with this change. Some things to note:

* The loading indicator is now the "cylon" version that is used in the tab strip.
* There is an audio playing indicator in the menu which can be used to mute/unmute a tab.
* Hidden tabs are also included, if a hidden tab is playing audio it should be pulled into the first menu.
* The Hidden Tabs option under History has changed to a button that opens the Hidden Tabs submenu in the all tabs menu.
* The long-press menu on the new tab button to open a container tab has intentionally not been updated, but should still work as usual.

You can hide tabs using the Hide Tabs extension [1]. Note that it has a bug when you hide the current tab (in the extension, not Firefox).

[1] https://addons.mozilla.org/en-US/firefox/addon/hide-tab/
Flags: qe-verify+
Blocks: 1470661

Comment 27

9 months ago
Posted image bug.jpg
Is this ok?
On windows 10

Comment 28

9 months ago
Posted image bug2
Sometimes the throbbers disappear except on the selected tab

Comment 29

9 months ago
Two scroll bars? Missing throbbers
Is this a known bug?
or intended?
Flags: needinfo?(dao+bmo)

Comment 30

9 months ago
browser.tabs.hideThrobber=TRUE is also not honored if set by user in the popup list

Updated

9 months ago
Depends on: 1470684

Updated

9 months ago
Depends on: 1470690

Comment 31

9 months ago
If Site icons/Fav icons are disabled, there is an empty space left before the tab name, earlier that space was used to show the tab title in more detail without any empty space.
(In reply to Mark Galoway from comment #29)
> Two scroll bars?

It's not two scrollbars. The thinner bar indicates tabs that are currently visible vs. those that are scrolled off-screen in the tab bar.

> Missing throbbers
> Is this a known bug?

Not that I know.
Flags: needinfo?(dao+bmo)

Comment 33

9 months ago
(In reply to Dão Gottwald [::dao] from comment #32)
> (In reply to Mark Galoway from comment #29)
> > Two scroll bars?
> 
> It's not two scrollbars. The thinner bar indicates tabs that are currently
> visible vs. those that are scrolled off-screen in the tab bar.


makes sense now

> > Missing throbbers
> > Is this a known bug?
> 
> Not that I know.

browser.tabs.hideThrobber=TRUE is also not honored if set by user in the popup list

Comment 34

9 months ago
(In reply to Dão Gottwald [::dao] from comment #32)
> (In reply to Mark Galoway from comment #29)
> > Two scroll bars?
> 
> It's not two scrollbars. The thinner bar indicates tabs that are currently
> visible vs. those that are scrolled off-screen in the tab bar.


There is a break in the second thin scrollbar now
Flags: needinfo?(dao+bmo)

Updated

9 months ago
Depends on: 1470829

Updated

9 months ago
Depends on: 1470859

Updated

9 months ago
Depends on: 1470865
Mark, please do not show the indicator for what part of the list is visible. 
We found that this is causing more confusion than it helps prevent. 
See comment #29 for an example of that:
(In reply to Mark Galoway from comment #29)
> Two scroll bars?

Also, please remove the white-space before and after the scroll-bar, 
that separates it from the end of the list, as well as from the separator-line before.

Comment 36

9 months ago
(In reply to Markus Jaritz [:designakt] (UX) from comment #35)
> Mark, please do not show the indicator for what part of the list is visible. 
> We found that this is causing more confusion than it helps prevent. 
> See comment #29 for an example of that:
> (In reply to Mark Galoway from comment #29)
> > Two scroll bars?


Sorry but disagree, second-scroll bar is part of the UX that make Fx a breeze to use please *don't* remove it !!

Just make the new scrollbar *thinner* and *sleeker* as it is too wide IMO and bit darker in color

and add the scroll arrows at top and bottom tabs
which were removed by this patch as it provides an alternate scrolling method.

> Also, please remove the white-space before and after the scroll-bar, 
> that separates it from the end of the list, as well as from the
> separator-line before.
Flags: needinfo?(mjaritz)

Comment 37

9 months ago
(In reply to Markus Jaritz [:designakt] (UX) from comment #35)

> Also, please remove the white-space before and after the scroll-bar, 
> that separates it from the end of the list, as well as from the
> separator-line before.

Could not edit the above comment

Please don't remove it,better make the new scrollbar behave like overlay ones

http://linuxbsdos.com/wp-content/uploads/2013/10/TweakUbuntu5-600x404.png
(Assignee)

Updated

9 months ago
Blocks: 1470947
(Assignee)

Updated

9 months ago
No longer blocks: 1470947
Depends on: 1470947
Flags: needinfo?(dao+bmo)
(In reply to Mark Striemer [:mstriemer] (on PTO through 06/29) from comment #25)
> Please verify that there are no regressions with the updates to the all tabs
> panel with this change. Some things to note:
> 
> * The loading indicator is now the "cylon" version that is used in the tab strip.
I cannot confirm whether this is the correct loading icon, but I can say that the loading icon has changed from the release version to the latest nightly version.

> * There is an audio playing indicator in the menu which can be used to mute/unmute a tab.
This is correct.

> * Hidden tabs are also included, if a hidden tab is playing audio it should be pulled into the first menu.
Hidden tabs section is correctly displayed and the hidden tabs that have playing audio are being pulled into the first menu.

> * The Hidden Tabs option under History has changed to a button that opens the Hidden Tabs submenu in the all tabs menu.
I can't find a "Hidden Tabs option" under History menu or window. Where should it be displayed?

> * The long-press menu on the new tab button to open a container tab has intentionally not been updated, but should still work as usual.
This feature works correctly.

> You can hide tabs using the Hide Tabs extension [1]. Note that it has a bug
> when you hide the current tab (in the extension, not Firefox).
> 
> [1] https://addons.mozilla.org/en-US/firefox/addon/hide-tab/


Mark Striemer, we have to clarify the "The Hidden Tabs option under History" item, that I could not find.
Other than this, after a first session of testing, the main functionality of the feature/modifications made is working correctly.
Flags: needinfo?(mstriemer)

Comment 39

9 months ago
Posted image Bug1446101.gif
I can reproduce this issue on Firefox 61.0 (20180621125625) under Win 7 64-bit and Mac OS X 10.13.3

This issue is verified as fixed on Firefox 63.0a1 (20180627100027) and Firefox 62.0b3 (     20180625141512) under Win 7 64-bit and Mac OS X 10.13.3.

Please see the attached video.

Updated

9 months ago
Status: RESOLVED → VERIFIED
status-firefox62: fixed → verified
status-firefox63: --- → verified

Updated

9 months ago
Flags: qe-verify+
(In reply to Firefox_Ninja from comment #36)
> (In reply to Markus Jaritz [:designakt] (UX) from comment #35)
> > Mark, please do not show the indicator for what part of the list is visible. 
> > We found that this is causing more confusion than it helps prevent. 
> > See comment #29 for an example of that:
> > (In reply to Mark Galoway from comment #29)
> > > Two scroll bars?
> 
> Sorry but disagree, second-scroll bar is part of the UX that make Fx a
> breeze to use please *don't* remove it !!
> 
> Just make the new scrollbar *thinner* and *sleeker* as it is too wide IMO
> and bit darker in color

Please help me understand what makes that indicator helpful to you? 
When are you looking at it, for what information?
The new implementation has an indicator for what tab is selected/active, to allow people to relate the list to the row of tabs. 
(I was expecting that indicator to sever nearly the same purpose than the "visible tabs indicator" or "second scrollbar" as you called it. - where it is not working like a scroll bar in any way, but just looking like one.)


> and add the scroll arrows at top and bottom tabs
> which were removed by this patch as it provides an alternate scrolling
> method.

It sounds like this addresses a different UI than the "visible tabs indicator"
Thanks for mentioning that aspect. I did miss that function when proposing the new UI.
The automatic scrolling once the mouse reaches the end of the screen seams helpful.
Can you please file a bug for adding that functionality back. 
(I am not sure if that behavior is possible with the new implementation, Mark might know)
Flags: needinfo?(mjaritz)
Blocks: 1461735
Depends on: 1473974
Depends on: 1474728
Depends on: 1474352

Updated

8 months ago
Depends on: 1475268

Comment 41

8 months ago
(In reply to Markus Jaritz [:designakt] (UX) from comment #40)
> (In reply to Firefox_Ninja from comment #36)
> > (In reply to Markus Jaritz [:designakt] (UX) from comment #35)
> > > Mark, please do not show the indicator for what part of the list is visible. 
> > > We found that this is causing more confusion than it helps prevent. 
> > > See comment #29 for an example of that:
> > > (In reply to Mark Galoway from comment #29)
> > > > Two scroll bars?
> > 
> > Sorry but disagree, second-scroll bar is part of the UX that make Fx a
> > breeze to use please *don't* remove it !!
> > 
> > Just make the new scrollbar *thinner* and *sleeker* as it is too wide IMO
> > and bit darker in color
> 
> Please help me understand what makes that indicator helpful to you? 
> When are you looking at it, for what information?
> The new implementation has an indicator for what tab is selected/active, to
> allow people to relate the list to the row of tabs. 
> (I was expecting that indicator to sever nearly the same purpose than the
> "visible tabs indicator" or "second scrollbar" as you called it. - where it
> is not working like a scroll bar in any way, but just looking like one.)
> 
> 
> > and add the scroll arrows at top and bottom tabs
> > which were removed by this patch as it provides an alternate scrolling
> > method.
> 
> It sounds like this addresses a different UI than the "visible tabs
> indicator"
> Thanks for mentioning that aspect. I did miss that function when proposing
> the new UI.
> The automatic scrolling once the mouse reaches the end of the screen seams
> helpful.
> Can you please file a bug for adding that functionality back. 
> (I am not sure if that behavior is possible with the new implementation,
> Mark might know)

Mark can these two be implemented back please, super handy.

Updated

8 months ago
Depends on: 1477594
Depends on: 1478708
Flags: needinfo?(mstriemer)

Updated

7 months ago
Depends on: 1482731

Updated

7 months ago
Blocks: 1483632
No longer blocks: 1483632
Depends on: 1483632
Depends on: 1489129
Depends on: 1489124

Updated

4 months ago
Depends on: 1511367
No longer depends on: 1511367
Blocks: 1516083

Updated

2 months ago
Blocks: 1188830

Updated

2 months ago
Blocks: 1188819
You need to log in before you can comment on or make changes to this bug.