1353360 - Update panelmultiview subviews to match photon design

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, enhancement, P1)

Description

[:Gijs (he/him)]

Attachment: Sample mock of subview

Changes:

- no longer show an overlapping bar on the left
- no longer have a concept of an 'anchor' for the subview, nor styling for those.
- change the header font color and styling
- show an arrow button in the header to navigate back to the main view

When doing this we should keep in mind that we may want to allow sub-subviews (etc.) in future (in a separate bug).

Comment 1

[:Gijs (he/him)]

Per conversations with UX, we should be able to do this without putting it behind a pref of sorts.

Comment 2

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1353360 - implement Photon design spec for panel menus.

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

Comment 3

[:Gijs (he/him)]

Comment on attachment 8861511 [details]
Bug 1353360 - implement Photon design spec for panel menus.

https://reviewboard.mozilla.org/r/133486/#review137242

I think this is still missing the hover/active color changes and border changes for items in the subview bodies.

Also, if possible I would prefer to just change the styling for all the panels in one go, rather than the gradual changes here, mostly because it feels like every time we've done the 'gradual change' thing in the past, we've ended up with permanently split styles (like the toolbar button styling being different in the nav bar compared to pretty much everywhere else). Of course, hopefully this time we do it right, but the changes you're making here look relatively small (famous last words!) and like in some cases just updating the existing styles (esp. the !important ones) would be neater. Maybe that's naive though and I'm not seeing some of the problems with that approach?

::: browser/themes/shared/customizableui/panelUI.inc.css:1146
(Diff revision 1)
> +  padding-left: 12px;
> +  padding-right: 12px;
> +  padding-top: 4px;
> +  padding-bottom: 4px;

padding: 4px 12px;

::: browser/themes/shared/customizableui/panelUI.inc.css:1157
(Diff revision 1)
> +photonpanelmultiview .subviewbutton-iconic > .toolbarbutton-text {
> +  padding-inline-start: 8px; /* See '.subviewbutton-iconic > .toolbarbutton-text' rule above. */
> +}
> +
> +photonpanelmultiview .cui-widget-panelview .panel-subview-footer {
> +  font-size: 13px;

Please can we not hardcode font sizes?

::: browser/themes/shared/customizableui/panelUI.inc.css:1853
(Diff revision 1)
> +  padding: 4px;
> +  display: flex;
> +  flex: 1 auto;
> +  align-items: center;

Given the back button is floated, and we center the text using text-align, is this necessary?

::: browser/themes/shared/customizableui/panelUI.inc.css:1883
(Diff revision 1)
> +
> +.panel-header > .subviewbutton-back > .toolbarbutton-text {
> +  display: none;
> +}
> +
> +photonpanelmultiview .PanelUI-subView toolbarseparator {

Can this use a child selector?

::: browser/themes/shared/menu-icons/back-small.svg:4
(Diff revision 1)
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +<svg xmlns="http://www.w3.org/2000/svg" width="12" height="12" viewBox="0 0 12 12">

Here and in the other one, I don't think we need the viewBox attribute

Comment 4

[:Gijs (he/him)]

(In reply to :Gijs from comment #3)
> Comment on attachment 8861511 [details]
> Bug 1353360 - implement Photon design spec for panel menus.
> 
> https://reviewboard.mozilla.org/r/133486/#review137242
> 
> I think this is still missing the hover/active color changes and border
> changes for items in the subview bodies.

Err, I take that back based on actually running the patch. I missed it because it's basically done via a style reset rather than removing extant styling. Sorry!

Comment 5

[Dão Gottwald [:dao]]

(In reply to :Gijs from comment #3)
> Comment on attachment 8861511 [details]
> Bug 1353360 - implement Photon design spec for panel menus.
> 
> https://reviewboard.mozilla.org/r/133486/#review137242
> 
> I think this is still missing the hover/active color changes and border
> changes for items in the subview bodies.
> 
> Also, if possible I would prefer to just change the styling for all the
> panels in one go, rather than the gradual changes here, mostly because it
> feels like every time we've done the 'gradual change' thing in the past,
> we've ended up with permanently split styles (like the toolbar button
> styling being different in the nav bar compared to pretty much everywhere
> else). Of course, hopefully this time we do it right, but the changes you're
> making here look relatively small (famous last words!) and like in some
> cases just updating the existing styles (esp. the !important ones) would be
> neater. Maybe that's naive though and I'm not seeing some of the problems
> with that approach?

I'd also appreciate if you didn't add "photon" to class names and element names and whatnot. Photon is the code name under which we're doing this work, it's not something we need in the code base long-term.

Please test this with high contrast themes too.

Comment 6

[Dão Gottwald [:dao]]

Comment on attachment 8861511 [details]
Bug 1353360 - implement Photon design spec for panel menus.

https://reviewboard.mozilla.org/r/133486/#review137262

::: browser/components/customizableui/PanelMultiView.jsm:464
(Diff revision 1)
>  
>              // Set the transition style and listen for its end to clean up and
>              // make sure the box sizing becomes dynamic again.
>              nodeToAnimate.style.transition = "transform ease-" + (aReverse ? "in" : "out") +
>                " var(--panelui-subview-transition-duration)";
> +            nodeToAnimate.style.borderInlineStart = "1px solid var(--panel-separator-color)";

Can you set an attribute on the node and move these kind of style changes to the stylesheet?

::: browser/themes/shared/customizableui/panelUI.inc.css:1866
(Diff revision 1)
> +  font-size: 13px;
> +  font-weight: 500;
> +  margin: 0;
> +  /* Add the size of the back button to center properly. */
> +  margin-inline-end: 32px;
> +  text-shadow: 0 1px 0 rgba(255, 255, 255, 0.4);

This looks like it will be wrong with dark themes.

Comment 7

[Mike de Boer [:mikedeboer]]

(In reply to Dão Gottwald [::dao] from comment #6)
> Can you set an attribute on the node and move these kind of style changes to
> the stylesheet?

I tried, but that didn't work... do you know what I might be doing wrong?

Comment 8

[Dão Gottwald [:dao]]

(In reply to Mike de Boer [:mikedeboer] from comment #7)
> (In reply to Dão Gottwald [::dao] from comment #6)
> > Can you set an attribute on the node and move these kind of style changes to
> > the stylesheet?
> 
> I tried, but that didn't work... do you know what I might be doing wrong?

Not offhand, no.

Comment 9

[Mike de Boer [:mikedeboer]]

Comment on attachment 8861511 [details]
Bug 1353360 - implement Photon design spec for panel menus.

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

Comment 10

[Mike de Boer [:mikedeboer]]

(In reply to :Gijs from comment #3)
> Also, if possible I would prefer to just change the styling for all the
> panels in one go, rather than the gradual changes here, mostly because it
> feels like every time we've done the 'gradual change' thing in the past,
> we've ended up with permanently split styles (like the toolbar button
> styling being different in the nav bar compared to pretty much everywhere
> else). Of course, hopefully this time we do it right, but the changes you're
> making here look relatively small (famous last words!) and like in some
> cases just updating the existing styles (esp. the !important ones) would be
> neater. Maybe that's naive though and I'm not seeing some of the problems
> with that approach?

Well, I've picked this approach because they're the least invasive for the current style 'system'. The goal - in my view - is to remove everything with 'photon' in it _before_ we uplift 57, thus during the Nightly cycle. This'll be the timeframe where we'll have converted all the panels to use the new, simplified, binding.

> Given the back button is floated, and we center the text using text-align,
> is this necessary?

Apparently, yes. I don't have other ideas.

> Can this use a child selector?

I tried to use this: 'photonpanelmultiview > .panel-viewcontainer > .panel-viewstack > .cui-widget-panelview', but fails spectacularly. Can you see what I'm doing wrong?

Comment 11

[Mike de Boer [:mikedeboer]]

Comment on attachment 8861511 [details]
Bug 1353360 - implement Photon design spec for panel menus.

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

Comment 12

[Mike de Boer [:mikedeboer]]

Comment on attachment 8861511 [details]
Bug 1353360 - implement Photon design spec for panel menus.

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

Comment 13

[:Gijs (he/him)]

(In reply to Mike de Boer [:mikedeboer] from comment #10)
> > Can this use a child selector?
> 
> I tried to use this: 'photonpanelmultiview > .panel-viewcontainer >
> .panel-viewstack > .cui-widget-panelview', but fails spectacularly. Can you
> see what I'm doing wrong?

Looks like you're missing at least the subview-body. But this is also ugly, so let's just leave it like this, then... we can improve this once we remove the non-photon styling.

Comment 14

[:Gijs (he/him)]

Comment on attachment 8861511 [details]
Bug 1353360 - implement Photon design spec for panel menus.

https://reviewboard.mozilla.org/r/133486/#review139820

(In reply to Mike de Boer [:mikedeboer] from comment #10)
> (In reply to :Gijs from comment #3)
> > Also, if possible I would prefer to just change the styling for all the
> > panels in one go, rather than the gradual changes here, mostly because it
> > feels like every time we've done the 'gradual change' thing in the past,
> > we've ended up with permanently split styles (like the toolbar button
> > styling being different in the nav bar compared to pretty much everywhere
> > else). Of course, hopefully this time we do it right, but the changes you're
> > making here look relatively small (famous last words!) and like in some
> > cases just updating the existing styles (esp. the !important ones) would be
> > neater. Maybe that's naive though and I'm not seeing some of the problems
> > with that approach?
> 
> Well, I've picked this approach because they're the least invasive for the
> current style 'system'. The goal - in my view - is to remove everything with
> 'photon' in it _before_ we uplift 57, thus during the Nightly cycle. This'll
> be the timeframe where we'll have converted all the panels to use the new,
> simplified, binding.

My understanding is that these style changes can be shipped pre-57, also for the non-Photon panels. So I think that it will be simpler to make the changes to the existing rules and get them over with, than to keep the previous styling as a separate dead thing. IME it's going to be harder to catch all the dead (ie always overridden) stuff further down the line, than it would be to just change the styling that is now 'wrong' by altering those rules, rather than adding more specific ones that override them and then having to reverse-engineer what they override later on.

::: browser/themes/shared/customizableui/panelUI.inc.css:1157
(Diff revision 4)
> +  margin-right: 0;
> +  padding: 4px 12px;
> +}
> +
> +photonpanelmultiview .subviewbutton-iconic > .toolbarbutton-text {
> +  padding-inline-start: 8px; /* See '.subviewbutton-iconic > .toolbarbutton-text' rule above. */

The non-iconic items need to line up, though, and they don't on Windows:

http://imgur.com/a/fvgLD (175% dpi)

I also noticed the height of the rows is off compared to the spec ( https://mozilla.invisionapp.com/share/ZKBC94BPQ#/screens/229252106_Menus ) - should be 24px. The footer looks more or less OK.

Comment 15

[Mike de Boer [:mikedeboer]]

(In reply to :Gijs from comment #14)
> I also noticed the height of the rows is off compared to the spec (
> https://mozilla.invisionapp.com/share/ZKBC94BPQ#/screens/229252106_Menus ) -
> should be 24px. The footer looks more or less OK.

The Browser Toolbox is lying! clientHeight of the button is reported as 24px correctly.

Comment 16

[Mike de Boer [:mikedeboer]]

Comment on attachment 8861511 [details]
Bug 1353360 - implement Photon design spec for panel menus.

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

Comment 17

[:Gijs (he/him)]

Comment on attachment 8861511 [details]
Bug 1353360 - implement Photon design spec for panel menus.

https://reviewboard.mozilla.org/r/133486/#review140816

As discussed f2f, still needs work on icon alignment.

Comment 18

[Mike de Boer [:mikedeboer]]

Comment on attachment 8861511 [details]
Bug 1353360 - implement Photon design spec for panel menus.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133486/diff/5-6/

Comment 19

[:Gijs (he/him)]

Comment on attachment 8861511 [details]
Bug 1353360 - implement Photon design spec for panel menus.

https://reviewboard.mozilla.org/r/133486/#review141324

Ship it!

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0a6194b15bd6
implement Photon design spec for panel menus. r=Gijs

Comment 21

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

https://hg.mozilla.org/mozilla-central/rev/0a6194b15bd6

Comment 22

[u554753]

Hi Gijs, 

Do you happen to have any STR for how to be able to verify this?

Comment 23

[:Gijs (he/him)]

(In reply to Grover Wimberly IV [:Grover-QA] from comment #22)
> Hi Gijs, 
> 
> Do you happen to have any STR for how to be able to verify this?

This is just about the style and design of the photon menu.

I think to verify, we should ensure:

- identity, downloads, and non-photon hamburger panel (and their standard subviews') appearance is the same as before
- in the photon panel, for all the subviews:
-- the back button works
-- there are headers displayed at the top
-- the hover/active styling & sizing matches the design ( https://mozilla.invisionapp.com/share/ENBBK0F9U#/screens/229252106 )

Comment 24

[u554753]

Attachment: Bookmark menu.png

Is it intended that the mockup does not completely match what has been implemented so far? There doesn't seem to be a header/back button available to choose from, or perhaps I opened it from the wrong location.

Comment 25

[:Gijs (he/him)]

(In reply to Grover Wimberly IV [:Grover-QA] from comment #24)
> Created attachment 8869615 [details]
> Bookmark menu.png
> 
> Is it intended that the mockup does not completely match what has been
> implemented so far? There doesn't seem to be a header/back button available
> to choose from, or perhaps I opened it from the wrong location.

Yes, the back button, titles and changed hover styling will only appear in the main hamburger panel right now.

Comment 26

[u554753]

Verified on Windows, Mac, Ubuntu.