405719 - xul template used on a menu doesn't work

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Neil Deakin]

This doesn't work in current builds, but did in earlier builds (just before the popup reworking for instance). Need to find a regression range.

Expected: clicking the button should give a menu with three items
Actual: no menu

Comment 1

[Martijn Wargers (dead)]

This regressed between 2007-07-04 and 7007-07-05, so a regression from bug 279703.

Comment 2

[Neil Deakin]

OK thanks. I took a look and discovered the problem. Templates don't generate their children for menus until they are opened, which means that the child <menupopup> won't be created. However, as there is no <menupopup>, a frame doesn't exist, so the menu won't open. A vicious circle.

Comment 3

[[e-mail]]

Presumably a workaround is to include a static <menupopup/>? (This seems to be a popular style in Mozilla code.)

Comment 4

[Neil Deakin]

That may work in some cases. It doesn't work for recursive menus though without adding a menupopup in each menu, due to the menu:empty rule in xul.css

I'm working on a fix for the former. The right fix in my opinion for the latter is to remove the menu:empty rule.

Comment 5

[Neil Deakin]

Attachment: fix menus with templates

This patch:
- removes the building which occurs when the open attribute is set to true, and instead builds during the ShowMenu call before the menu is opened. This ensures that the popup exists when the menu is opened, and means that the open attribute does not need to be set earlier than it should be
- changes xul.css so that only empty menus on menubars are collapsed.

This patch allows menus built with templates to not require extra menupopups (as Mozilla 1.8 and earlier don't require them)

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 301270 [details] [diff] [review]
fix menus with templates

>     /**
>      * Invoked lazily by a XUL element that needs its child content
>-     * built.
>+     * built. If aForceMenuCreation is true, then the contents of a
>+     * menu will be generated even if it is closed. If false, a menu
>+     * will only generate its contents if it is open.
>      */
>-    [noscript] void createContents(in nsIContent aElement);
>+    [noscript] void createContents(in nsIContent aElement,
>+                                   in boolean aForceMenuCreation);

Maybe the last parameter could be called just aForceCreation?
IMO, would be great to not have any menu specific things in the interface
(though on practice the parameter is for menu code).

> void
> nsXULPopupManager::ShowMenu(nsIContent *aMenu,
>                             PRBool aSelectFirstItem,
>                             PRBool aAsynchronous)
> {
>+  // generate any template content first. Otherwise, the menupopup may not
>+  // have been created yet.
>+  if (aMenu) {
>+    nsIContent* element = aMenu;
>+    do {
>+      nsCOMPtr<nsIDOMXULElement> xulelem = do_QueryInterface(element);
>+      if (xulelem) {
>+        nsCOMPtr<nsIXULTemplateBuilder> builder;
>+        xulelem->GetBuilder(getter_AddRefs(builder));
>+        if (builder) {
>+          builder->CreateContents(aMenu, PR_TRUE);
>+          break;
>+        }
>+      }
>+      element = element->GetParent();
>+    } while (element);
>+  }

Should this handle also anonymous content? I think not.
Does it matter whether template content is generated in child->parent order, 
or should it be parent first and then child?

So is the too-early-set-open-attribute some sort of regression from 1.8?

Could you explain the xul.css change? Why menubar >

Comment 7

[Neil Deakin]

> Should this handle also anonymous content? I think not.

No.

> Does it matter whether template content is generated in child->parent order, 
> or should it be parent first and then child?

Doesn't matter, but the children won't exist unless the parent has been generated.

> 
> So is the too-early-set-open-attribute some sort of regression from 1.8?

> 
> Could you explain the xul.css change? Why menubar >

That style rule was added by bug 295711 due to the way dom inspector uses overlays to apply to both Firefox, Thunderbird and Seamonkey. It makes menus with no child menupopup invisible. I consider the change made there to be very incorrect, thus I limit it here to only top-level menus.

Otherwise, the documented way of creating menus with templates doesn't work as templates don't create the menupopup until the menu is opened. Especially an issue with submenus since the menu label won't even appear.

Comment 8

[[e-mail]]

Comment on attachment 301270 [details] [diff] [review]
fix menus with templates

smaug seems to have all the important questions covered ;-)

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

But does the .css change still keep some case not-working?
As you suggest in bug 295711, could that css-rule be somewhere in DOMi?

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 301270 [details] [diff] [review]
fix menus with templates

Though, r=me anyway.

Comment 11

[Neil Deakin]

(In reply to comment #9)
> But does the .css change still keep some case not-working?
> As you suggest in bug 295711, could that css-rule be somewhere in DOMi?
> 

Yes, we could move it into dom inspector, but I wasn't sure what change was necessary there. This patch lessens the impact to only apply to <menu> elements on menubars, which aren't generally built with templates, yet allows submenus which are built with templates to work.

Comment 12

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

Comment on attachment 301270 [details] [diff] [review]
fix menus with templates

a=beltzner for 1.9

Comment 13

[Neil Deakin]

Attachment: fix broken test

I had to back out part of nsXULPopupManager which changes the open attribute due to a broken test.

This patch fixes the test and readds the nsXULPopupManager change.

Comment 14

[Neil Deakin]

Checked in the fix for the test.