Closed Bug 848504 Opened 11 years ago Closed 11 years ago

SideMenuWidget should be keyboard accessible

Categories

(DevTools :: Debugger, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 24

People

(Reporter: vporof, Assigned: vporof)

References

Details

Attachments

(1 file, 3 obsolete files)

      No description provided.
Assignee: nobody → vporof
Priority: -- → P2
Status: NEW → ASSIGNED
Attached patch v1 (obsolete) — Splinter Review
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).
(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.
(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.
Attached patch v2 (obsolete) — Splinter Review
Cleaned up things. Gave the debugger some love too.
Attachment #751443 - Attachment is obsolete: true
Attached patch v3 (obsolete) — Splinter Review
Made existing tests pass again.
Attachment #751463 - Attachment is obsolete: true
Attached patch v4Splinter Review
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...
(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
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 24
Depends on: 876111
Depends on: 876112
Depends on: 876113
No longer depends on: 876111
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: