Closed Bug 735214 Opened 13 years ago Closed 13 years ago

[inspector] Give the focus to the toolbar and make the buttons tabbable

Categories

(DevTools :: Inspector, defect)

Product:
DevTools
Component:
Inspector
Platform:
x86
All
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Firefox 14

People

(Reporter: paul, Assigned: paul)

References

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 11 obsolete files)

patch v2.1 - (Part A - shortcut)
13.47 KB, patch
dao
: review+
Details | Diff | Splinter Review
patch v2.2 - (Part B - tabbable toolbar)
13.19 KB, patch
Details | Diff | Splinter Review
It is not possible to got through the different buttons of the inspector toolbar with the tab key. This is also true for the infobar buttons (bug 717916).
Attached patch patch v1 (obsolete) — — Splinter Review
Attachment #606505 - Flags: review?(jwalker)
Attachment #606505 - Flags: review?(dao)
Blocks: 736418
Assignee: nobody → paul
Status: NEW → ASSIGNED
Blocks: 724509
Attachment #606505 - Flags: review?(jwalker) → review+
Blocks: 717922
Comment on attachment 606505 [details] [diff] [review] patch v1 > <toolbarbutton id="inspector-inspect-toolbutton" > class="devtools-toolbarbutton" >+ tabindex="0" > command="Inspector:Inspect"/> This button shouldn't be focusable, as per bug 736418. >+ // Focus the first focusable element in the toolbar >+ this.toolbar.querySelector("*[tabindex]").focus(); Can you use this.toolbar.ownerDocument.commandDispatcher.advanceFocusIntoSubtree?
Attachment #606505 - Flags: review?(dao) → review-
Attached patch patch v1.1 (obsolete) — — Splinter Review
Attachment #606505 - Attachment is obsolete: true
Attachment #607146 - Flags: review?(dao)
Comment on attachment 607146 [details] [diff] [review] patch v1.1 use :-moz-focusring instead of :focus throughout this patch
Attachment #607146 - Flags: review?(dao) → review+
Attached patch patch v1.2 (obsolete) — — Splinter Review
Thank you Dao.
Attachment #607146 - Attachment is obsolete: true
Whiteboard: [land-in-fx-team]
This patch seems to break mochitests on Mac: mochitest-browser-chrome failed: TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/highlighter/test/browser_inspector_bug_672902_keyboard_shortcuts.js | Test timed out TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/highlighter/test/browser_inspector_bug_672902_keyboard_shortcuts.js | Found a tab after previous test timed out: data:text/html,<html><head><title>Test%20for%20the%20highlighter%20keybindings</title></head><body><h1>Hello</h1><p><strong>Greetings,%20earthlings!</strong>%20I%20come%20in%20peace.</body></html> TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/highlighter/test/browser_inspector_keybindings.js | selection matches node - Got null, expected [object HTMLHeadingElement @ 0x1093b0580 (native @ 0x1094f3200)] TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/highlighter/test/browser_inspector_keybindings.js | the node is unlocked TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/highlighter/test/browser_inspector_keybindings.js | Test timed out TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/highlighter/test/browser_inspector_keybindings.js | Found a tab after previous test timed out: data:text/html,<h1>foobar</h1>
Whiteboard: [land-in-fx-team]
The problem here is that we focus the toolbar when we open the inspector. So when we press "Enter", the focused button is activated (HTML panel), instead of the Toggle Inspection shortcut. Not sure what to do yet…
Different options: 1) we preventDefault() the keypress event on the button, and toggle the inspection. This might break some accessibility expectations. 2) we focus the highlighter by default when we start the inspection. But then, it's hard to reach the toolbar with tab. 3) we focus the Inspect button when we start the inspection. The Enter key will toggle the button, and provide the same effect as if the highlighter were focused.
4) Change the inspect shortcut key.
This is an important shortcut. I don't think we want to change it. 3) would be ideal I think. The only problem I see is that we would make the Inspect button focusable, and that might be a problem for bug 736418.
Paul asked me on irc to comment. Even if the inspect button is supposed to enable/disable a feature for mouse users then it might be handy to allow users to reach the button from keyboard. Honestly I just don't see a good reason why it should be different from other buttons. Anyway, if somebody says that removing the inspect button from tab order is friendly for screen readers users because it's harder for them to locate it then I'm fine with that. But in either case it's worth to provide a keyboard accelerator.
(In reply to alexander :surkov from comment #11) > Even if the inspect button is supposed to enable/disable a feature for mouse > users then it might be handy to allow users to reach the button from > keyboard. It has a shortcut key (Enter).
Attached patch patch v1.3 (obsolete) — — Splinter Review
Making the button focusable let us conserve the Enter key shortcut, and the toolbar stays focused (and then, all the other buttons are easily reachable). I consider this as the best compromise. If we decide (bug 736418) to not make the Inspect button focusable and tabbable, we will need to find a better solution. (not asking for a review yet)
Attachment #607528 - Attachment is obsolete: true
Attached patch patch v1.3 - (Part 2 - Tilt) (obsolete) — — Splinter Review
Tilt needs to refocus the previously focused element
Attachment #611461 - Flags: review?(rcampbell)
Comment on attachment 611461 [details] [diff] [review] patch v1.3 - (Part 2 - Tilt) I think this looks OK. Victor? Also, this'll need a test.
Attachment #611461 - Flags: review?(vporof)
Attachment #611461 - Flags: review?(rcampbell)
Attachment #611461 - Flags: review+
(In reply to Rob Campbell [:rc] (:robcee) from comment #15) > Comment on attachment 611461 [details] [diff] [review] > patch v1.3 - (Part 2 - Tilt) > > I think this looks OK. Victor? > > Also, this'll need a test. This is already covered by a test. And I realize it fails (damn, I was pretty sure I fixed it).
(In reply to Paul Rouget [:paul] from comment #16) > (In reply to Rob Campbell [:rc] (:robcee) from comment #15) > > Comment on attachment 611461 [details] [diff] [review] > > patch v1.3 - (Part 2 - Tilt) > > > > I think this looks OK. Victor? > > > > Also, this'll need a test. > > This is already covered by a test. > And I realize it fails (damn, I was pretty sure I fixed it). as long as there's a test. :)
(In reply to Rob Campbell [:rc] (:robcee) from comment #17) > (In reply to Paul Rouget [:paul] from comment #16) > > (In reply to Rob Campbell [:rc] (:robcee) from comment #15) > > > Comment on attachment 611461 [details] [diff] [review] > > > patch v1.3 - (Part 2 - Tilt) > > > > > > I think this looks OK. Victor? > > > > > > Also, this'll need a test. > > > > This is already covered by a test. > > And I realize it fails (damn, I was pretty sure I fixed it). > > as long as there's a test. :) There's a test. It's the test that fails. (In reply to Paul Rouget [:paul] from comment #16) What happens? Is it the same fail as before? + let toFocus = this.visualizers[aId].originalActiveElement; + if (toFocus) + toFocus.focus(); + else + this.chromeWindow.gBrowser.selectedBrowser.focus(); Do we need an else here? Are there cases in which we can't have an originalActiveElement?
(In reply to Victor Porof from comment #18) > (In reply to Rob Campbell [:rc] (:robcee) from comment #17) > > (In reply to Paul Rouget [:paul] from comment #16) > > > (In reply to Rob Campbell [:rc] (:robcee) from comment #15) > > > > Comment on attachment 611461 [details] [diff] [review] > > > > patch v1.3 - (Part 2 - Tilt) > > > > > > > > I think this looks OK. Victor? > > > > > > > > Also, this'll need a test. > > > > > > This is already covered by a test. > > > And I realize it fails (damn, I was pretty sure I fixed it). > > > > as long as there's a test. :) > > There's a test. It's the test that fails. > > > (In reply to Paul Rouget [:paul] from comment #16) > > What happens? Is it the same fail as before? > > + let toFocus = this.visualizers[aId].originalActiveElement; > + if (toFocus) > + toFocus.focus(); > + else > + this.chromeWindow.gBrowser.selectedBrowser.focus(); > > Do we need an else here? Are there cases in which we can't have an > originalActiveElement? You don't know what got the focus in the first place (a breadcrumb button, or any element in Firefox). The element might be deleted.
(In reply to Paul Rouget [:paul] from comment #19) > You don't know what got the focus in the first place (a breadcrumb button, > or any element in Firefox). The element might be deleted. Maybe I'm misreading this, but: if we save an originalActiveElement and that element is deleted, then the reference still exists in this.visualizers[aId], regardless if it's nowhere else. It won't be deleted in there too. So the "if (toFocus)" check probably doesn't do what it should. Do we *really* want to give focus back to that exact original element in the first place? Originally, the whole point of Tilt giving focus back to the selectedBrowser was to make sure keyboard events still fire and are caught by the Inspector.
(In reply to Victor Porof from comment #20) > (In reply to Paul Rouget [:paul] from comment #19) > > You don't know what got the focus in the first place (a breadcrumb button, > > or any element in Firefox). The element might be deleted. > > Maybe I'm misreading this, but: if we save an originalActiveElement and that > element is deleted, then the reference still exists in > this.visualizers[aId], regardless if it's nowhere else. It won't be deleted > in there too. So the "if (toFocus)" check probably doesn't do what it should. You're right. We need a weak reference here, or need to check if the element is still in the DOM tree. > Do we *really* want to give focus back to that exact original element in the > first place? Originally, the whole point of Tilt giving focus back to the > selectedBrowser was to make sure keyboard events still fire and are caught > by the Inspector. I don't know. I thought it made sense. Giving back the focus to the selectedBrowser is easier though (because I am struggling with the test right now). You tell me.
So I can just throw these changes and fix the test to just test if the window is focused (not the previously focused element).
(In reply to Paul Rouget [:paul] from comment #22) > So I can just throw these changes and fix the test to just test if the > window is focused (not the previously focused element). That would make more sense.
Attached patch patch v1.3.1 - (Part 2 - Tilt) (obsolete) — — Splinter Review
Attachment #611461 - Attachment is obsolete: true
Attachment #611461 - Flags: review?(vporof)
Attachment #611781 - Flags: review?(vporof)
Comment on attachment 611781 [details] [diff] [review] patch v1.3.1 - (Part 2 - Tilt) Review of attachment 611781 [details] [diff] [review]: ----------------------------------------------------------------- Change "The focus wasn't correctly given back to the initial element." to "The focus wasn't correctly given back to the selectedBrowser"
Attachment #611781 - Flags: review?(vporof) → review+
Attached patch patch v1.3.2 - (Part 2 - Tilt) (obsolete) — — Splinter Review
thanks Victor
Attachment #611781 - Attachment is obsolete: true
For the keyboard shortcut, we will use ctrl-enter. Patch coming…
So Ctrl-Enter has the same problem (it opens the HTML Panel). Should I preventDefault() these keypress events when caught by the toolbar's buttons?
We could re-use ctrl-shift-i (as mentioned by @dcamp).
Today, we open and close the inspector with Ctrl-shift-i. We toggle the Inspection (lock/unlock) with Enter. We could use Ctrl-shift-i to open the inspector AND toggle the Inspection. We can always use Escape to close the inspector. This will solve our current problem (we can't use Enter for accessibility reasons).
Dao, if we do so, do you think we should append the Keyboard shortcut to the tooltip? (the shortcut is already present in the menuitem)
Yeah, I'd still add it to the tooltip. The redundancy doesn't hurt.
Attached patch patch v2 - (Part A - shortcut) (obsolete) — — Splinter Review
Switching from Enter to Ctrl-Shift-I
rebased + Tilt fix
Attachment #611432 - Attachment is obsolete: true
Attachment #611784 - Attachment is obsolete: true
Comment on attachment 611899 [details] [diff] [review] patch v2 - (Part A - shortcut) (Stephen agrees with this change)
Attachment #611899 - Flags: review?(dcamp)
forgot to remove the tabindex=0 from the inspect button
Attachment #611900 - Attachment is obsolete: true
Comment on attachment 611907 [details] [diff] [review] patch v2 - (Part B - tabbable toolbar) This patch is basically what we got before the tests failures report, with the tests update.
Attachment #611899 - Flags: review?(dao)
Comment on attachment 611899 [details] [diff] [review] patch v2 - (Part A - shortcut) >+ let modifiers = []; >+ if (modifiersAttr.match("accel")) { >+ if (Services.appinfo.OS == "Darwin") { >+ modifiers.push(keysbundle.GetStringFromName("VK_META")); >+ } else { >+ modifiers.push(keysbundle.GetStringFromName("VK_CONTROL")); >+ } >+ } You should change this jsm to be preprocessed and use ifdef here. >+ if (modifiersAttr.match("shift")) >+ modifiers.push(keysbundle.GetStringFromName("VK_SHIFT")); >+ if (modifiersAttr.match("alt")) >+ modifiers.push(keysbundle.GetStringFromName("VK_ALT")); >+ if (modifiersAttr.match("ctrl")) >+ modifiers.push(keysbundle.GetStringFromName("VK_CONTROL")); >+ if (modifiersAttr.match("meta")) >+ modifiers.push(keysbundle.GetStringFromName("VK_META")); >+ >+ modifiers.push(key.getAttribute("key")); Rename "modifiers" to "combo" or something like this. >+ let key = buildKeyFromKeyTag(document.getElementById("key_inspect")); >+ EventUtils.synthesizeKey(key.name, key.modifiers); >+ let key = buildKeyFromKeyTag(document.getElementById("key_inspect")); >+ EventUtils.synthesizeKey(key.name, key.modifiers); >+ let key = buildKeyFromKeyTag(document.getElementById("key_inspect")); >+ EventUtils.synthesizeKey(key.name, key.modifiers); Looks like you could simplify this by letting the helper function (1) take an id instead of an element and (2) automatically call EventUtils.synthesizeKey.
Addressed Dao's comments
Attachment #611899 - Attachment is obsolete: true
Attachment #611899 - Flags: review?(dcamp)
Attachment #611899 - Flags: review?(dao)
Attachment #612147 - Flags: review?(dao)
Comment on attachment 612147 [details] [diff] [review] patch v2.1 - (Part A - shortcut) >+ if (modifiersAttr.match("accel")) >+#ifdef XP_MACOSX >+ combo.push(keysbundle.GetStringFromName("VK_META")); >+#else >+ combo.push(keysbundle.GetStringFromName("VK_CONTROL")); >+#endif >+ if (modifiersAttr.match("shift")) >+ combo.push(keysbundle.GetStringFromName("VK_SHIFT")); >+ if (modifiersAttr.match("alt")) >+ combo.push(keysbundle.GetStringFromName("VK_ALT")); >+ if (modifiersAttr.match("ctrl")) >+ combo.push(keysbundle.GetStringFromName("VK_CONTROL")); >+ if (modifiersAttr.match("meta")) >+ combo.push(keysbundle.GetStringFromName("VK_META")); Alt and Shift should probably precede accel.
Attachment #612147 - Flags: review?(dao) → review+
Thanks for the r+. (In reply to Dão Gottwald [:dao] from comment #40) > Comment on attachment 612147 [details] [diff] [review] > patch v2.1 - (Part A - shortcut) > > >+ if (modifiersAttr.match("accel")) > >+#ifdef XP_MACOSX > >+ combo.push(keysbundle.GetStringFromName("VK_META")); > >+#else > >+ combo.push(keysbundle.GetStringFromName("VK_CONTROL")); > >+#endif > >+ if (modifiersAttr.match("shift")) > >+ combo.push(keysbundle.GetStringFromName("VK_SHIFT")); > >+ if (modifiersAttr.match("alt")) > >+ combo.push(keysbundle.GetStringFromName("VK_ALT")); > >+ if (modifiersAttr.match("ctrl")) > >+ combo.push(keysbundle.GetStringFromName("VK_CONTROL")); > >+ if (modifiersAttr.match("meta")) > >+ combo.push(keysbundle.GetStringFromName("VK_META")); > > Alt and Shift should probably precede accel. I don't think so, because we would get "Shift-Ctrl+I" as a shortcut name (in the menu, I see Ctrl-Shift-I).
rebased
Attachment #611907 - Attachment is obsolete: true
(In reply to Victor Porof from comment #20) > (In reply to Paul Rouget [:paul] from comment #19) > > You don't know what got the focus in the first place (a breadcrumb button, > > or any element in Firefox). The element might be deleted. > > Maybe I'm misreading this, but: if we save an originalActiveElement and that > element is deleted, then the reference still exists in > this.visualizers[aId], regardless if it's nowhere else. It won't be deleted > in there too. So the "if (toFocus)" check probably doesn't do what it should. > > Do we *really* want to give focus back to that exact original element in the > first place? Originally, the whole point of Tilt giving focus back to the > selectedBrowser was to make sure keyboard events still fire and are caught > by the Inspector. this is old news by now, but in hindsight, if we were entering/exiting tilt via the 3D button, wouldn't that automatically have focus?
(In reply to Rob Campbell [:rc] (:robcee) from comment #43) > (In reply to Victor Porof from comment #20) > > (In reply to Paul Rouget [:paul] from comment #19) > > > You don't know what got the focus in the first place (a breadcrumb button, > > > or any element in Firefox). The element might be deleted. > > > > Maybe I'm misreading this, but: if we save an originalActiveElement and that > > element is deleted, then the reference still exists in > > this.visualizers[aId], regardless if it's nowhere else. It won't be deleted > > in there too. So the "if (toFocus)" check probably doesn't do what it should. > > > > Do we *really* want to give focus back to that exact original element in the > > first place? Originally, the whole point of Tilt giving focus back to the > > selectedBrowser was to make sure keyboard events still fire and are caught > > by the Inspector. > > this is old news by now, but in hindsight, if we were entering/exiting tilt > via the 3D button, wouldn't that automatically have focus? No, we're giving focus to the canvas when starting tilt. We don't have a canvas after closing it.
Comment on attachment 612152 [details] [diff] [review] patch v2.1 - (Part B - tabbable toolbar) this looks good, I think. Dao will probably want to verify the focusring style.
Attachment #612152 - Flags: review+
(In reply to Victor Porof from comment #44) > (In reply to Rob Campbell [:rc] (:robcee) from comment #43) > > this is old news by now, but in hindsight, if we were entering/exiting tilt > > via the 3D button, wouldn't that automatically have focus? > > No, we're giving focus to the canvas when starting tilt. We don't have a > canvas after closing it. yes, but if the user's clicking or otherwise activating the 3D View button on the toolbar when closing tilt, shouldn't that have focus? Also, when activating tilt, if it's via the toolbar button, shouldn't that have focus before giving focus to the tilt canvas? It's a moot point anyway now.
(In reply to Rob Campbell [:rc] (:robcee) from comment #46) > (In reply to Victor Porof from comment #44) > > (In reply to Rob Campbell [:rc] (:robcee) from comment #43) > > > this is old news by now, but in hindsight, if we were entering/exiting tilt > > > via the 3D button, wouldn't that automatically have focus? > > > > No, we're giving focus to the canvas when starting tilt. We don't have a > > canvas after closing it. > > yes, but if the user's clicking or otherwise activating the 3D View button > on the toolbar when closing tilt, shouldn't that have focus? > > Also, when activating tilt, if it's via the toolbar button, shouldn't that > have focus before giving focus to the tilt canvas? > > It's a moot point anyway now. Via toolbar buttons yes, should work fine. But I always close tilt via ESC :)
Whiteboard: [land-in-fx-team]
http://hg.mozilla.org/integration/fx-team/rev/0ba7e290a08f
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
TEST-START | chrome://mochitests/content/browser/browser/devtools/highlighter/test/browser_inspector_bug_672902_keyboard_shortcuts.js TEST-INFO | chrome://mochitests/content/browser/browser/devtools/highlighter/test/browser_inspector_bug_672902_keyboard_shortcuts.js | Console message: [JavaScript Error: "The character encoding of the HTML document was not declared. The document will render with garbled text in some browser configurations if the document contains characters from outside the US-ASCII range. The character encoding of the page must to be declared in the document or in the transfer protocol." {file: "data:text/html,<html><head><title>Test%20for%20the%20highlighter%20keybindings</title></head><body><h1>Hello</h1><p><strong>Greetings,%20earthlings!</strong>%20I%20come%20in%20peace.</body></html>" line: 0}] TEST-INFO | chrome://mochitests/content/browser/browser/devtools/highlighter/test/browser_inspector_bug_672902_keyboard_shortcuts.js | Console message: [JavaScript Warning: "XUL box for hbox element contained an inline span child, forcing all its children to be wrapped in a block." {file: "resource:///modules/highlighter.jsm" line: 537}] TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/highlighter/test/browser_inspector_bug_672902_keyboard_shortcuts.js | Test timed out args: ['arch', '-arch', 'i386', '/usr/sbin/screencapture', '-C', '-x', '-t', 'png', '/var/folders/Xr/Xr--yJnSEY0U11ET5NZuMU+++TM/-Tmp-/mozilla-test-fail_4NJngx'] SCREENSHOT: XXXX INFO TEST-END | chrome://mochitests/content/browser/browser/devtools/highlighter/test/browser_inspector_bug_672902_keyboard_shortcuts.js | finished in 30028ms TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/highlighter/test/browser_inspector_bug_672902_keyboard_shortcuts.js | Found a tab after previous test timed out: data:text/html,<html><head><title>Test%20for%20the%20highlighter%20keybindings</title></head><body><h1>Hello</h1><p><strong>Greetings,%20earthlings!</strong>%20I%20come%20in%20peace.</body></html> TEST-INFO | checking window state
Whiteboard: [fixed-in-fx-team]
So on Mac, if the "Full Keyboard Access" option is not activated (by default), only inputs are tabbables. That's why the test failed. An extra test to make sure the toolbar is focused is necessary. If not, we focus the content. (patch coming)
fixed test
Attachment #612152 - Attachment is obsolete: true
Verified with a try. Landed: https://hg.mozilla.org/integration/fx-team/rev/134706dc07b1
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/134706dc07b1
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 14
No longer depends on: 747600
No longer blocks: 747603
Depends on: 747603
Depends on: 751815
I'm not sure, but we might need to update some doc somewhere. Before: - ctrl-shift-I was toggling (open & close) the inspector. - Escape was closing the inspector. - Enter was toggling the inspect mode (inspection on while hover the page or not) Now: - ctrl-shift-I opens the inspector if not open. - Escape closes the inspector. - ctrl-shift-I toggles the inspect mode (inspection on while hover the page or not)
No longer blocks: 717922, 724509, 736418
No longer depends on: 747603, 751815
Keywords: dev-doc-needed
Blocks: 717922, 724509, 736418
Depends on: 747603, 751815
As far as I know Inspector keyboard shortcuts are documented: https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector#Keyboard_shortcuts, so I'm marking this dev-doc-complete.
Keywords: dev-doc-needed → dev-doc-complete
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: