Closed Bug 1388167 Opened 4 years ago Closed 4 years ago

Doorhanger footer styles need unifying

Categories

(Firefox :: Menus, enhancement, P1)

Product:
Firefox
Component:
Menus
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 57
Iteration:
57.3 - Sep 19
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: abenson, Assigned: ewright)

References

Details

(Whiteboard: [reserve-photon-structure])

Attachments

(2 files, 1 obsolete file)

Doorhanger footer audit
130.37 KB, image/png
Details
Bug 1388167 - Unify doorhanger footer styles. ui-r=abenson
59 bytes, text/x-review-board-request
johannh
: review+
Details
Attached image Doorhanger footer audit — 
There are a few different doorhanger footer styles in the wild atm. Attached is a quick audit of what's out there with the spec for what we should be aiming for. Blake, I'm attaching this to you only because I heard you might know something about fixing this .. but if not then feel free to clear the NI. Thanks!
Flags: needinfo?(bwinton)
Whiteboard: [photon-structure] → [photon-structure] [triage]
I don't really know what I can add here, except that it should be possible, and hopefully as easy as deleting some custom CSS, to let the various doorhangers use the default styles…  (ProTip: It won't be that easy.
Flags: needinfo?(bwinton)
Flags: qe-verify?
Priority: -- → P3
Whiteboard: [photon-structure] [triage] → [photon-structure]
Whiteboard: [photon-structure] → [reserve-photon-structure]
Priority: P3 → P4
Duplicate of this bug: 1391214
So, I guess I'm going to start working on this now that my other bugs have been (mostly) landed.  Marco, feel free to set whatever flags you set to reflect that.  ;)  Thanks!
Assignee: nobody → bwinton
Status: NEW → ASSIGNED
Priority: P4 → P1
Comment on attachment 8901897 [details]
Bug 1388167 - Unify doorhanger footer styles. .

https://reviewboard.mozilla.org/r/173322/#review178704

::: browser/components/downloads/content/downloadsOverlay.xul:154
(Diff revision 1)
>                          class="downloadsPanelFooterButton"
>                          label="&downloadsHistory.label;"
>                          accesskey="&downloadsHistory.accesskey;"
>                          flex="1"
> -                        oncommand="DownloadsPanel.showDownloadsHistory();"/>
> +                        oncommand="DownloadsPanel.showDownloadsHistory();"
> +                        pack="start"/>

I'm not 100% happy with this, but it seems to work, and is a less invasive change than re-structuring the entire download panel to be more like the other photonpanelmultiviews…
(Oh, and I forgot to round the corners of the overflow menu on Mac!)
Comment on attachment 8901897 [details]
Bug 1388167 - Unify doorhanger footer styles. .

https://reviewboard.mozilla.org/r/173322/#review179576

::: browser/themes/shared/customizableui/panelUI.inc.css:235
(Diff revision 1)
>    margin: 4px 0 0;
> -  -moz-box-pack: center;
> +  padding: 11px 0;
> +}
> +
> +.cui-widget-panelview .subviewbutton.panel-subview-footer > .menu-text {
> +  margin-inline-start: -32px !important;

How was -32px arrived at? And does this need to be !important? Please add a comment in the code explaining why !important is necessary.

::: browser/themes/shared/customizableui/panelUI.inc.css:240
(Diff revision 1)
> +  margin-inline-start: -32px !important;
> +  -moz-box-flex: 1;
> +}
> +
> +.cui-widget-panelview .subviewbutton.panel-subview-footer .menu-accel-container {
> +  float: right;

Will this work correctly in RTL locales? You can go to about:config and set `intl.uidirection` to 1 to flip the browser to RTL mode.
Attachment #8901897 - Flags: review?(jaws) → review-
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #7)
> ::: browser/themes/shared/customizableui/panelUI.inc.css:235
> > +.cui-widget-panelview .subviewbutton.panel-subview-footer > .menu-text {
> > +  margin-inline-start: -32px !important;
> How was -32px arrived at? And does this need to be !important? Please add a
> comment in the code explaining why !important is necessary.

So, that's a good question.  I just changed it to "16px", and it seemed better in rtl…  I think I might need to think about this some more.  (I first got to it by changing it in the browser toolbox until it looked right. ;)

> ::: browser/themes/shared/customizableui/panelUI.inc.css:240
> > +.cui-widget-panelview .subviewbutton.panel-subview-footer .menu-accel-container {
> > +  float: right;
> Will this work correctly in RTL locales? You can go to about:config and set
> `intl.uidirection` to 1 to flip the browser to RTL mode.

Seems to: https://cl.ly/1s1Q0s3B3m1V :D

I also wonder how much of this will get easier once Gijs's panel styling patch lands, so I may wait for that, too.
(I've gotten busy with other stuff, so I'm asking Erica to take this over, and fix all the remaining problems…)
Assignee: bwinton → ewright
Comment on attachment 8907195 [details]
Bug 1388167 - Unify doorhanger footer styles. ui-r=abenson

https://reviewboard.mozilla.org/r/178460/#review184696

A couple of questions/remarks :)

::: browser/themes/shared/customizableui/panelUI.inc.css:237
(Diff revision 1)
> +.cui-widget-panelview .subviewbutton.panel-subview-footer > .menu-text {
> +  -moz-box-flex: 1;
> +}
> +
> +.cui-widget-panelview .subviewbutton.panel-subview-footer .menu-accel-container {
> +  float: right;

Do you still need the float: right when you're scaling the menu-text with -moz-box-flex?

::: browser/themes/shared/customizableui/panelUI.inc.css:238
(Diff revision 1)
> +  -moz-box-flex: 1;
> +}
> +
> +.cui-widget-panelview .subviewbutton.panel-subview-footer .menu-accel-container {
> +  float: right;
> +  color: GrayText;

This should already be set by this rule, no? https://searchfox.org/mozilla-central/rev/6326724982c66aaeaf70bb7c7ee170f7a38ca226/browser/themes/shared/customizableui/panelUI.inc.css#1509

::: browser/themes/shared/customizableui/panelUI.inc.css:1345
(Diff revision 1)
>    min-width: auto;
>    padding: 4px;
>  }
>  
> +#widget-overflow-fixed-list > .toolbarbutton-1 {
> +  padding-inline-start: 18px;

This change seems unrelated to this bug, what's the intent here? :)

::: toolkit/content/widgets/popup.xml:275
(Diff revision 1)
>      <handlers>
>        <handler event="popupshowing" phase="target">
>          <![CDATA[
> +          if (this.id == "BMB_bookmarksPopup") {
> +            return;
> +          }

So, if I'm interpreting this correctly, you're trying to prevent the accel texts from getting width attributes set because that prevents the accel text from aligning on the right edge?

I can see several issues with doing that here. We shouldn't really mix browser and toolkit code, it doesn't scale well for other popups with accel texts, the popup widget should be consumer agnostic.

Can you try changing this -moz-box-pack attribute to "end" as an alternative? https://searchfox.org/mozilla-central/rev/6326724982c66aaeaf70bb7c7ee170f7a38ca226/browser/themes/shared/customizableui/panelUI.inc.css#1506
Attachment #8907195 - Flags: review?(jhofmann)
(Answering for Erica, cause I was involved in some of this work.  Heck, most of the things you mention above were probably my fault. ;)

(In reply to Johann Hofmann [:johannh] from comment #11)
> ::: toolkit/content/widgets/popup.xml:275
> (Diff revision 1)
> >      <handlers>
> >        <handler event="popupshowing" phase="target">
> >          <![CDATA[
> > +          if (this.id == "BMB_bookmarksPopup") {
> > +            return;
> > +          }
> 
> So, if I'm interpreting this correctly, you're trying to prevent the accel
> texts from getting width attributes set because that prevents the accel text
> from aligning on the right edge?

Yep.

> Can you try changing this -moz-box-pack attribute to "end" as an
> alternative?
> https://searchfox.org/mozilla-central/rev/
> 6326724982c66aaeaf70bb7c7ee170f7a38ca226/browser/themes/shared/
> customizableui/panelUI.inc.css#1506

I tried that in the Browser Inspector, and it didn't make a difference. (Nor did text-align, nor any of the other things I tried. I forget whether I tried align-content, though.)  Does it work for you, and if so, are you on Windows 10?
Attachment #8901897 - Attachment is obsolete: true
(In reply to Blake Winton (:bwinton) (:☕️) from comment #12)
> (Answering for Erica, cause I was involved in some of this work.  Heck, most
> of the things you mention above were probably my fault. ;)
> 
> (In reply to Johann Hofmann [:johannh] from comment #11)
> > ::: toolkit/content/widgets/popup.xml:275
> > (Diff revision 1)
> > >      <handlers>
> > >        <handler event="popupshowing" phase="target">
> > >          <![CDATA[
> > > +          if (this.id == "BMB_bookmarksPopup") {
> > > +            return;
> > > +          }
> > 
> > So, if I'm interpreting this correctly, you're trying to prevent the accel
> > texts from getting width attributes set because that prevents the accel text
> > from aligning on the right edge?
> 
> Yep.
> 
> > Can you try changing this -moz-box-pack attribute to "end" as an
> > alternative?
> > https://searchfox.org/mozilla-central/rev/
> > 6326724982c66aaeaf70bb7c7ee170f7a38ca226/browser/themes/shared/
> > customizableui/panelUI.inc.css#1506
> 
> I tried that in the Browser Inspector, and it didn't make a difference. (Nor
> did text-align, nor any of the other things I tried. I forget whether I
> tried align-content, though.)  Does it work for you, and if so, are you on
> Windows 10?

Yes, I just tried it again and it works for me in Windows 10 and OSX. If it keeps on not working for you we might have to find out why.
Maybe Erica can confirm whether that change works for her or not :)
Flags: qe-verify? → qe-verify-
Comment on attachment 8907195 [details]
Bug 1388167 - Unify doorhanger footer styles. ui-r=abenson

https://reviewboard.mozilla.org/r/178460/#review184696

> This change seems unrelated to this bug, what's the intent here? :)

Without this, the items in the overflow menu are too close to the left side (in comparison with the other dropdowns). So, though the issue and fix is with the widgets, it is the footer that appears misaligned. I can revert this if you deem it inappropriate for this patch.

> So, if I'm interpreting this correctly, you're trying to prevent the accel texts from getting width attributes set because that prevents the accel text from aligning on the right edge?
> 
> I can see several issues with doing that here. We shouldn't really mix browser and toolkit code, it doesn't scale well for other popups with accel texts, the popup widget should be consumer agnostic.
> 
> Can you try changing this -moz-box-pack attribute to "end" as an alternative? https://searchfox.org/mozilla-central/rev/6326724982c66aaeaf70bb7c7ee170f7a38ca226/browser/themes/shared/customizableui/panelUI.inc.css#1506

-moz-box-pack also does not work for me on win 10.
(In reply to Johann Hofmann [:johannh] from comment #13)
> 
> Yes, I just tried it again and it works for me in Windows 10 and OSX. If it
> keeps on not working for you we might have to find out why.

Just wanted to make sure we are on the same page here, because this is not a problem in osx. The issue is specifically and only with the bookmarks dropdown shortcut accel text not properly shifting to the right.
Comment on attachment 8907195 [details]
Bug 1388167 - Unify doorhanger footer styles. ui-r=abenson

https://reviewboard.mozilla.org/r/178460/#review185422

::: browser/themes/shared/customizableui/panelUI.inc.css:1210
(Diff revision 3)
> -.widget-overflow-list .toolbarbutton-1 {
> +.widget-overflow-list .toolbarbutton-1,
> +.widget-overflow-list > #search-container {
>    -moz-appearance: none;
>    margin: 0;
>    min-height: 24px;
> -  padding: 4px 12px;
> +  padding: 4px 18px;

By modifying this number you're adjusting the horizontal padding of almost all elements in panels, not just for footers or the overflow menu (e.g. it also affects the bookmarks or library panel). Is that what you want to achieve?
Just as an update, I spoke with Aaron Benson about this, the issues in this bug have been addressed as he'd like. Some other doorhanger styling issues came up (the downloads panel font being smaller, the downloads panel text being center aligned, and the overflow widgets being positioned too close to the left) These other issues will be addressed in other bugs - which I'll open now ;)
Comment on attachment 8907195 [details]
Bug 1388167 - Unify doorhanger footer styles. ui-r=abenson

https://reviewboard.mozilla.org/r/178460/#review185588

It looks much better now, thank you!
Attachment #8907195 - Flags: review?(jhofmann) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/008c2a926077
Unify doorhanger footer styles. ui-r=abenson r=johannh
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/008c2a926077
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Iteration: --- → 57.3 - Sep 19
Depends on: 1400681
Depends on: 1400682
Depends on: 1400810
Depends on: 1401867
You need to log in before you can comment on or make changes to this bug.