Closed
Bug 848504
Opened 11 years ago
Closed 11 years ago
SideMenuWidget should be keyboard accessible
Categories
(DevTools :: Debugger, defect, P2)
DevTools
Debugger
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 24
People
(Reporter: vporof, Assigned: vporof)
References
Details
Attachments
(1 file, 3 obsolete files)
58.14 KB,
patch
|
rcampbell
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → vporof
Priority: -- → P2
Assignee | ||
Updated•11 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•11 years ago
|
||
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).
Comment 2•11 years ago
|
||
(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•11 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.
Comment 4•11 years ago
|
||
(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•11 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•11 years ago
|
||
Cleaned up things. Gave the debugger some love too.
Attachment #751443 -
Attachment is obsolete: true
Assignee | ||
Comment 7•11 years ago
|
||
Made existing tests pass again.
Attachment #751463 -
Attachment is obsolete: true
Assignee | ||
Comment 8•11 years ago
|
||
Added tests. Woo!
Attachment #751501 -
Attachment is obsolete: true
Attachment #751604 -
Flags: review?(rcampbell)
Assignee | ||
Comment 9•11 years ago
|
||
Going through try: https://tbpl.mozilla.org/?tree=Try&rev=4749720a1e96
Comment 10•11 years ago
|
||
So I assume that bug 859271 is also getting fixed in this one ?
Comment 11•11 years ago
|
||
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 12•11 years ago
|
||
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+
Comment 13•11 years ago
|
||
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•11 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.
Assignee | ||
Comment 15•11 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/a31e88d74ce7
Whiteboard: [fixed-in-fx-team]
Comment 16•11 years ago
|
||
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
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•