Add "overflowed" property to CustomizableUI widgets

RESOLVED FIXED in Firefox 28

Status

()

RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: mconley, Assigned: mconley)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 28
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 1 obsolete attachment)

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)

Updated

6 years ago
Assignee: nobody → mconley
Whiteboard: [Australis:M?] → [Australis:M7]
Status: NEW → ASSIGNED
Hardware: x86 → All
(Assignee)

Comment 1

6 years ago
Created attachment 760553 [details] [diff] [review]
Patch v1

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+
(Assignee)

Comment 3

6 years ago
(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".
(Assignee)

Comment 5

6 years ago
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]
(Assignee)

Comment 7

6 years ago
(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.
(Assignee)

Comment 8

6 years ago
Filed bug 895429 to rename / flip the nooverflow attribute to overflows.
(Assignee)

Comment 9

6 years ago
Created attachment 777837 [details] [diff] [review]
Patch v1.1

Unbitrotted.
Attachment #760553 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #777837 - Flags: review?(gijskruitbosch+bugs)

Comment 10

6 years ago
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+
(Assignee)

Updated

6 years ago
Blocks: 881905
(Assignee)

Comment 11

6 years ago
Landed in UX as https://hg.mozilla.org/projects/ux/rev/3a2eda2fae3f
Whiteboard: [Australis:M?][Australis:P3] → [Australis:M?][Australis:P3][fixed-in-ux]

Comment 12

5 years ago
https://hg.mozilla.org/mozilla-central/rev/3a2eda2fae3f
Status: ASSIGNED → RESOLVED
Last Resolved: 5 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.