1269915 - Remove context menu integration from breadcrumbs widget

Status: VERIFIED FIXED

(DevTools :: Inspector, defect, P1)

Description

[Brian Grinstead [:bgrins]]

Splitting this out from Bug 1259812.  See https://bugzilla.mozilla.org/show_bug.cgi?id=1259812#c1 for more context.

Comment 1

[Brian Grinstead [:bgrins]]

Submitting for triage since this is blocking context menu work at bug 1266478

Comment 2

[Steve Melia]

Attachment: Remove context menu from breadcrumbs

Split out from patch for Bug 1259812

Can I draw your attention to breadcrumbs.js:473 which is different from the patch on that bug; rather than change the tests I kept this click handler - I had previously thought this was a difference with XUL buttons. (jumped to conclusions during fiddling about)

Also I included the linting in this patch hope that's OK.

Comment 3

[Brian Grinstead [:bgrins]]

Comment on attachment 8748713 [details] [diff] [review]
Remove context menu from breadcrumbs

Review of attachment 8748713 [details] [diff] [review]:
-----------------------------------------------------------------

Nice, looks great.  Thanks Steve!  Let's see how the test server likes it: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d502ac097d7e

Comment 4

[Brian Grinstead [:bgrins]]

Marking for checkin, since try is looking good

Comment 5

[Steve Melia]

Hey Brian I've just realised my patch doesn't contain my user info - I just copied it out of my mq series instead of importing it. Lesson learnt I guess. Does this matter?

Comment 6

[Brian Grinstead [:bgrins]]

(In reply to Steve Melia from comment #5)
> Hey Brian I've just realised my patch doesn't contain my user info - I just
> copied it out of my mq series instead of importing it. Lesson learnt I
> guess. Does this matter?

Yeah, let's make sure your authorship info is in the patch before landing

Comment 7

[Steve Melia]

Attachment: Update patch to include authorship

What an idiot.

Comment 8

[Brian Grinstead [:bgrins]]

Copying out of your mq series should include authorship info - does your .hgrc have the username config? https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 9

[Steve Melia]

yeah it's got the [ui] section and works fine when I "hg export qtip"; I think my lazy error is copying files out of .hg/patches... i'll do a bit more reading, need to investigate mozreview as well I think.

Comment 10

[Brian Grinstead [:bgrins]]

I have this in the [defaults] section: https://gist.github.com/bgrins/a323ae0aaffbf835b3b5#file-hgrc-L18. I think the qnew = -Ue does the trick, where U adds authorship info (and e lets you add a commit message right away)

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/29662e28a9c9

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/29662e28a9c9

Comment 13

[Ada [:adalucinet]]

Confirming that with latest Nightly 49.0a1, across platforms [1], the context menu is no longer applicable for breadcrumbs toolbar. While performing some additional exploratory testing, I've encountered bug 1273125 and noticed a dotted rectangle when an element from breadcrumbs toolbar is selected (Mac 10.11.1 and Ubuntu 14.04 32-bit only) → https://goo.gl/z7oS3L; is this an expected behavior? 

[1] Windows 10 64-bit, Ubuntu 14.04 32-bit and Mac OS X 10.11.1

Comment 14

[Steve Melia]

I had a feeling this might be related to Bug 1242851 (improving focus highlighting) and a mozregression found the following pushlog https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=5a937f17f3369f64554d5552ecfeb22ef4757fcb&tochange=d5b56d7097ba3ba8b77f88b7f57c6afba7e10c78

So - intentional I think; but it's probably a good idea for Yura to confirm. (i'm quite a new contributor...)

Comment 15

[Yura Zenevich [:yzen] (please use "needinfo")]

Focus highlighting will be removed once bug 1272011 is implemented. We should rather focus on the breadcrumbs container instead of each particular one.