873712 - Add "overflowed" property to CustomizableUI widgets

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

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

Once bug 865926 lands, it will probably be handy for widgets to know whether or not they've been overflowed, in case they need to do something special. Blair suggested we add an "overflowed" property to the widget wrappers to make this easy.

Comment 1

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

Attachment: Patch v1

Hey Mike,

I think we should start getting you into reviewing some customization patches. Feel OK reviewing this?

-Mike

Comment 2

[Mike de Boer [:mikedeboer]]

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

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

LGTM, but I want to ask you something that might disturb the basis of this patch; isn't it more intuitive in an API to have an 'overflow' property that defaults to TRUE instead of `nooverflow`? Cuz the first thing I wanted to do is camelCase it ;)

Comment 3

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

(In reply to Mike de Boer [:mikedeboer] from comment #2)
> Comment on attachment 760553 [details] [diff] [review]
> Patch v1
> 
> Review of attachment 760553 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM, but I want to ask you something that might disturb the basis of this
> patch; isn't it more intuitive in an API to have an 'overflow' property that
> defaults to TRUE instead of `nooverflow`? Cuz the first thing I wanted to do
> is camelCase it ;)

That's a good question. I really can't think of a good reason to stick with "nooverflow" vs something like "overflows" (it just flips the signs, but I don't think that makes too much of a difference).

Good idea. I'll update the attribute name.

Comment 4

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

(In reply to Mike Conley (:mconley) from comment #3)
> (In reply to Mike de Boer [:mikedeboer] from comment #2)
> > Comment on attachment 760553 [details] [diff] [review]
> > Patch v1
> > 
> > Review of attachment 760553 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > LGTM, but I want to ask you something that might disturb the basis of this
> > patch; isn't it more intuitive in an API to have an 'overflow' property that
> > defaults to TRUE instead of `nooverflow`? Cuz the first thing I wanted to do
> > is camelCase it ;)
> 
> That's a good question. I really can't think of a good reason to stick with
> "nooverflow" vs something like "overflows" (it just flips the signs, but I
> don't think that makes too much of a difference).

Please make sure to keep overflowing the default behavior. For example, if the item doesn't have an overflows attribute, we should default it to overflows="true".

Comment 5

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

Removing the items from M7 that do not block us from landing on m-c.

Comment 6

[Justin Dolske [:Dolske]]

Is this really just "handy" for addons, or more important than that?

Comment 7

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

(In reply to Justin Dolske [:Dolske] from comment #6)
> Is this really just "handy" for addons, or more important than that?

It's handy for add-ons so that they can handle their overflowed case.

Comment 8

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

Filed bug 895429 to rename / flip the nooverflow attribute to overflows.

Comment 9

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

Attachment: Patch v1.1

Unbitrotted.

Comment 10

[:Gijs (he/him)]

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

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

LGTM. We should note in whatever docs we end up with that to use this property, you need to drill down to single widget wrappers, as the overflowed state of an item might be different in different windows, and therefore the property doesn't exist on the group of items.

On a sort of related matter: what happens if you move the edit controls and zoom controls to the navbar and they overflow? I'm guessing they should be getting the "I'm in a panel" appearance, not the toolbar one, but maybe that's naive? I suspect they don't deal with this case yet. If you agree, please file a followup bug.

Comment 11

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

Landed in UX as https://hg.mozilla.org/projects/ux/rev/3a2eda2fae3f

Comment 12

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/3a2eda2fae3f