1395674 - [Photon] Inconsistent appearance of sliding subviews in the overflow menu

Status: VERIFIED FIXED

(Firefox :: Theme, defect, P1)

Description

[Alice0775 White]

[Tracking Requested - why for this release]: More polish is needed before ship it

Photon theme is bullshit at all.
User will be confused.


Reproducible: 100%

Steps To Reproduce:
1. Place several toolbutton into overflowed panel via cutomize
2. Look appearance


Actual Results:
Some item has a sliding submenu, however, no > mark at the right side of it.
Some item is checkbox type, Background is dark, and not check mark at left side of it
Some item is checkbox type, But no Background color.

Comment 1

[Alice0775 White]

Attachment: screenshot

Comment 2

[Julien Cristau [:jcristau] (back April 22)]

P4 bug, not tracking for 57.

Comment 3

[Sam Foster [:sfoster] (he/him)]

I'll have a crack at this. Seems like we're adding the subview handlers at http://searchfox.org/mozilla-central/source/browser/components/customizableui/CustomizableUI.jsm#1479, and somewhere around there we'll also need to add a .subviewbutton-nav class. Need to also add this if we add the item to the overflow button at runtime I assume. 

And have similar rules to insert the arrow as .PanelUI-subView .subviewbutton-nav::after around http://searchfox.org/mozilla-central/source/browser/themes/shared/customizableui/panelUI.inc.css#1291

And I assume we'll probably need to remove the class again when removed, and/ ensure it doesnt trip wierdness in the customize palette. 

Does that seem like a reasonable starting point, Gijs? Sorry I'm not sure how much time I'll save you here, as I'm not that familiar yet with the customizable stuff. But think of it as an investment :)

Comment 4

[:Gijs (he/him)]

(In reply to Sam Foster [:sfoster] from comment #3)
> I'll have a crack at this. Seems like we're adding the subview handlers at
> http://searchfox.org/mozilla-central/source/browser/components/
> customizableui/CustomizableUI.jsm#1479, and somewhere around there we'll
> also need to add a .subviewbutton-nav class. Need to also add this if we add
> the item to the overflow button at runtime I assume. 
> 
> And have similar rules to insert the arrow as .PanelUI-subView
> .subviewbutton-nav::after around
> http://searchfox.org/mozilla-central/source/browser/themes/shared/
> customizableui/panelUI.inc.css#1291
> 
> And I assume we'll probably need to remove the class again when removed,
> and/ ensure it doesnt trip wierdness in the customize palette. 

So, the class would need to be on the widget node (ie toolbarbutton, `node` in the CUI.jsm method you linked), rather than the `viewNode` (which is the container being used as a subview). I think we should just put it there permanently - I'm fairly sure we don't actually create the ::after element (or do anything else based on that class) unless the node is in a PanelUI-subview anyway, so we should be good.

> Does that seem like a reasonable starting point, Gijs? Sorry I'm not sure
> how much time I'll save you here, as I'm not that familiar yet with the
> customizable stuff. But think of it as an investment :)

No worries, thanks for picking this up. I think that's a fine start for the '>' indicators.

I'm not sure what to do about the checked item (sidebars). I've looked at that before now. We don't currently make space for a checkbox in this menu, in addition to icons... I guess we should? Not sure how tricky that is. That would be more upliftable than making the label say 'view sidebar' and 'hide sidebar', which would require string changes.

If you could fix those 2 things (ie the subview indicators and the 'checked' state) in 2 separate patches that would help. :-)

Comment 5

[Sam Foster [:sfoster] (he/him)]

Filed bug 1403388 for the checkbox type buttons. Adjusting summary here to reflect the menu-type focus of this bug.

Comment 6

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1395674 - Adding subviewbutton-nav class to menu-type toolbarbuttons and display the '>' in the overflow list.

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

Comment 7

[Sam Foster [:sfoster] (he/him)]

The patch in  attachment 8912444 [details] my first pass at this. I'm not sure if I've identified all the items which could show up in the customization palette and be added to the overflow list. I've gone ahead and added the .subviewbutton-nav class to a couple with type: checkbox. Seems like there's no need to do this programmatically if we know ahead of time they will need it and can just add it in the markup? 

After consulting with UX (aaron) I left the '>' in the customize mode, but gave them a disabled look with 50% opacity.

Comment 8

[:Gijs (he/him)]

Comment on attachment 8912444 [details]
Bug 1395674 - Adding subviewbutton-nav class to menu-type toolbarbuttons and display the '>' in the overflow list.

https://reviewboard.mozilla.org/r/183770/#review189152

r=me with the following issues corrected, but feel free to ask for re-review if you end up needing to make changes that you think warrant it.

::: browser/base/content/browser.xul:664
(Diff revision 1)
>                       ondragexit="newTabButtonObserver.onDragExit(event)"
>                       cui-areatype="toolbar"
>                       removable="true"/>
>  
>        <toolbarbutton id="alltabs-button"
> -                     class="toolbarbutton-1 chromeclass-toolbar-additional tabs-alltabs-button"
> +                     class="toolbarbutton-1 chromeclass-toolbar-additional tabs-alltabs-button subviewbutton-nav"

This button can't be moved outside the tabstrip, so it's never shown in the overflow panel, so it doesn't need this class.

::: browser/base/content/browser.xul:931
(Diff revision 1)
>                         overflows="false"
>                         cui-areatype="toolbar"
>                         hidden="true"
>                         tooltip="dynamic-shortcut-tooltip"/>
>  
>          <toolbarbutton id="library-button" class="toolbarbutton-1 chromeclass-toolbar-additional"

This button also needs the class.

::: browser/base/content/browser.xul:1020
(Diff revision 1)
>              <scrollbox orient="horizontal"
>                         id="PlacesToolbarItems"
>                         flex="1"/>
>              <toolbarbutton type="menu"
>                             id="PlacesChevron"
> -                           class="toolbarbutton-1"
> +                           class="toolbarbutton-1 subviewbutton-nav"

I don't think this one is ever shown in the overflow panel, we only show the placeholder button thingy, ie https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.xul#996-1000 , which doesn't open a subview so we can leave it.

::: browser/components/customizableui/CustomizableUI.jsm:1481
(Diff revision 1)
>  
>          if (viewNode) {
>            // PanelUI relies on the .PanelUI-subView class to be able to show only
>            // one sub-view at a time.
>            viewNode.classList.add("PanelUI-subView");
> +          nodeClasses.push("subviewbutton-nav");

So... the only issue I have with this is that it will show up for webextensions, independent of whether those webextensions have a subview or not, because webextensions just mark all their widgets as type=view, and add-ons can add/remove popups for them at runtime. So we can't know, in this place, whether the item has a subview or not. :-(

Can you add a check for `widget.source` and only add the class for the `SOURCE_BUILTIN` widgets, and file a followup to make webextensions deal with this case correctly? That'll need a webextension fix.

::: browser/themes/shared/customizableui/panelUI.inc.css:1712
(Diff revision 1)
>  
>  toolbaritem[overflowedItem=true],
>  .widget-overflow-list .toolbarbutton-1 {
>    width: 100%;
>    max-width: @wideMenuPanelWidth@;
> +  -moz-box-flex: 1;

I'm... pretty nervous about this. Why do we need the flex in addition to the `width: 100%` that we already have?

Comment 9

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8912444 [details]
Bug 1395674 - Adding subviewbutton-nav class to menu-type toolbarbuttons and display the '>' in the overflow list.

https://reviewboard.mozilla.org/r/183770/#review189152

> So... the only issue I have with this is that it will show up for webextensions, independent of whether those webextensions have a subview or not, because webextensions just mark all their widgets as type=view, and add-ons can add/remove popups for them at runtime. So we can't know, in this place, whether the item has a subview or not. :-(
> 
> Can you add a check for `widget.source` and only add the class for the `SOURCE_BUILTIN` widgets, and file a followup to make webextensions deal with this case correctly? That'll need a webextension fix.

Understood on the guard around widget.source. I'm not sure I understand how to word the bug that needs filing here though?

> I'm... pretty nervous about this. Why do we need the flex in addition to the `width: 100%` that we already have?

The toolbarbutton is inside a hbox here, and the width:100% has no effect. Its a little xul flexbox voodoo for me TBH. I was also a bit nervous about this as I'm not confident I understand all the different kinds of content that this could match. But, I did test with compound controls like the zoom and edit controls and all seems well. Maybe there's a better way to accomplish the same result?

Comment 10

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8912444 [details]
Bug 1395674 - Adding subviewbutton-nav class to menu-type toolbarbuttons and display the '>' in the overflow list.

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

Comment 11

[:Gijs (he/him)]

(In reply to Sam Foster [:sfoster] from comment #9)
> > I'm... pretty nervous about this. Why do we need the flex in addition to the `width: 100%` that we already have?
> 
> The toolbarbutton is inside a hbox here, and the width:100% has no effect.
> Its a little xul flexbox voodoo for me TBH. I was also a bit nervous about
> this as I'm not confident I understand all the different kinds of content
> that this could match. But, I did test with compound controls like the zoom
> and edit controls and all seems well. Maybe there's a better way to
> accomplish the same result?

Which toolbarbutton is inside an <hbox> ? The .widget-overflow-list containers are <vbox>s, right?

Comment 12

[:Gijs (he/him)]

(In reply to Sam Foster [:sfoster] from comment #9)
> > So... the only issue I have with this is that it will show up for webextensions, independent of whether those webextensions have a subview or not, because webextensions just mark all their widgets as type=view, and add-ons can add/remove popups for them at runtime. So we can't know, in this place, whether the item has a subview or not. :-(
> > 
> > Can you add a check for `widget.source` and only add the class for the `SOURCE_BUILTIN` widgets, and file a followup to make webextensions deal with this case correctly? That'll need a webextension fix.
> 
> Understood on the guard around widget.source. I'm not sure I understand how
> to word the bug that needs filing here though?

"WebExtension browserActions in the overflow panel need to toggle the subviewbutton-nav class when the panel is shown based on whether they have a popup" or something. The WebExtension team can change the summary further if appropriate. :-)

Comment 13

[Sam Foster [:sfoster] (he/him)]

(In reply to :Gijs from comment #11)
> 
> Which toolbarbutton is inside an <hbox> ? The .widget-overflow-list
> containers are <vbox>s, right?

It goes e.g.

 vbox#widget-overflow-fixed-list > toolbarpaletteitem > hbox.toolbarpaletteitem-box > toolbarbutton#history-panelmenu

Comment 14

[Sam Foster [:sfoster] (he/him)]

> > I'm not sure I understand how
> > to word the bug that needs filing here though?

I filed bug 1403666 for this. 
Ni for Comment #13

Comment 15

[:Gijs (he/him)]

(In reply to Sam Foster [:sfoster] from comment #13)
> (In reply to :Gijs from comment #11)
> > 
> > Which toolbarbutton is inside an <hbox> ? The .widget-overflow-list
> > containers are <vbox>s, right?
> 
> It goes e.g.
> 
>  vbox#widget-overflow-fixed-list > toolbarpaletteitem >
> hbox.toolbarpaletteitem-box > toolbarbutton#history-panelmenu

Ahhh. OK. But then we only need this if the item is the direct child of the toolbarpaletteitem-box in the panel. So maybe add a rule like:

.toolbarpaletteitem-box[place=panel] > .toolbarbutton-1 {
  -moz-box-flex: 1;
}

You can also use this technique to simplify the rule for .widget-overflow-list > toolbarpaletteitem .subviewbutton-nav::after that you're adding a little, ie:

.toolbarpaletteitem-box[place=panel] .subviewbutton-nav::after

I think should also work.

Comment 16

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8912444 [details]
Bug 1395674 - Adding subviewbutton-nav class to menu-type toolbarbuttons and display the '>' in the overflow list.

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

Comment 17

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8912444 [details]
Bug 1395674 - Adding subviewbutton-nav class to menu-type toolbarbuttons and display the '>' in the overflow list.

https://reviewboard.mozilla.org/r/183770/#review189152

> The toolbarbutton is inside a hbox here, and the width:100% has no effect. Its a little xul flexbox voodoo for me TBH. I was also a bit nervous about this as I'm not confident I understand all the different kinds of content that this could match. But, I did test with compound controls like the zoom and edit controls and all seems well. Maybe there's a better way to accomplish the same result?

As discussed in IRC, the hbox is anon. content and hbox.toolbarpaletteitem-box > .toolbarbutton-1 doesnt match here (it is toolbarpaletteitem > .toolbarbutton-1) so have adjusted accordingly.

Comment 18

[:Gijs (he/him)]

Comment on attachment 8912444 [details]
Bug 1395674 - Adding subviewbutton-nav class to menu-type toolbarbuttons and display the '>' in the overflow list.

https://reviewboard.mozilla.org/r/183770/#review189320

Haven't tested, but the code looks great, thanks!

::: browser/components/customizableui/CustomizableUI.jsm:1481
(Diff revision 3)
>  
>          if (viewNode) {
>            // PanelUI relies on the .PanelUI-subView class to be able to show only
>            // one sub-view at a time.
>            viewNode.classList.add("PanelUI-subView");
> +          if (aWidget.source === CustomizableUI.SOURCE_BUILTIN) {

Nitty nit: 2 * equals is prevailing style in pretty much all browser/toolkit code.

::: browser/themes/shared/customizableui/panelUI.inc.css:1721
(Diff revision 3)
>  .widget-overflow-list .toolbarbutton-1 {
>    -moz-box-align: center;
>    -moz-box-orient: horizontal;
>  }
>  
> +toolbarpaletteitem[place=panel] > .toolbarbutton-1 {

Nitty nit: I would group this with the other customize mode styling.

Comment 19

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8912444 [details]
Bug 1395674 - Adding subviewbutton-nav class to menu-type toolbarbuttons and display the '>' in the overflow list.

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

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5193b9173c69
Adding subviewbutton-nav class to menu-type toolbarbuttons and display the '>' in the overflow list. r=Gijs

Comment 21

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/5193b9173c69

Comment 22

[:Gijs (he/him)]

Comment on attachment 8912444 [details]
Bug 1395674 - Adding subviewbutton-nav class to menu-type toolbarbuttons and display the '>' in the overflow list.

Approval Request Comment
[Feature/Bug causing the regression]: photon changes to panel styling
[User impact if declined]: hard to tell what items in the overflow panel would do
[Is this code covered by automated tests?]: nope, styling change only
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]: test that items that show sliding subviews when they're in the overflow panel actually have a little ">" chevron next to them.
[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: no
[Why is the change risky/not risky?]: minor markup, JS and CSS changes, still quite some beta runway, actual functionality *is* subject to automated tests.
[String changes made/needed]: nope

Comment 23

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8912444 [details]
Bug 1395674 - Adding subviewbutton-nav class to menu-type toolbarbuttons and display the '>' in the overflow list.

recent regression due to photon changes, beta57+

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/569466abab14

Comment 25

[Emil Ghitta, QA [:emilghitta]]

Hi Sam!

It seems that the "Developer" option doesn't have a ">" mark in the overflow menu on Firefox 58.0a1 (Latest Nightly) and Firefox 57.0b7.

Can you have a look into this? 

Thanks!

Comment 26

[Sam Foster [:sfoster] (he/him)]

(In reply to Emil Ghitta, QA [:emilghitta] from comment #25)
> Hi Sam!
> 
> It seems that the "Developer" option doesn't have a ">" mark in the overflow
> menu on Firefox 58.0a1 (Latest Nightly) and Firefox 57.0b7.

Agreed. I can look into this. I've opened bug 1407718, as this one has already landed and uplifted.

Comment 27

[u554753]

On 58: 
'Subscribe' has a '>' mark but does not navigate to a sub-menu.
'Text Encoding' has a '>' mark but does not navigate to a sub-menu.

Subscribe and Text Encoding also do not have highlights when hovering over the selections in the Overflow menu.

Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:58.0) Gecko/20100101 Firefox/58.0

Comment 28

[u554753]

Is this intended?

Comment 29

[Sam Foster [:sfoster] (he/him)]

(In reply to Grover Wimberly IV [:Grover-QA] from comment #27)
> On 58: 
> 'Subscribe' has a '>' mark but does not navigate to a sub-menu.

I can reproduce this. Can you file a bug for it? I can also confirm this item doesnt get a highlight - not sure if that is related. I'm not sure if this was the case when this patch first landed - that might be good to know too, or if it was a more recent regression

> 'Text Encoding' has a '>' mark but does not navigate to a sub-menu.

I get a menu of encoding options when I add Text Encoding to the overflow menu and click on it. And it also highlights correctly (I tried win10 and linux) If you have solid steps to reproduce this that would help

Comment 30

[Sajedul Islam]

I have reproduced this bug with Nightly 57.0a1 (2017-08-31) on Windows 10 , 64 Bit !

This bug's fix is Verified with latest Beta & Nightly !

Build   ID    20171023180840
User Agent    Mozilla/5.0 (Windows NT 10.0; WOW64; rv:57.0) Gecko/20100101 Firefox/57.0

Build   ID    20171025100449
User Agent    Mozilla/5.0 (Windows NT 10.0; WOW64; rv:58.0) Gecko/20100101 Firefox/58.0

[bugday-20171025]

Comment 31

[Mohammad Maruf Rahman]

I have reproduced this Bug with Nightly 57.0a1 (2017-08-31) on Ubuntu 16.04 LTS!

This bug's fix is Verified with latest Beta &  latest Nightly !

Build ID 	20171023180840
User Agent 	Mozilla/5.0 (X11; Linux x86_64; rv:57.0) Gecko/20100101 Firefox/57.0

Build   ID      20171025100449
User Agent      Mozilla/5.0 (X11; Linux x86_64; rv:58.0) Gecko/20100101 Firefox/58.0

Comment 32

[Sajedul Islam]

As par comment 30 & comment 31 , I am marking this bug as Verified Fixed !

Comment 33

[u554753]

We need to check OS X behavior first.

Comment 34

[u554753]

(In reply to Sam Foster [:sfoster] from comment #29)
> (In reply to Grover Wimberly IV [:Grover-QA] from comment #27)
> > On 58: 
> > 'Subscribe' has a '>' mark but does not navigate to a sub-menu.
> 
> I can reproduce this. Can you file a bug for it? I can also confirm this
> item doesnt get a highlight - not sure if that is related. I'm not sure if
> this was the case when this patch first landed - that might be good to know
> too, or if it was a more recent regression
> 
> > 'Text Encoding' has a '>' mark but does not navigate to a sub-menu.
> 
> I get a menu of encoding options when I add Text Encoding to the overflow
> menu and click on it. And it also highlights correctly (I tried win10 and
> linux) If you have solid steps to reproduce this that would help

Bug 1411747 filed.

Comment 35

[Sam Foster [:sfoster] (he/him)]

(In reply to Grover Wimberly IV [:Grover-QA] from comment #34)
> Bug 1411747 filed.

thanks!

Comment 36

[u554753]

Verified on 57 and 58 for Mac OSX