Closed Bug 873712 Opened 8 years ago Closed 7 years ago

Add "overflowed" property to CustomizableUI widgets

Categories

(Firefox :: Toolbars and Customization, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 28

People

(Reporter: mconley, Assigned: mconley)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Australis:M?][Australis:P3])

Attachments

(1 file, 1 obsolete file)

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.
Assignee: nobody → mconley
Whiteboard: [Australis:M?] → [Australis:M7]
Status: NEW → ASSIGNED
Hardware: x86 → All
Attached patch Patch v1 (obsolete) — Splinter Review
Hey Mike,

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

-Mike
Attachment #760553 - Flags: review?(mdeboer)
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 ;)
Attachment #760553 - Flags: review?(mdeboer) → feedback+
(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.
(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".
Removing the items from M7 that do not block us from landing on m-c.
Whiteboard: [Australis:M7] → [Australis:M?]
Is this really just "handy" for addons, or more important than that?
Whiteboard: [Australis:M?] → [Australis:M?][Australis:P3]
(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.
Filed bug 895429 to rename / flip the nooverflow attribute to overflows.
Attached patch Patch v1.1Splinter Review
Unbitrotted.
Attachment #760553 - Attachment is obsolete: true
Attachment #777837 - Flags: review?(gijskruitbosch+bugs)
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.
Attachment #777837 - Flags: review?(gijskruitbosch+bugs) → review+
Landed in UX as https://hg.mozilla.org/projects/ux/rev/3a2eda2fae3f
Whiteboard: [Australis:M?][Australis:P3] → [Australis:M?][Australis:P3][fixed-in-ux]
https://hg.mozilla.org/mozilla-central/rev/3a2eda2fae3f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:M?][Australis:P3][fixed-in-ux] → [Australis:M?][Australis:P3]
Target Milestone: --- → Firefox 28
You need to log in before you can comment on or make changes to this bug.