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

RESOLVED FIXED

Status

Other Applications
DOM Inspector
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: crussell, Assigned: crussell)

Tracking

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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.
Blocks: 310370
Created attachment 483908 [details] [diff] [review]
use single onpopupshowing method
Assignee: nobody → Sevenspade
Status: NEW → ASSIGNED
Attachment #483908 - Flags: review?(neil)

Comment 2

7 years ago
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

7 years ago
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]
Created attachment 484132 [details] [diff] [review]
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.
Attachment #483908 - Attachment is obsolete: true
Attachment #484132 - Flags: review?(neil)
Attachment #483908 - Flags: review?(neil)
Also, I forgot to mention the XBL attachment in the previous comment (although I did mention it in the patch description).

Comment 6

7 years ago
(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

7 years ago
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?
> 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

7 years ago
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.
Attachment #484132 - Flags: review?(neil)
Created attachment 486846 [details] [diff] [review]
mkIII, address review, split up popupshowing listener method and the computation [Checkin: comment 11]
Attachment #484132 - Attachment is obsolete: true
Attachment #486846 - Flags: review?(neil)

Updated

7 years ago
Attachment #486846 - Flags: review?(neil) → review+
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Summary: Consolidate onCommandPopupShowing and onContextCreate in DOM Nodes viewer → DOM Nodes viewer's context menu's Paste and Insert menus should be disabled if no items are enabled
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
Attachment #486846 - Attachment description: mkII, address review, split up popupshowing listener method and the computation → mkIII, address review, split up popupshowing listener method and the computation [Checkin: comment 11]
Blocks: 592530

Updated

6 years ago
Blocks: 635503
You need to log in before you can comment on or make changes to this bug.