613156 - Implement splitmenu binding

Status: RESOLVED FIXED

(Firefox :: Menus, defect)

Description

[Dão Gottwald [:dao]]

Attachment: patch

We might need this in bug 589146.

Comment 1

[Dão Gottwald [:dao]]

(It's also just nicer and more sane markup. Adding a binding was originally requested in bug 571842.)

Comment 2

[Dão Gottwald [:dao]]

Comment on attachment 491476 [details] [diff] [review]
patch

>+.split-menuitem-menu {
>+  /* cut off inheritence */
>+  list-style-image: none;
>+  -moz-image-region: auto;
>+}

Actually, this isn't needed, as menus don't inherit that stuff by default.

Comment 3

[Dão Gottwald [:dao]]

Attachment: patch

Comment 4

[Justin Dolske [:Dolske]]

Maybe fodder for a followup, but it would sure be nice if the <splitmenu> could figure out its own label/key/command based on the first <menuitem> inside the popup.

EG, the markup would just be:

<splitmenu id="blah">
  <menupopup>
    <menuitem label="foo" key="bar" command="baz"/>
  </menupopup>
</splitmenu>

(Faaborg says the corrent usage of splitmenus will always be to have the splitmenu and 1st menupopup menuitem look/work the same way.)

Comment 5

[Alex Faaborg [:faaborg] (Firefox UX)]

>(Faaborg says the corrent usage of splitmenus will always be to have the
>splitmenu and 1st menupopup menuitem look/work the same way.)

sometimes they will have a different string (for instance, in the Firefox menu we use "Bookmarks" but with "Show all Bookmarks" as the first item), but yeah, I think we are going to try to have the first item been the same action so that users can continue forward and never have to backtrack.  It's redundant, but seems to significantly reduce confusion and streamline the user choosing between multiple items.

Comment 6

[:Margaret Leibovic]

I think it's worth thinking about how we're going to solve the problems in bug 589146 before taking this patch, especially since it seems like that's what motivated this change. I tried working with this patch, but the new binding still makes it difficult to achieve the hover behavior described in bug 589146. I posted a WIP patch in that bug that uses a different binding to implement the hover effect, and although that patch still has problems, it seems to get closer to the right behavior than my attempts to use this binding.

Comment 7

[Dão Gottwald [:dao]]

(In reply to comment #6)
I don't think waiting with this is really helpful. Bug 589146 can update the binding with whatever may be needed.

Comment 8

[Bill Gianopoulos [:WG9s]]

(In reply to comment #3)
> Created attachment 491493 [details] [diff] [review]
> patch

I have found an issue that is caused by this patch.

Some of the sub-menu items, particularly those on the right hand panel now
seem to perform 2 actions.

For example, selecting options and re-enabling the Menubar results in both the
menubar being enabled and the Options dialog box opening.  Similarly, doing a
help -> about results in the about dialog and the help page being displayed.

Comment 9

[:Margaret Leibovic]

Attachment: command fix

I found that setting a custom attribute on the splitmenu element instead of command/oncommand fixes the problem Bill discovered.

Comment 10

[Dão Gottwald [:dao]]

Attachment: patch v2

with a simpler fix for the issue mentioned in comment 8

Comment 11

[Justin Dolske [:Dolske]]

Comment on attachment 493922 [details] [diff] [review]
patch v2

Seems like the binding should really just live over in toolkit's menu.xml. That's where I'd guess it would be (certainly not urlbarBindings.xml ;). r+ with that.

Comment 12

[Dão Gottwald [:dao]]

(In reply to comment #11)
> Comment on attachment 493922 [details] [diff] [review]
> patch v2
> 
> Seems like the binding should really just live over in toolkit's menu.xml.

The reason I didn't put it in toolkit is that a) it doesn't work for the native mac menu and b) it lacks keyboard access (currently not an issue for the app button, as that can't be opened with the keyboard). I don't think this is ready for arbitrary consumers.

> That's where I'd guess it would be (certainly not urlbarBindings.xml ;). r+
> with that.

urlbarBindings.xml has become the dumping ground for custom browser.xul bindings. We could rename it, but that would affect extensions extending the bindings...

Comment 13

[Bill Gianopoulos [:WG9s]]

I am verifying that this fixes my issue form comment 8.

The patch no longer applies cleanly because of bug 610922.

Merely editing the patch file replacing all occurrences of preferencesCmd.label with preferencesCmd2.label is sufficient to get it to apply to current tip.

Comment 14

[Dão Gottwald [:dao]]

Comment on attachment 493922 [details] [diff] [review]
patch v2

blocks a blocker, so I'm going to just land this

Comment 15

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/405a236b5872

Comment 16

[Jim Jeffery not reading bug-mail 1/2/11]

This broke the Menu Button
History->Expand to view sub-menu
Note that Recently closed tabs/windows is now just a small circle.

Menu Item for History is OK

Comment 17

[Jim Jeffery not reading bug-mail 1/2/11]

Filed: https://bugzilla.mozilla.org/show_bug.cgi?id=616461