860535 - Make it possible to add widgets to the ends of customization areas

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

My patch for this bug focuses on making it possible to add widgets to the ends of customization areas, it also fixes the following:

1) Makes the nav-bar customization target not weirdly wide (with its flex="100") if the search bar is not in the panel by default
2) Makes it possible to add widgets to the nav-bar even if there were no widgets in there to begin with.

Patch coming in a second.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 736024 [details] [diff] [review]
Patch v1

Review of attachment 736024 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser.xul
@@ +659,5 @@
>                       label="&stopCmd.label;" removable="true"
>                       command="Browser:Stop"
>                       tooltiptext="&stopButton.tooltip;"/>
>  
> +      <hbox id="nav-bar-customizationtarget" class="customization-target" flex="1"/>

As we talked about in person, I don't think the change from flex=100 to flex=1 will be good for addon-compat. We may need to find a different route to go here.

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1.1

Alright, doing away with the flex="1" bit - we'll solve that problem down the road once we figure out what we're going to do with add-ons adding items to the toolbar.

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 736066 [details] [diff] [review]
Patch v1.1

Review of attachment 736066 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/modules/CustomizableUI.jsm
@@ +1377,5 @@
> +    if (aElement.hasAttribute("flex")) {
> +      let parent = aElement.parentNode;
> +      let parentFlex = parent.hasAttribute("flex") ? parseInt(parent.getAttribute("flex"), 10) : 0;
> +      let elementFlex = parseInt(aElement.getAttribute("flex"), 10);
> +      parent.setAttribute("flex", parentFlex + elementFlex);

Negative flex or flex attributes that have non-numeric values are gonna make the behavior of this code somewhat undefined. In practice, I don't think add-ons with >10 users will have this type of issue, so I'm fine with leaving this as-is.

::: browser/themes/shared/customization.inc.css
@@ +36,5 @@
>  }
>  
> +#main-window[customizing] .customization-target {
> +  min-width: 100px;
> +  padding: 0 10px 0 10px;

Are we sure that we want to remove whatever vertical padding we had? Why not just use padding-left:10px; and padding-right:10px;? (besides the downside of the extra verbosity).

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1.2 (r+'d by jaws)

Thanks - swapped out the padding rule for padding-left and padding-right.

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Landed in jamun as https://hg.mozilla.org/projects/jamun/rev/29e4ed6f3d35

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Landed in UX as https://hg.mozilla.org/projects/ux/rev/29e4ed6f3d35

Comment 8

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/db946ac26b87