1170477 - [regression] Breadcrumbs contextual menu doesn't appear anymore (FF41)

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Patrick Brosset <:pbro>]

Looks like I broke the inspector breadcrumbs context menu in bug 1160972 and no test complained.

STR:
- open the inspector on any page
- right click on any part of the breadcrumbs widget in the inspector

ER: the contextual menu appears
AR: the contextual menu doesn't appear and the following error is logged:

TypeError: this.inspector.strings is undefined
breadcrumbs.js:254:4

This is because I changed the place the L10N "strings" property is attached to in bug 1160972. I had no idea it was used somewhere else than in the inspector-panel module, and it looks like we have no tests that check that the context menu appear on breadcrumbs right-click.

Comment 1

[Patrick Brosset <:pbro>]

Attachment: MozReview Request: Bug 1170477 - 1 - Quick code cleanup for better eslint compliancy

Bug 1170477 - 1 - Quick code cleanup for better eslint compliancy

Comment 2

[Patrick Brosset <:pbro>]

Attachment: MozReview Request: Bug 1170477 - 2 - Fix undefined inspector.strings accessor in breadcrumbs

Bug 1170477 - 2 - Fix undefined inspector.strings accessor in breadcrumbs

The only user of the l10s inspector strings bundle is a string that this
change removes.
The "Siblings" string is supposed to tell users that the section of the
menu that follows contains sibling nodes, but it's disabled (and looks like
an item you should normally be able to click on), and it's separated from
the list of siblings by a horizontal separator (making it look like even
less related to the next section).

Also fixed the fact that the inspector context menu was regenerated for
every sibling found on the current node.

Also fixed the fact that the inspector context menu would not appear when
right clicking on the documentElement breadcrumb node.

Comment 3

[Patrick Brosset <:pbro>]

Attachment: MozReview Request: Bug 1170477 - 3 - Test for the inspector breadcrumbs context menu

Bug 1170477 - 3 - Test for the inspector breadcrumbs context menu

Comment 4

[Patrick Brosset <:pbro>]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e84ecb1995fc

Comment 5

[Patrick Brosset <:pbro>]

Attachment: breadcrumbs.gif

Screen capture showing:
- how slow opening the context menu feels,
- the useless (to me at least) "siblings" item

The part 2 patch fixes these 2 things as well as the main problem whereby the menu wouldn't open at all anymore.

Comment 6

[Tom Tromey :tromey]

Comment on attachment 8614040 [details]
MozReview Request: Bug 1170477 - 1 - Quick code cleanup for better eslint compliancy

https://reviewboard.mozilla.org/r/9847/#review8709

Looks good.

Comment 7

[Tom Tromey :tromey]

Comment on attachment 8614041 [details]
MozReview Request: Bug 1170477 - 2 - Fix undefined inspector.strings accessor in breadcrumbs

https://reviewboard.mozilla.org/r/9849/#review8711

Looks good.

Comment 8

[Tom Tromey :tromey]

Comment on attachment 8614042 [details]
MozReview Request: Bug 1170477 - 3 - Test for the inspector breadcrumbs context menu

https://reviewboard.mozilla.org/r/9851/#review8715

This looks good.  Just one nit, see notes.

It took me a while to understand how the tests that say "ok(true,...)" actually worked.
I don't think this is an issue, though, as other tests seem to follow the same basic idea.

::: browser/devtools/inspector/test/browser_inspector_breadcrumbs_menu.js:24
(Diff revision 1)
> +  ok(true, "The context menu appeared on righ-click");

Typo "righ" should be "right".  This shows up in a few spots in this file.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/f3fef0aaa4ca
https://hg.mozilla.org/integration/fx-team/rev/9ec6079da488
https://hg.mozilla.org/integration/fx-team/rev/5b6e1a3e7b9b

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f3fef0aaa4ca
https://hg.mozilla.org/mozilla-central/rev/9ec6079da488
https://hg.mozilla.org/mozilla-central/rev/5b6e1a3e7b9b