SideMenuWidget should be keyboard accessible

RESOLVED FIXED in Firefox 24

Status

DevTools
Debugger
P2
normal
RESOLVED FIXED
5 years ago
8 days ago

People

(Reporter: vporof, Assigned: vporof)

Tracking

unspecified
Firefox 24
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Comment hidden (empty)
(Assignee)

Updated

5 years ago
Assignee: nobody → vporof
Priority: -- → P2
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 1

5 years ago
Created attachment 751443 [details] [diff] [review]
v1

WIP. Probably just needs a test.

Fun fact: keyboard navigation in the debugger's sources list doesn't really work, because the current behavior is:
1. click on item => 2. source editor gets focused.

You need to sidemenuwidget to be focused in order for keyboard navigation to work. Mind blown.

In any case, I don't think it's wise to change this behavior (most likely if you clicked on a source url, you want to immediately start navigating in the source text).
(In reply to Victor Porof [:vp] from comment #1)
> Created attachment 751443 [details] [diff] [review]
> v1
> 
> WIP. Probably just needs a test.
> 
> Fun fact: keyboard navigation in the debugger's sources list doesn't really
> work, because the current behavior is:
> 1. click on item => 2. source editor gets focused.

Why not use Ctrl + UP/DOWN ? That way, you will not need to have focus on the sidemenuwidget. Netmonitor will be treated as exception and there, normal UP/DOWN without any modifiers will work.

Because given the current behaviors, the same problem will exist for Style Editor (and Options Panel in future) also (when it gets converted to sidemenuwidget, that is).
(Assignee)

Comment 3

5 years ago
(In reply to Girish Sharma [:Optimizer] from comment #2)
> (In reply to Victor Porof [:vp] from comment #1)
> > Created attachment 751443 [details] [diff] [review]
> > v1
> > 
> > WIP. Probably just needs a test.
> > 
> > Fun fact: keyboard navigation in the debugger's sources list doesn't really
> > work, because the current behavior is:
> > 1. click on item => 2. source editor gets focused.
> 
> Why not use Ctrl + UP/DOWN ? That way, you will not need to have focus on
> the sidemenuwidget. Netmonitor will be treated as exception and there,
> normal UP/DOWN without any modifiers will work.

The widget has no knowledge of where it lives. If a tool decides that Ctrl + UP/DOWN is a good idea for keyboard navigation for the widget, it will have to implement it by itself. 

There are helpers like focusNextItem and focusPrevItem on the MenuContainer prototype that make this endeavor trivial.

I don't think highjacking the owner document (*by the widget*) and injecting keys and commands programatically is a good idea. Moreover, the widget may live secluded in an iframe, in which case deciding which is the owner document where key/command injection needs to happen is very tricky. We could add helpers for this, sure, but it's out of the scope of this bug.
(In reply to Victor Porof [:vp] from comment #3)
> (In reply to Girish Sharma [:Optimizer] from comment #2)
> > (In reply to Victor Porof [:vp] from comment #1)
> > > Created attachment 751443 [details] [diff] [review]
> > > v1
> > > 
> > > WIP. Probably just needs a test.
> > > 
> > > Fun fact: keyboard navigation in the debugger's sources list doesn't really
> > > work, because the current behavior is:
> > > 1. click on item => 2. source editor gets focused.
> > 
> > Why not use Ctrl + UP/DOWN ? That way, you will not need to have focus on
> > the sidemenuwidget. Netmonitor will be treated as exception and there,
> > normal UP/DOWN without any modifiers will work.
> 
> The widget has no knowledge of where it lives. If a tool decides that Ctrl +
> UP/DOWN is a good idea for keyboard navigation for the widget, it will have
> to implement it by itself. 

I know, it should not be implemented by the widget itself. In options panel, I was adding the listeners explicitly for Ctrl UP/DOWN when I was using the widget. What I am trying to say is that maybe for debugger, the fix can go along in this patch itself.
(Assignee)

Comment 5

5 years ago
(In reply to Girish Sharma [:Optimizer] from comment #4)
> 
> I know, it should not be implemented by the widget itself. In options panel,
> I was adding the listeners explicitly for Ctrl UP/DOWN when I was using the
> widget. What I am trying to say is that maybe for debugger, the fix can go
> along in this patch itself.

Maybe. It would be cleaner to have a different bug like "implement ctrl+up/down in the debugger to navigate the sources list", since this bug is only specifically about making the sidemenuwidget accessible, but I'll think about it.
(Assignee)

Comment 6

5 years ago
Created attachment 751463 [details] [diff] [review]
v2

Cleaned up things. Gave the debugger some love too.
Attachment #751443 - Attachment is obsolete: true
(Assignee)

Comment 7

5 years ago
Created attachment 751501 [details] [diff] [review]
v3

Made existing tests pass again.
Attachment #751463 - Attachment is obsolete: true
(Assignee)

Comment 8

5 years ago
Created attachment 751604 [details] [diff] [review]
v4

Added tests. Woo!
Attachment #751501 - Attachment is obsolete: true
Attachment #751604 - Flags: review?(rcampbell)
So I assume that bug 859271 is also getting fixed in this one ?
adding 848504 to series file
applying 848504
patching file browser/devtools/netmonitor/test/browser_net_filter-03.js
Hunk #1 FAILED at 93
1 out of 1 hunks FAILED -- saving rejects to file browser/devtools/netmonitor/test/browser_net_filter-03.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh 848504

rebasing locally.
Comment on attachment 751604 [details] [diff] [review]
v4

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

::: browser/devtools/debugger/debugger-panes.js
@@ -638,5 @@
> -      this.selectedItem = item;
> -    }
> -  },
> -
> -  /**

this is some nice delenda.

::: browser/devtools/debugger/debugger-view.js
@@ +358,2 @@
>        this.Sources.selectedValue = aUrl;
> +      this.Sources.node.preventFocusOnSelection = false;

well that's a thing you could do.
Attachment #751604 - Flags: review?(rcampbell) → review+
We're going to need some followups for this and the other netmonitor keyboard bug. Need to figure out our tabbing behavior from main panel to sidepanel.

Likely true in our other tools as well. Also, framework tabs. Sidepanel tabs...
(Assignee)

Comment 14

5 years ago
(In reply to Rob Campbell [:rc] (:robcee) from comment #13)
> We're going to need some followups for this and the other netmonitor
> keyboard bug. Need to figure out our tabbing behavior from main panel to
> sidepanel.
> 
> Likely true in our other tools as well. Also, framework tabs. Sidepanel
> tabs...

Bug 859040.
https://hg.mozilla.org/mozilla-central/rev/a31e88d74ce7
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 24
Depends on: 876111
Depends on: 876112
Depends on: 876113
(Assignee)

Updated

5 years ago
No longer depends on: 876111

Updated

8 days ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.