Closed Bug 1388180 Opened 7 years ago Closed 7 years ago

Customization Mode > Overflow doorhanger has wrong styles

Categories

(Firefox :: Menus, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 57
Iteration:
57.3 - Sep 19
Tracking Status
firefox57 --- verified

People

(Reporter: abenson, Assigned: mikedeboer)

References

Details

(Whiteboard: [reserve-photon-structure])

Attachments

(1 file)

The overflow doorhanger inside Customization Mode should have the same style as other doorhangers (white background, border, dropshadow). It should also be pointing directly at the Overflow Menu icon in the toolbar. Currently it's offset by a few pixels.

Reference spec: https://mozilla.invisionapp.com/share/WUBRB21NH#/229252091_Customization
Whiteboard: [photon-structure] → [photon-structure] [triage]
Flags: qe-verify?
Priority: -- → P3
Whiteboard: [photon-structure] [triage] → [photon-structure]
Whiteboard: [photon-structure] → [reserve-photon-structure]
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Flags: qe-verify? → qe-verify+
Iteration: --- → 57.3 - Sep 19
Priority: P3 → P1
QA Contact: gwimberly
Comment on attachment 8905963 [details]
Bug 1388180 - Update the overflow panel styling in Customize Mode and show the overflow button as in the opened state, but with a disabled looking icon.

https://reviewboard.mozilla.org/r/177734/#review182842

::: browser/themes/shared/customizableui/customizeMode.inc.css:33
(Diff revision 3)
>  #customization-empty {
>    padding: 5px 20px 20px;
>  }
>  
>  #customization-header {
> -  font-weight: 600;
> +  font-weight: 500;

This works for me on OSX, but it's rather arbitrary on Windows (as usual). I'll happily leave this change out, if you want.
Comment on attachment 8905963 [details]
Bug 1388180 - Update the overflow panel styling in Customize Mode and show the overflow button as in the opened state, but with a disabled looking icon.

https://reviewboard.mozilla.org/r/177734/#review182992

::: browser/components/customizableui/CustomizableUI.jsm:4294
(Diff revision 3)
>  
>    _onClickChevron(aEvent) {
>      if (this._chevron.open) {
>        this._panel.hidePopup();
>        this._chevron.open = false;
> -    } else if (this._panel.state != "hiding") {
> +    } else if (this._panel.state != "hiding" && !this._chevron.disabled) {

Ugh, I guess this is fixing the regression from bug 1395871. I should have caught this in review. :-\

Please add a dep relationship to the bug and/or make a note in that bug.

::: browser/themes/shared/customizableui/customizeMode.inc.css:425
(Diff revision 3)
>    background: var(--arrowpanel-background);
>    background-clip: padding-box;
>    border: 1px solid var(--arrowpanel-border-color);
> -  box-shadow: 0 0 10px hsla(0,0%,0%,.2);
>  %ifdef XP_MACOSX
> +  box-shadow: 0 4px 10px hsla(0,0%,0%,.3);

So my understanding was that this kinda also needs a border on OS X (that doesn't look the same as the shadow. Does this patch do that just by changing the shadow, or should we add a 'harsher' border in customize mode than the 'real' panels have, maybe because they display against a grey background? This might be a question for Aaron... We're currently (ie pre-patch) basically using the panel styling copied from popup.css, so if it's not right then I think we need more to go on than "make it look like the other panels". Maybe you had that conversation already?

::: browser/themes/shared/customizableui/customizeMode.inc.css:436
(Diff revision 3)
>  
>  #customization-panelWrapper > .panel-arrowbox {
>    position: relative;
> -  height: 10px;
>    margin-bottom: -1px;
> +  height: 10px;

Nit: please leave this where you found it. ;-)

::: browser/themes/shared/customizableui/customizeMode.inc.css:440
(Diff revision 3)
>    margin-bottom: -1px;
> +  height: 10px;
> +}
> +
> +#nav-bar[customizing] > .overflow-button {
> +  opacity: 1;

Um, so, the spec has it disabled ( https://mozilla.invisionapp.com/share/BXBFSCU6P#/screens/229252091_Customization ). Do we really want it to not be disabled again, but the spec hasn't been updated, or something?

::: browser/themes/shared/customizableui/customizeMode.inc.css:456
(Diff revision 3)
>                          url("chrome://global/skin/arrow/panelarrow-vertical.png"));
>    /* The OS X image is 2px narrower than the windows/linux one.
>     * Add padding to compensate: */
>    padding: 0 1px;
>    /* specify width for hidpi image to fit correctly */
> -  width: 20px;
> +  width: 18px;

There's 2px padding, and the image is 18px, and in XUL width includes padding (right?), so I don't think this change is right?

::: browser/themes/shared/customizableui/customizeMode.inc.css:473
(Diff revision 3)
> -   * 14px + 3 * toolbarbutton-inner-padding
> -   * Finally, we need to center the arrow, which is 20px wide, so subtract
> -   * 10px.
> +   * 14px + 4 * toolbarbutton-inner-padding
> +   * Finally, we need to center the arrow, which is 18px wide, so subtract
> +   * 9px.
>     */
> -  margin-inline-end: calc(4px + 3 * var(--toolbarbutton-inner-padding));
> +  margin-inline-end: calc(5px + 4 * var(--toolbarbutton-inner-padding));

I remember how much of a pain this is, I'll try testing this on Monday / when talking to you, but clearing review for now given the other comments. :-)
Attachment #8905963 - Flags: review?(gijskruitbosch+bugs)
@Aaron - In Customize Mode, do you want the overflow button to look 'selected'/ 'open' or disabled? The spec at https://mozilla.invisionapp.com/share/BXBFSCU6P#/screens/229252091_Customization is a bit ambiguous; show one screen with a disabled state, but all the others with the selected state.

Personally, I find the selected state more clear and connects better with the panel shown below it, but up to you!
Flags: needinfo?(abenson)
Mike, great point. Lets do a hybrid of the selected/open state and disabled state. The idea is to represent clearly what the doorhanger is pointing to *and* that you can't move its position in the toolbar. See this updated comp: https://mozilla.invisionapp.com/share/Y7BQQLJDC#/229252092_Customize_-_Empty_Overflow

The selected/open background should be the same as it is for when things like the Libary doorhanger are open and the icon for the Overflow Menu should be the same as the Application Menu.
Flags: needinfo?(abenson)
Comment on attachment 8905963 [details]
Bug 1388180 - Update the overflow panel styling in Customize Mode and show the overflow button as in the opened state, but with a disabled looking icon.

This only looks right on OSX... need to do more on Windows.
Attachment #8905963 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8905963 [details]
Bug 1388180 - Update the overflow panel styling in Customize Mode and show the overflow button as in the opened state, but with a disabled looking icon.

Ugh, was a build problem; it actually looks awesome on Windows.
Attachment #8905963 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8905963 [details]
Bug 1388180 - Update the overflow panel styling in Customize Mode and show the overflow button as in the opened state, but with a disabled looking icon.

https://reviewboard.mozilla.org/r/177734/#review184472

Close, but I don't think the arrow positioning is correct still. :-(

::: browser/themes/shared/customizableui/customizeMode.inc.css:477
(Diff revision 4)
> -   * Finally, we need to center the arrow, which is 20px wide, so subtract
> -   * 10px.
> +   * Finally, we need to center the arrow, which is 18px wide, so subtract
> +   * 9px.
>     */
> -  margin-inline-end: calc(4px + 3 * var(--toolbarbutton-inner-padding));
> +  margin-inline-end: calc(5px + 4 * var(--toolbarbutton-inner-padding));

The arrow container is 20px wide, and so this doesn't really make sense still...

This is also stil off in touch mode. :-(

The 4 * inner padding is also confusing... the original intent here is to have the following situation:

```
[pad[overflo]pad]|border|[pad[hamburger]pad]
.....[arrow]<---margin-for-this-distance--->
```

So in my head, the margin on the arrow should be:

3 * padding + hamburger button size + margin/border of hamburger + half the overflow button size
- half the arrow size - customization panel container padding.

I don't really understand where padding #4 comes from (and from what I can tell, the result is still not quite right).

Is the panel code setting inline padding/margin on the arrow or something?
Attachment #8905963 - Flags: review?(gijskruitbosch+bugs)
OK, I *think* this is right (to be applied on top of your patch): https://pastebin.mozilla.org/9032311 . Mike, can you verify?
Flags: needinfo?(mdeboer)
(In reply to :Gijs from comment #12)
> OK, I *think* this is right (to be applied on top of your patch):
> https://pastebin.mozilla.org/9032311 . Mike, can you verify?

LGTM, but I'll check! Oh and good that you mentioned touch mode... I'll take that into account as well.
Flags: needinfo?(mdeboer)
Comment on attachment 8905963 [details]
Bug 1388180 - Update the overflow panel styling in Customize Mode and show the overflow button as in the opened state, but with a disabled looking icon.

https://reviewboard.mozilla.org/r/177734/#review184588

Thanks!
Attachment #8905963 - Flags: review?(gijskruitbosch+bugs) → review+
Pushed by mdeboer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c55dbc386e21
Update the overflow panel styling in Customize Mode and show the overflow button as in the opened state, but with a disabled looking icon. r=Gijs
https://hg.mozilla.org/mozilla-central/rev/c55dbc386e21
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Depends on: 1402311
Reproduced the initial issue on an affected Nightly build from 2017-08-07.

Verified fixed on Beta 57.0b7 (20171009192146) on Windows 10 x64, Mac OS X 10.11 and Ubuntu 16.04 x64.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.