722079 - Error: button is null ... inspector.jsm when Right Clicking on Breadcrumbs

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P1)

Description

[Rob Campbell [:rc] (:robcee)]

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]]

bad

Comment 2

[Paul Rouget [:paul]]

triage, filter on centaur

Comment 3

[Murali]

Hi, I'm interested in fixing this bug. Could you please guide me? Thanks a lot!

Comment 4

[Paul Rouget [:paul]]

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]]

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]]

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]

Attachment: Proposed Patch

Could you please check if its okay?

Comment 8

[Paul Rouget [:paul]]

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]

Attachment: Updated Patch

Hi, Can you please verify this please? Thanks!

Comment 10

[Murali]

Attachment: Changed Patch

I had made a mistake in the previous patch. I think this resolves this. Please advice.

Comment 11

[Paul Rouget [:paul]]

Comment on attachment 611155 [details] [diff] [review]
Changed Patch

Looks good. Thank you Murali!

Comment 12

[Paul Rouget [:paul]]

(for whoever will land that, please add the correct summary and the correct author name to the diff)

Comment 13

[Rob Campbell [:rc] (:robcee)]

https://hg.mozilla.org/integration/fx-team/rev/8bf75b4eafb4

thanks, Murali!

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/8bf75b4eafb4