Last Comment Bug 722079 - Error: button is null ... inspector.jsm when Right Clicking on Breadcrumbs
: Error: button is null ... inspector.jsm when Right Clicking on Breadcrumbs
Status: RESOLVED FIXED
[good first bug][mentor=paul][lang=js]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Inspector (show other bugs)
: unspecified
: All All
: P1 normal (vote)
: Firefox 14
Assigned To: Murali
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-28 12:41 PST by Rob Campbell [:rc] (:robcee)
Modified: 2012-04-13 03:26 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Proposed Patch (1.80 KB, patch)
2012-03-30 11:50 PDT, Murali
paul: feedback+
Details | Diff | Review
Updated Patch (2.64 KB, patch)
2012-03-30 12:16 PDT, Murali
no flags Details | Diff | Review
Changed Patch (2.31 KB, patch)
2012-03-31 05:31 PDT, Murali
paul: review+
Details | Diff | Review

Description Rob Campbell [:rc] (:robcee) 2012-01-28 12:41:52 PST
The offending source:

    this.menu.addEventListener("popuphiding", (function() {
      while (this.menu.hasChildNodes()) {
        this.menu.removeChild(this.menu.firstChild);
      }
      let button = this.container.querySelector("button[siblings-menu-open]");
      button.removeAttribute("siblings-menu-open"); //// THIS LINE!
    }).bind(this), false);

Can be reproduced by double-right-clicking on a breadcrumbs button.
Comment 1 Paul Rouget [:paul] 2012-03-05 22:43:11 PST
bad
Comment 2 Paul Rouget [:paul] 2012-03-14 10:18:22 PDT
triage, filter on centaur
Comment 3 Murali 2012-03-30 06:43:21 PDT
Hi, I'm interested in fixing this bug. Could you please guide me? Thanks a lot!
Comment 4 Paul Rouget [:paul] 2012-03-30 06:57:47 PDT
Hi Murali. First, I encourage you to join us on IRC (https://wiki.mozilla.org/IRC) channel #devtools.

Did you manage to reproduce the bug? Do you see the exception?

Them, you'll need to look at inspector.jsm (/browser/devtools/highlighter/), the breadcrumbs related code (look for the piece of code Rob's pointed out in comment 0).

I think the problem here is that we are trying to open the context menu twice (see openSiblingMenu) when at the same time, we are being notified that the menu is hiding (code Rob's pointed out).

The solution might be to not open the sibling menu if the sibling menu is already open.
Comment 5 Paul Rouget [:paul] 2012-03-30 07:39:08 PDT
My guess in comment 4 is wrong. This is what happens:

We add the "siblings-menu-open" attribute only on hold (left click for 500ms), not on right click. So when we want to remove the attributes, it fails if the menu has been open with right click.

The easiest way to fix that is just to test if we manage to get the button after the querySelector.
The best way to fix that is to add "siblings-menu-open" on right click. But…

This attribute is used to show a "pressed" state (see /browser/themes/gnomestripe/browser.css, look for "siblings-menu-open") when we open a menu from the non selected button. Try it:

In the breadcrumbs:
- click on a node
- press left click for 500ms on another node
- the initial node is still selected, and the other looks "pressed" but not selected

This is the right behavior.

If we want to do that for right click, we need to add the attribute as well, but also prevent the node to be selected on right click.
Comment 6 Paul Rouget [:paul] 2012-03-30 10:44:36 PDT
The problem is: when the popup disappear (using an extra right click, or using escape, … what ever), and if the popup has been triggered by a right click, then we get this exception.

This happens because we don't use the "siblings-menu-open" attribute on right click, only on hold.

The "siblings-menu-open" attribute is useful to mark a button as pressed, but not selected.

So we need to use it on right click as well. But on right-clicking on a button select the node. This is not the behavior we want. We want the current node to stay selected.

So we need to do 2 things:

1) set the attribute "siblings-menu-open" to "true", on hold and on right click. We should move 'setAttribute("siblings-menu-open", "true")' to the 'openSiblingMenu' function.

2) not select a node on right click. To do so, we just need to not execute this block on right click: http://mxr.mozilla.org/mozilla-central/source/browser/devtools/highlighter/inspector.jsm#2300
Comment 7 Murali 2012-03-30 11:50:57 PDT
Created attachment 610956 [details] [diff] [review]
Proposed Patch

Could you please check if its okay?
Comment 8 Paul Rouget [:paul] 2012-03-30 12:01:42 PDT
Comment on attachment 610956 [details] [diff] [review]
Proposed Patch

Looks good!

Can you please move setAttribute at the end of the function and add your name in the file header?

Once done, re-attach a patch, and ask for review?, and add my name ":paul".

Thank you :)
Comment 9 Murali 2012-03-30 12:16:23 PDT
Created attachment 610964 [details] [diff] [review]
Updated Patch

Hi, Can you please verify this please? Thanks!
Comment 10 Murali 2012-03-31 05:31:07 PDT
Created attachment 611155 [details] [diff] [review]
Changed Patch

I had made a mistake in the previous patch. I think this resolves this. Please advice.
Comment 11 Paul Rouget [:paul] 2012-04-02 03:00:34 PDT
Comment on attachment 611155 [details] [diff] [review]
Changed Patch

Looks good. Thank you Murali!
Comment 12 Paul Rouget [:paul] 2012-04-05 12:15:18 PDT
(for whoever will land that, please add the correct summary and the correct author name to the diff)
Comment 13 Rob Campbell [:rc] (:robcee) 2012-04-12 12:23:16 PDT
https://hg.mozilla.org/integration/fx-team/rev/8bf75b4eafb4

thanks, Murali!
Comment 14 Tim Taubert [:ttaubert] 2012-04-13 03:26:05 PDT
https://hg.mozilla.org/mozilla-central/rev/8bf75b4eafb4

Note You need to log in before you can comment on or make changes to this bug.