Restyle customize mode main palette ('toolbox')

VERIFIED FIXED in Firefox 56

Status

()

Firefox
Toolbars and Customization
P1
normal
VERIFIED FIXED
5 months ago
20 days ago

People

(Reporter: Gijs, Assigned: Gijs)

Tracking

(Blocks: 2 bugs)

53 Branch
Firefox 56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 verified)

Details

(Whiteboard: [photon-structure])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(5 attachments)

(Assignee)

Description

5 months ago
+++ This bug was initially created as a clone of Bug #1354126 +++

See https://mozilla.invisionapp.com/share/5ZAEYEW8M#/screens/225203398 .

We should restyle the main content area to match the spec. This includes:
- icon/margin/padding size adjustments
- not showing a visible border on the main toolbars (it might be useful to still have a border to avoid additional reflows!)
- making the background continuous with the toolbar (ie same colour)
- remove the horizontal line under 'additional tools and features' and restyle the text itself per spec
(Assignee)

Updated

5 months ago
Summary: Restyle customize mode customizable area → Restyle customize mode main palette ('toolbox')

Updated

4 months ago
Flags: qe-verify+
Priority: -- → P2
QA Contact: gwimberly
Whiteboard: [photon] → [photon-structure]
Moving to reserve backlog for now.
Priority: P2 → P3

Updated

3 months ago
Whiteboard: [photon-structure] → [reserve-photon-structure]

Comment 2

3 months ago
Mockup has moved to here: https://mozilla.invisionapp.com/share/8HBR8ZD2B#/230606754_Customize_-_First_Run
(Assignee)

Updated

2 months ago
Blocks: 1373703
(Assignee)

Updated

2 months ago
Blocks: 1374431

Updated

2 months ago
Priority: P3 → P2
Whiteboard: [reserve-photon-structure] → [photon-structure]
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 months ago
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Iteration: --- → 56.2 - Jul 10
Priority: P2 → P1
(Assignee)

Comment 7

2 months ago
mozreview-review
Comment on attachment 8881520 [details]
Bug 1354145 - fix background colour across customize mode as well as integration with the footer,

https://reviewboard.mozilla.org/r/152674/#review157800

::: browser/themes/shared/browser.inc.css:31
(Diff revision 1)
> +  --toolbar-background-color: #f9f9fa;
> +  --toolbar-foreground-color: #0c0c0d;

Requesting review from Dale because I imagine we'll want to use this in bug 1367439, but I can't also fix that bug here because it depends on the tabstrip, and I don't want this fix to wait on the tabstrip changes.
(Assignee)

Updated

2 months ago
No longer blocks: 1374431
Duplicate of this bug: 1374431
(Assignee)

Updated

2 months ago
No longer blocks: 1373703
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 months ago
Blocks: 1354123

Comment 13

2 months ago
mozreview-review
Comment on attachment 8881520 [details]
Bug 1354145 - fix background colour across customize mode as well as integration with the footer,

https://reviewboard.mozilla.org/r/152674/#review157998

This looks great, I was considering creating a CSS var for this already, probably the best way, will update my patch to use it, cheers
Attachment #8881520 - Flags: review?(dale) → review+
(Assignee)

Updated

2 months ago
Blocks: 1367370

Comment 14

2 months ago
mozreview-review
Comment on attachment 8881518 [details]
Bug 1354145 - set photon-structure attribute on root, update palette/panel icon sizes and layout,

https://reviewboard.mozilla.org/r/152670/#review158136

::: browser/components/customizableui/content/panelUI.js:124
(Diff revision 2)
>        this.overflowFixedList.hidden = false;
>        // Also unhide the separator. We use CSS to hide/show it based on the panel's content.
>        this.overflowFixedList.previousSibling.hidden = false;
>        CustomizableUI.registerMenuPanel(this.overflowFixedList, CustomizableUI.AREA_FIXED_OVERFLOW_PANEL);
>        this.navbar.setAttribute("photon-structure", "true");
> +      document.documentElement.setAttribute("photon-structure", "true");

This looks like duplicating the thing for navbar, but it's probably better for perf in the interim.

::: browser/themes/shared/browser.inc:23
(Diff revision 2)
> +
> +%ifdef MOZ_PHOTON_THEME
> +%define panelPaletteIconSize var(--panel-palette-icon-size)
> +%else
> +%define panelPaletteIconSize 32px
> +%endif

Why can't you use `var(--panel-palette-icon-size)` everywhere? It looks like you're overriding it properly in PanelUI.inc.css, so it should be possible...
For perf? If so, why?

::: browser/themes/shared/customizableui/panelUI.inc.css:1172
(Diff revision 2)
>  panelview .toolbarbutton-1,
>  .subviewbutton,
>  .widget-overflow-list .toolbarbutton-1,
>  .panelUI-grid .toolbarbutton-1 > .toolbarbutton-menubutton-button,
>  .share-provider-button,
> -.toolbaritem-combined-buttons@inAnyPanel@ > toolbarbutton {
> +:root:not([photon-structure]) .toolbaritem-combined-buttons@inAnyPanel@ > toolbarbutton {

cart is not going to like this, now is it? Not saying there's a way around it, but just raising awareness...
Attachment #8881518 - Flags: review?(mdeboer) → review-

Comment 15

2 months ago
mozreview-review
Comment on attachment 8881519 [details]
Bug 1354145 - remove dashed outlines for customization targets for photon,

https://reviewboard.mozilla.org/r/152672/#review158138

Good riddance ;)
Attachment #8881519 - Flags: review?(mdeboer) → review+

Comment 16

2 months ago
mozreview-review
Comment on attachment 8881521 [details]
Bug 1354145 - adjust sizing of header and margins/paddings on palette,

https://reviewboard.mozilla.org/r/152676/#review158140

It looks good, except for these two remarks: https://www.evernote.com/l/APu14LVh5SRFfKRuHIWz1p9eE-3ZQGSYVWo
Please let me know what you think!
Attachment #8881521 - Flags: review?(mdeboer) → review-
(Assignee)

Comment 17

2 months ago
(In reply to Mike de Boer [:mikedeboer] from comment #14)
> ::: browser/themes/shared/browser.inc:23
> (Diff revision 2)
> > +
> > +%ifdef MOZ_PHOTON_THEME
> > +%define panelPaletteIconSize var(--panel-palette-icon-size)
> > +%else
> > +%define panelPaletteIconSize 32px
> > +%endif
> 
> Why can't you use `var(--panel-palette-icon-size)` everywhere? It looks like
> you're overriding it properly in PanelUI.inc.css, so it should be possible...
> For perf? If so, why?

Yeah, variables are known to have... issues, perf-wise. But this won't need to be a variable long-term - it'll just be 16px when we ship photon. The only reason we need a variable now is because you can turn it on/off. I would expect that when we're dumping the MOZ_PHOTON_THEME flag for 57 we just hardcode it to 16px and remove the variable. Are you OK with this, or do you feel strongly that we should just use a variable and not worry about it?

> ::: browser/themes/shared/customizableui/panelUI.inc.css:1172
> (Diff revision 2)
> >  panelview .toolbarbutton-1,
> >  .subviewbutton,
> >  .widget-overflow-list .toolbarbutton-1,
> >  .panelUI-grid .toolbarbutton-1 > .toolbarbutton-menubutton-button,
> >  .share-provider-button,
> > -.toolbaritem-combined-buttons@inAnyPanel@ > toolbarbutton {
> > +:root:not([photon-structure]) .toolbaritem-combined-buttons@inAnyPanel@ > toolbarbutton {
> 
> cart is not going to like this, now is it? Not saying there's a way around
> it, but just raising awareness...

Err, not sure why cart particularly would care? I guess it makes things slightly worse for now, but if a single descendant selector like this would make a noticeable impact on cart I think we have worse problems.
Anyway, we're getting rid of things completely anyway (bug 1354123) so I don't think it matters. Note also that this selector doesn't apply on current m-c as-is, so I wouldn't expect the numbers to change when this lands.
Flags: needinfo?(mdeboer)
(In reply to :Gijs from comment #17)
> Yeah, variables are known to have... issues, perf-wise. But this won't need
> to be a variable long-term - it'll just be 16px when we ship photon. The
> only reason we need a variable now is because you can turn it on/off. I
> would expect that when we're dumping the MOZ_PHOTON_THEME flag for 57 we
> just hardcode it to 16px and remove the variable. Are you OK with this, or
> do you feel strongly that we should just use a variable and not worry about
> it?

Oooh now I see that you're trying to affect release as little as possible. It results in hard to follow non directional logic, but we'll learn to live with it. ;-) Alright, ship it.
Flags: needinfo?(mdeboer)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 24

2 months ago
mozreview-review
Comment on attachment 8881518 [details]
Bug 1354145 - set photon-structure attribute on root, update palette/panel icon sizes and layout,

https://reviewboard.mozilla.org/r/152670/#review158272

Nice, thanks!
Attachment #8881518 - Flags: review?(mdeboer) → review+

Comment 25

2 months ago
mozreview-review
Comment on attachment 8881962 [details]
Bug 1354145 - Make customize mode deal with drops in the padding around the panel contents,

https://reviewboard.mozilla.org/r/153026/#review158276

LGTM!

::: browser/components/customizableui/CustomizeMode.jsm:1109
(Diff revision 1)
>        await this._wrapToolbarItem(area);
>      }
>    },
>  
>    _addDragHandlers(aTarget) {
> +    // Allow dropping on the padding of the arrow panel

nit: missing dot.

::: browser/components/customizableui/CustomizeMode.jsm:1130
(Diff revision 1)
>      }
>    },
>  
>    _removeDragHandlers(aTarget) {
> +    // Remove handler from different target if it was added to
> +    // allow dropping on the padding of the arrow panel

nit: missing dot.
Attachment #8881962 - Flags: review?(mdeboer) → review+

Comment 26

2 months ago
mozreview-review
Comment on attachment 8881962 [details]
Bug 1354145 - Make customize mode deal with drops in the padding around the panel contents,

https://reviewboard.mozilla.org/r/153026/#review158280

::: browser/components/customizableui/CustomizeMode.jsm:2210
(Diff revision 1)
>          return aElement;
>        }
>        aElement = aElement.parentNode;
>      }
> +
> +    if (gPhotonStructure) {

Nope! Please make this `gPhotonStructure && aElement`

Comment 27

2 months ago
mozreview-review
Comment on attachment 8881962 [details]
Bug 1354145 - Make customize mode deal with drops in the padding around the panel contents,

https://reviewboard.mozilla.org/r/153026/#review158282

Restore default (empty overflow panel) and try dragging something into it.
You’ll never get an item in there!
Attachment #8881962 - Flags: review+ → review-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 32

2 months ago
mozreview-review
Comment on attachment 8881521 [details]
Bug 1354145 - adjust sizing of header and margins/paddings on palette,

https://reviewboard.mozilla.org/r/152676/#review158372

Goodgood!
Attachment #8881521 - Flags: review?(mdeboer) → review+

Comment 33

2 months ago
mozreview-review
Comment on attachment 8881962 [details]
Bug 1354145 - Make customize mode deal with drops in the padding around the panel contents,

https://reviewboard.mozilla.org/r/153026/#review158374

Alright, ready to ship!
Attachment #8881962 - Flags: review?(mdeboer) → review+

Comment 34

2 months ago
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 3b5768fbbe41 -d ed1964e4da87: rebasing 404572:3b5768fbbe41 "Bug 1354145 - set photon-structure attribute on root, update palette/panel icon sizes and layout, r=mikedeboer"
merging browser/base/content/browser.css
merging browser/themes/shared/customizableui/customizeMode.inc.css
merging browser/themes/shared/customizableui/panelUI.inc.css
rebasing 404573:77dddfa4a4cf "Bug 1354145 - remove dashed outlines for customization targets for photon, r=mikedeboer"
merging browser/themes/shared/customizableui/customizeMode.inc.css
rebasing 404574:aaf968aa9ca5 "Bug 1354145 - fix background colour across customize mode as well as integration with the footer, r=daleharvey"
merging browser/base/content/browser.css
merging browser/themes/shared/customizableui/customizeMode.inc.css
warning: conflicts while merging browser/themes/shared/customizableui/customizeMode.inc.css! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 40

2 months ago
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s e0ce32deb2e1 -d ed1964e4da87: rebasing 404572:e0ce32deb2e1 "Bug 1354145 - set photon-structure attribute on root, update palette/panel icon sizes and layout, r=mikedeboer"
merging browser/base/content/browser.css
merging browser/themes/shared/customizableui/customizeMode.inc.css
rebasing 404573:d2265ecc7d29 "Bug 1354145 - remove dashed outlines for customization targets for photon, r=mikedeboer"
merging browser/themes/shared/customizableui/customizeMode.inc.css
rebasing 404574:5fcb730fdcff "Bug 1354145 - fix background colour across customize mode as well as integration with the footer, r=daleharvey"
merging browser/base/content/browser.css
merging browser/themes/shared/customizableui/customizeMode.inc.css
warning: conflicts while merging browser/themes/shared/customizableui/customizeMode.inc.css! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 46

2 months ago
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/55fd64a8515a
set photon-structure attribute on root, update palette/panel icon sizes and layout, r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/0896f6d55565
remove dashed outlines for customization targets for photon, r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/518a396f9e78
fix background colour across customize mode as well as integration with the footer, r=daleharvey
https://hg.mozilla.org/integration/autoland/rev/8b016e1da34d
Make customize mode deal with drops in the padding around the panel contents, r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/99e8731d94f0
adjust sizing of header and margins/paddings on palette, r=mikedeboer

Comment 47

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/55fd64a8515a
https://hg.mozilla.org/mozilla-central/rev/0896f6d55565
https://hg.mozilla.org/mozilla-central/rev/518a396f9e78
https://hg.mozilla.org/mozilla-central/rev/8b016e1da34d
https://hg.mozilla.org/mozilla-central/rev/99e8731d94f0
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Depends on: 1377413
Improvements noticed! Thank you for landing these

== Change summary for alert #7681 (as of July 04 2017 07:19 UTC) ==

Improvements:

  8%  cart summary linux64 opt e10s     21.77 -> 19.92
  5%  cart summary windows7-32 opt e10s 26.37 -> 25.06

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=7681
Depends on: 1386575
Blocks: 1387512
Verified on Windows, Mac, and Ubuntu
Status: RESOLVED → VERIFIED
status-firefox56: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.