1273125 - Unable to select a different element if the already selected one is not visible in the breadcrumbs toolbar

Status: NEW

(DevTools :: Inspector, defect, P2)

Description

[Ada [:adalucinet]]

Attachment: Screenrecording

[Affected versions]:
- latest Aurora 48.0a2 
- latest Nightly 49.0a1

[Affected platforms]:
- Windows 10 64-bit
- Ubuntu 14.04 32-bit

[Steps to reproduce]:
1. Launch Firefox.
2. Open the Inspector (Ctrl+Shift+C/Cmd+Opt+C)
3. Select any element on the page so that the arrow icons at the left&right of the breadcrumbs toolbar are visible.
4. Scroll left or right so that the selected breadcrumb is not visible in the toolbar.
5. Select another breadcrumb.

[Expected result]:
- Element is properly selected.

[Actual result]:
- Focus jumps back to the already selected breadcrumb.

[Regression range]:
- Regressed by https://bugzil.la/1242852 

[Additional notes]:
- Note that on Windows 10 64-bit there is an intermittent glitch and the breadcrumbs area is blank.
- Gif attached.
- Unable to reproduce on Mac OS X 10.11.1
- Not reproducible with Firefox 47 beta 5 nor with Firefox 46.0.1 RC

Comment 1

[Steve Melia]

This is the same symptom as described in Bug 1259812, Comment 19

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

Steve, would this bug resolved by Bug 1259812 ? If so, maybe we can close this one as duplicate ?

Comment 3

[Steve Melia]

Attachment: 1273125.patch

Hey Nicolas! Yes I had a fix in Bug 1259812 but it was awful - I wasn't really satisfied with it. Thinking about this in isolation has helped me come up with a much better solution.

I understand the expected behaviour to be:
- When the current crumb is not visible; clicking a different crumb should select that crumb and keep it visible.
- When the current crumb is not visible; tabbing into the breadcrumb trail should scroll to the current crumb.

See attached for what I think is the resolution - to just make sure the new crumb is selected before the focus event fires, by bubbling instead of capturing.

I think it is probably better to split the patch; but happy to accept your guidance on that!

Comment 4

[Steve Melia]

Attachment: 1273125-suspendFocus.patch

My last is nonsense and fails a mochitest, sorry. The attached is the original "awful" fix given with Bug 1259812 which is the best i've got i'm afraid.

This shares a flag across two event handlers, which is why I think it's less than ideal.

Comment 5

[Steve Melia]

Comment on attachment 8755231 [details] [diff] [review]
1273125-suspendFocus.patch

Sorry about this I am struggling a bit trying to finish Bug 1259812 and not really giving this my full attention. While testing that case i've already found an issue with this suspendFocus flag:

1. Click a breadcrumb.
2. Click another breadcrumb - this will set the suspendFocus flag without clearing it, since the breadcrumb container already has focus
3. Press tab to tab off the breadcrumb container
4. Press shift-tab to tab back on.

Actual: The last breadcrumb button will be highlighted
Expected: The selected breadcrumb button will be highlighted.

So as anticipated it's a bad idea and needs a bit more thought - i've cleared the review flag for now; I may be able to look at this later in the week.

Comment 6

[Joe Walker [:jwalker]]

This bug has a priority set that is not P1, so it's not an urgent fix.

Comment 7

[Steve Melia]

Whoops, forgot about this. Take a look at the end of Bug 1259812 Comment 45 - stating this will be fixed with Bug 1272011 - the reason being that clicking the "different element" causes the "already selected one" to receive focus (and thus scroll into view) whereas Bug 1272011 will mean the "already selected one" never receives focus. (In favour of the breadcrumb container)

I've added this as a "Depends on"

Comment 8

[Milan Sreckovic [:milan] (needinfo for best results)]

Tracking flags update please...