Closed Bug 1353360 Opened 3 years ago Closed 3 years ago

Update panelmultiview subviews to match photon design

Categories

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

53 Branch
enhancement

Tracking

()

VERIFIED FIXED
Firefox 55
Iteration:
55.5 - May 15
Tracking Status
firefox55 --- fixed
firefox56 --- verified

People

(Reporter: Gijs, Assigned: mikedeboer)

References

(Blocks 4 open bugs)

Details

(Whiteboard: [photon-structure])

Attachments

(3 files)

Attached image 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).
Per conversations with UX, we should be able to do this without putting it behind a pref of sorts.
Depends on: 1353362
Depends on: 1354141
Blocks: 1354141
No longer depends on: 1354141
Blocks: 1354144
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Flags: qe-verify+
Priority: -- → P1
QA Contact: gwimberly
Whiteboard: [photon] → [photon-structure]
Iteration: --- → 55.4 - May 1
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
Attachment #8861511 - Flags: review?(gijskruitbosch+bugs)
(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!
(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 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.
Iteration: 55.4 - May 1 → 55.5 - May 15
(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?
Flags: needinfo?(dao+bmo)
(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.
Flags: needinfo?(dao+bmo)
(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?
(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 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.
Attachment #8861511 - Flags: review?(gijskruitbosch+bugs)
(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 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.
Attachment #8861511 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8861511 [details]
Bug 1353360 - implement Photon design spec for panel menus.

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

Ship it!
Attachment #8861511 - Flags: review?(gijskruitbosch+bugs) → review+
Pushed by mdeboer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0a6194b15bd6
implement Photon design spec for panel menus. r=Gijs
https://hg.mozilla.org/mozilla-central/rev/0a6194b15bd6
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Hi Gijs, 

Do you happen to have any STR for how to be able to verify this?
Flags: needinfo?(gijskruitbosch+bugs)
(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 )
Flags: needinfo?(gijskruitbosch+bugs)
Attached image 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.
(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.
Verified on Windows, Mac, Ubuntu.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Blocks: 1387512
No longer blocks: 1425972
You need to log in before you can comment on or make changes to this bug.