467360 - support buttons with child panels

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect)

Description

[Neil Deakin]

Attachment: implement this

There was a request by mano at one point to support putting panels inside buttons, as in:

<button id="button" type="panel" label="Button">
  <panel>
    <textbox/>
  </panel>
</button>

that would work just like type="menu".

Comment 1

[Neil Deakin]

Dao, an issue I found with this is that in this example:

<toolbarbutton id="button" type="panel" label="Button">
  <panel>
    <textbox/>
  </panel>
</toolbarbutton>

On Mac, the text-shadow defined on the toolbarbutton is messing up the textbox in the panel. Characters typed aren't being drawn correctly, instead they seem to be just a few pixels of garbage at the left edge of the textbox. Is there still an issue with text-shadow inheritance?

Comment 2

[Dão Gottwald [:dao]]

(In reply to comment #1)
> On Mac, the text-shadow defined on the toolbarbutton is messing up the textbox
> in the panel.

Sure that it's text-shadow and not something else? Should be easy to investigate this (at least, exclude text-shadow) using DOMi, e.g. by looking a the computed style, setting style="text-shadow:none" on the toolbarbutton, etc.

By the way, it's also a little strange that popups inherit text-shadow.

Comment 3

[Neil Deakin]

Actually, it seems more work may be necessary to support this. Another issue is that the button key event handling is also interfering.

Comment 4

[[e-mail]]

Comment on attachment 350769 [details] [diff] [review]
implement this

I guess this works at least as well as <button type="menu"><menupopup ignorekeys="true"> ;-)
Anyway, I have no objection to it.

Comment 5

[Neil Deakin]

Attachment: updated patch, also fixes bug 393884

- changes the popup manager so that it keeps track of the popup type for menu opens, so that focus is cleared in FirePopupShowingEvent.
- disables button key navigation when a button's popup is open or is a popup type of menu.

Comment 6

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Whats needed (aside from review) to get this patch in for 1.9.1? I'm playing with panels and it seems they solve at least some of my problems with popup-in-popups. I think since the text-shadow issue can be worked around quite easily, this bug should go in soon.

Comment 7

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

I also think all other elements that support menupopups should support panels.

Comment 8

[Neil Deakin]

Comment on attachment 351248 [details] [diff] [review]
updated patch, also fixes bug 393884

OK, yes I think only the text shadow issue remains, which is bug 467817.

Comment 9

[Neil Deakin]

(In reply to comment #7)
> I also think all other elements that support menupopups should support panels.

What other elements? The only other elements are all menu related and it doesn't make sense to support panels for them.

Comment 10

[[e-mail]]

Comment on attachment 351248 [details] [diff] [review]
updated patch, also fixes bug 393884

>+        if (this.open)
>           return;
> 
>+        var type = this.type;
>+        if (type != "menu" && type != "menu-button" && type != "panel") {
Just skimming this: I'm not sure these tests are correct, in particular the open test is incorrect for non-menu buttons and the type test isn't generic. My suggestions would be something like this:
if (this.boxObject instanceof Components.interfaces.nsIMenuBoxObject) {
  if (this.open)
    return;
} else {
  /* arrow key processing here */
}

Comment 11

[[e-mail]]

Comment on attachment 351248 [details] [diff] [review]
updated patch, also fixes bug 393884

OK, r=me with comment #10 fixed.

Comment 12

[Neil Deakin]

Attachment: like so? also adds a button test [Checkin: Comment 16]

Comment 13

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 357801 [details] [diff] [review]
like so? also adds a button test
[Checkin: Comment 16]

I'd like to see this in 1.9.1, calendar could make use of this patch.

Comment 14

[Neil Deakin]

(In reply to comment #13)
> (From update of attachment 357801 [details] [diff] [review])
> I'd like to see this in 1.9.1, calendar could make use of this patch.

Phillip, can you explain what UI or feature would make use of this?

Comment 15

[Benjamin Smedberg]

Not blocking. Enn says he doesn't think we want this for 1.9.1, but wants to know why calendar wants/needs it.

Comment 16

[Serge Gautherie (:sgautherie)]

Comment on attachment 357801 [details] [diff] [review]
like so? also adds a button test
[Checkin: Comment 16]


http://hg.mozilla.org/mozilla-central/rev/31d411bd26bc

Comment 17

[Dave Townsend [:mossop]]

From the point of view of just making XUL apps more capable on 1.9.1 I'd like to see this on branch.

Comment 18

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 357801 [details] [diff] [review]
like so? also adds a button test
[Checkin: Comment 16]

a191=beltzner

Comment 19

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/2a76cfcdbb0e

Comment 20

[Mike Kaply [:mkaply]]

Documentation for this should be added to:

https://developer.mozilla.org/en/XUL/toolbarbutton#a-toolbarbutton.type