589300 - DOM Nodes viewer's context menu's Paste and Insert menus should be disabled if no items are enabled

Status: RESOLVED FIXED

(Other Applications :: DOM Inspector, defect)

Description

[Colby Russell :crussell (no longer Mozilla-ing)]

Steps to reproduce:
1. Open a fresh DOM Inspector window on some context document.
2. Bring up the context menu on the document node.
3. Note that the Paste and Insert menuitems have submenus.
4. Force the Paste submenu to show, followed by the Insert submenu.

Actual results:
The Paste and Insert menuitems are enabled, even though neither have any items enabled in their respective submenus.

Expected results:
The Paste menuitem for its submenu shouldn't be enabled if none of the items in its submenu are, and Insert should behave similarly.

From dom.xul:
>    <menupopup id="ppDOMContext"
>               onpopupshowing="viewer.onContextCreate(this)">
...
>        <menupopup onpopupshowing="viewer.onCommandPopupShowing(this)">
...
>        <menupopup onpopupshowing="viewer.onCommandPopupShowing(this)">

If we do a recursive check when the context menu is brought up, we can determine whether the Paste and Insert items are enabled.  It will also take care of the silliness of having two different popupshowing listeners.

Pull out the appropriate pieces of attachment 561734 [details] from bug 310370.

Comment 1

[Colby Russell :crussell (no longer Mozilla-ing)]

Attachment: use single onpopupshowing method

Comment 2

[[e-mail]]

Comment on attachment 483908 [details] [diff] [review]
use single onpopupshowing method

>-  cmdInspectBrowserIsValid: function DVr_CmdInspectBrowserIsValid()
...
>-             isvalid="return viewer.cmdInspectBrowserIsValid()"
>              oncommand="viewer.cmdInspectBrowser()"/>
> 
>     <command id="cmdCopyXML"
>              oncommand="viewer.cmdCopySelectedXML()"/>
> 
>     <command id="cmdBlink"
>-             isvalid="return viewer.cmdBlinkIsValid();"
I guess you need to remove cmdBlinkIsValid too.

Comment 3

[[e-mail]]

Comment on attachment 483908 [details] [diff] [review]
use single onpopupshowing method

>+    var el, hasEnabledItems = false;
Nit: el is only used inside the loop, so you can declare it there.

>+      let isEnabled = false;
If you find a menuitem without a command, should it be enabled or disabled? What if it's in a submenu? (Does anyone extend DOMI?)

>+        checkValid = el.hasAttribute("checkvalid");
[Any particular reason why this was moved from the command to the menuitem?]

>+      var subject = cmd || el;
[You could write let cmd = el; to avoid the ||. Possibly then s/cmd/subject/g]

Comment 4

[Colby Russell :crussell (no longer Mozilla-ing)]

Attachment: remove cmdBlinkIsValid, shift some paths around, and use explicit setAttribute instead of XBL setters for disabled

(In reply to comment #3)
> >+      let isEnabled = false;
> If you find a menuitem without a command, should it be enabled or disabled?
> What if it's in a submenu? (Does anyone extend DOMI?)

I guess we'll assume they know what they're doing, and the disabled state already reflects the correct value.

Side note: if anybody is extending us on that level, it has to be painful for them since we don't use proper controllers and command dispatchers.  It's already a factor in our own troubles (bug 425989).

> >+        checkValid = el.hasAttribute("checkvalid");
> [Any particular reason why this was moved from the command to the menuitem?]

It didn't make sense to me to have an attribute on the command that controlled the visibility of the menuitem.

> >+      var subject = cmd || el;
> [You could write let cmd = el; to avoid the ||. Possibly then s/cmd/subject/g]

I guess I don't follow you.  If I do that, then it will only disable the menuitem instead of the command altogether.

Comment 5

[Colby Russell :crussell (no longer Mozilla-ing)]

Also, I forgot to mention the XBL attachment in the previous comment (although I did mention it in the patch description).

Comment 6

[[e-mail]]

(In reply to comment #4)
> > >+      var subject = cmd || el;
> > [You could write let cmd = el; to avoid the ||. Possibly then s/cmd/subject/g]
> I guess I don't follow you.  If I do that, then it will only disable the
> menuitem instead of the command altogether.

OK, so you've got
let cmd = null;
if (bar(el))
  cmd = baz(el);
var subject = cmd || el;

You can write this as
let subject = el;
if (bar(el))
  subject = baz(el);

Comment 7

[[e-mail]]

Comment on attachment 484132 [details] [diff] [review]
remove cmdBlinkIsValid, shift some paths around, and use explicit setAttribute instead of XBL setters for disabled

+      if (el.hasAttribute("command")) {
+        cmd = document.getElementById(el.getAttribute("command"));
+        if (cmd) {
+          checkValid = el.hasAttribute("checkvalid");
+          isEnabled = this.isCommandEnabled(cmd.id);
+        }
+        else {
+          // There is no command here.  (Maybe we're being extended?)
Comment does not match code: there is a command attribute, but no element, which to me means programmer error, rather than we're being extended.

>+        // We can't use XBL setter here, because the element needs a CSS
>+        // frame for the bindings to apply.
>+        subject.removeAttribute("disabled");
Doesn't that limitation only applied to the main menu on the Mac, or something?

Or should you use getAttribute for the uncontrolled menuitem enabled check?

Comment 8

[Colby Russell :crussell (no longer Mozilla-ing)]

> Doesn't that limitation only applied to the main menu on the Mac, or something?

I thought it was weird, too.  My initial comment was going to be "I was seeing this on GTK with the context menu in question".  Now I know why.  Since we're using recursion, we're performing this on the submenus before they're being invoked, hence no bindings attached.

> Or should you use getAttribute for the uncontrolled menuitem enabled check?

And the menupopup getter, too.  Gah.

Comment 9

[[e-mail]]

Comment on attachment 484132 [details] [diff] [review]
remove cmdBlinkIsValid, shift some paths around, and use explicit setAttribute instead of XBL setters for disabled

In that case I'll wait for a new patch.

Comment 10

[Colby Russell :crussell (no longer Mozilla-ing)]

Attachment: mkIII, address review, split up popupshowing listener method and the computation [Checkin: comment 11]

Comment 11

[Colby Russell :crussell (no longer Mozilla-ing)]

Comment on attachment 486846 [details] [diff] [review]
mkIII, address review, split up popupshowing listener method and the computation [Checkin: comment 11]

Pushed:
http://hg.mozilla.org/dom-inspector/rev/257b43aa4180