Closed Bug 1446101 Opened 6 years ago Closed 6 years ago

Convert all tabs popup to photon panel

Categories

(Firefox :: Tabbed Browser, task, P1)

Product:
Firefox
Component:
Tabbed Browser
Version:
61 Branch
Type:
task

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 62
Iteration:
62.3 - Jun 18
Tracking Flags:
Tracking Status
firefox61 - wontfix
firefox62 --- verified
firefox63 --- verified

People

(Reporter: mstriemer, Assigned: mstriemer)

References

(Depends on 3 open bugs, Blocks 2 open bugs, Regressed 3 open bugs)

Details

Attachments

(7 files)

bug 1446101 - Part 1: Extract a common TabsListBase class from TabsPopup
59 bytes, text/x-review-board-request
dao
: review+
Details
Bug 1446101 - Part 2: Convert the all tabs panel to photon styles
59 bytes, text/x-review-board-request
dao
: review+
Details
Bug 1446101 - Part 3: Make History > Hidden Tabs deep link to Hidden Tabs menu
59 bytes, text/x-review-board-request
mak
: review+
Details
Bug 1446101 - Part 4: Remove old alltabs code, rename alltabs-popup to newtab-popup
59 bytes, text/x-review-board-request
dao
: review+
Details
bug.jpg
413.79 KB, image/jpeg
Details
bug2
17.93 KB, image/jpeg
Details
Bug1446101.gif
278.86 KB, image/gif
Details 
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.
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
Nice to have, but not required for the new tab hiding api work. Tracking61-
tracking-firefox61: ? → -
Dropping to P3 since this isn't needed for flipping the tab hiding pref and will likely target 62 now.
Priority: P1 → P2
You mentioned "Dropping to P3" but you set it to P2. Typo?
Flags: needinfo?(mstriemer)
Yeah, this is in progress but no longer targeting 61, so I believe it should be P2.
Flags: needinfo?(mstriemer)
Severity: normal → enhancement
Iteration: --- → 62.3 - Jun 18
Blocks: 1465171
Blocks: 1465119
Blocks: 1466813
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+
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 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 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 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+
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
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+
Attached image bug.jpg — 
Is this ok?
On windows 10
Attached image bug2 — 
Sometimes the throbbers disappear except on the selected tab
Two scroll bars? Missing throbbers
Is this a known bug?
or intended?
Flags: needinfo?(dao+bmo)
browser.tabs.hideThrobber=TRUE is also not honored if set by user in the popup list
Depends on: 1470684
Depends on: 1470690
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)
(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
(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)
Depends on: 1470829
Depends on: 1470859
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.
(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)
(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
Blocks: 1470947
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)
Attached 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.
Status: RESOLVED → VERIFIED
status-firefox62: fixed → verified
status-firefox63: --- → verified
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: 1474728
Depends on: 1474352
Depends on: 1475268
(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.
Depends on: 1477594
Depends on: 1478708
Flags: needinfo?(mstriemer)
Depends on: 1482731
Blocks: 1483632
No longer blocks: 1483632
Depends on: 1483632
Depends on: 1489129
Depends on: 1489124
Depends on: 1511367
No longer depends on: 1511367
Blocks: 1516083
Blocks: 1188830
Blocks: 1188819
Type: enhancement → task
Regressions: 1584855
Regressions: 1470829
Regressions: 1600548
No longer regressions: 1600548
Regressions: 1600548
Blocks: tab-manager
Regressions: 1628171
No longer regressions: 1628171
No longer depends on: 1446114
Regressions: 1634237
No longer depends on: 1489124
Regressions: 1489124
Regressions: 1853486
Depends on: 1853486
No longer regressions: 1853486
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: