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

VERIFIED FIXED in Firefox 57

Status

()

defect
P1
normal
VERIFIED FIXED
2 years ago
a year ago

People

(Reporter: alice0775, Assigned: sfoster)

Tracking

(Depends on 3 bugs, Blocks 1 bug, {polish})

57 Branch
Firefox 58
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox55 unaffected, firefox56 disabled, firefox57- verified, firefox58 verified)

Details

(Whiteboard: [reserve-photon-structure])

Attachments

(2 attachments)

Reporter

Description

2 years ago
[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.
Reporter

Updated

2 years ago
See Also: → 1393777
Reporter

Comment 1

2 years ago
Posted image screenshot
Whiteboard: [photon-visual] → [photon-visual] [triage]
Whiteboard: [photon-visual] [triage] → [photon-structure][triage]
Flags: qe-verify?
Priority: -- → P4
Whiteboard: [photon-structure][triage] → [reserve-photon-structure]
P4 bug, not tracking for 57.
Flags: qe-verify? → qe-verify+
QA Contact: gwimberly

Updated

2 years ago
Priority: P4 → P3
Assignee

Comment 3

2 years ago
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 :)
Assignee: nobody → sfoster
Flags: needinfo?(gijskruitbosch+bugs)
Status: NEW → ASSIGNED
Priority: P3 → P1

Comment 4

2 years ago
(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. :-)
Flags: needinfo?(gijskruitbosch+bugs)
Assignee

Updated

2 years ago
Blocks: 1403388
Assignee

Comment 5

2 years ago
Filed bug 1403388 for the checkbox type buttons. Adjusting summary here to reflect the menu-type focus of this bug.
Summary: [Photon] Inconsistent appearance of sliding subview menu and checkbox type menu → [Photon] Inconsistent appearance of sliding subviews in the overflow menu
Assignee

Comment 7

2 years ago
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

2 years ago
mozreview-review
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?
Attachment #8912444 - Flags: review?(gijskruitbosch+bugs) → review+
Assignee

Comment 9

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

Comment 11

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

Comment 12

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

Updated

2 years ago
Depends on: 1403666
> > 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
Flags: needinfo?(gijskruitbosch+bugs)

Comment 15

2 years ago
(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.
Flags: needinfo?(gijskruitbosch+bugs)
Comment hidden (mozreview-request)
Assignee

Comment 17

2 years ago
mozreview-review-reply
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

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

Comment 20

2 years ago
Pushed by sfoster@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/5193b9173c69
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58

Comment 22

2 years ago
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
Attachment #8912444 - Flags: approval-mozilla-beta?
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+
Attachment #8912444 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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!
Flags: needinfo?(sfoster)
(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.
Flags: needinfo?(sfoster)
Assignee

Updated

2 years ago
Depends on: 1407718

Comment 27

2 years ago
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

2 years ago
Is this intended?
Flags: needinfo?(sfoster)
(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
Flags: needinfo?(sfoster)

Comment 30

2 years ago
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]
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
QA Whiteboard: [bugday-20171025]

Comment 32

2 years ago
As par comment 30 & comment 31 , I am marking this bug as Verified Fixed !
Status: RESOLVED → VERIFIED

Comment 33

2 years ago
We need to check OS X behavior first.

Updated

2 years ago
Status: VERIFIED → RESOLVED
Last Resolved: 2 years ago2 years ago

Comment 34

2 years ago
(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.
Flags: needinfo?(sfoster)
(In reply to Grover Wimberly IV [:Grover-QA] from comment #34)
> Bug 1411747 filed.

thanks!
Flags: needinfo?(sfoster)

Comment 36

2 years ago
Verified on 57 and 58 for Mac OSX
Status: RESOLVED → VERIFIED
Depends on: 1443025
You need to log in before you can comment on or make changes to this bug.