Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED in Firefox 14

Status

()

Firefox
Developer Tools: Inspector
P1
normal
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: rc, Assigned: Murali)

Tracking

unspecified
Firefox 14
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=paul][lang=js])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

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

6 years ago
bad
Whiteboard: [good first bug][mentor=paul][lang=js]

Comment 2

5 years ago
triage, filter on centaur
Priority: -- → P1
(Assignee)

Comment 3

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

Comment 4

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

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

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

Updated

5 years ago
Assignee: nobody → murali.sr92
Status: NEW → ASSIGNED
(Assignee)

Comment 7

5 years ago
Created attachment 610956 [details] [diff] [review]
Proposed Patch

Could you please check if its okay?
Attachment #610956 - Flags: feedback?

Comment 8

5 years ago
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 :)
Attachment #610956 - Flags: feedback? → feedback+
(Assignee)

Comment 9

5 years ago
Created attachment 610964 [details] [diff] [review]
Updated Patch

Hi, Can you please verify this please? Thanks!
Attachment #610956 - Attachment is obsolete: true
Attachment #610964 - Flags: review?
(Assignee)

Comment 10

5 years ago
Created attachment 611155 [details] [diff] [review]
Changed Patch

I had made a mistake in the previous patch. I think this resolves this. Please advice.
Attachment #610964 - Attachment is obsolete: true
Attachment #610964 - Flags: review?
Attachment #611155 - Flags: review?

Comment 11

5 years ago
Comment on attachment 611155 [details] [diff] [review]
Changed Patch

Looks good. Thank you Murali!
Attachment #611155 - Flags: review? → review+

Updated

5 years ago
Whiteboard: [good first bug][mentor=paul][lang=js] → [good first bug][mentor=paul][lang=js][land-in-fx-team]

Comment 12

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

Comment 13

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

thanks, Murali!
Whiteboard: [good first bug][mentor=paul][lang=js][land-in-fx-team] → [good first bug][mentor=paul][lang=js][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/8bf75b4eafb4
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][mentor=paul][lang=js][fixed-in-fx-team] → [good first bug][mentor=paul][lang=js]
Target Milestone: --- → Firefox 14
You need to log in before you can comment on or make changes to this bug.