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

RESOLVED FIXED in Firefox 14

Status

()

Firefox
Developer Tools: Inspector
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: paul, Assigned: paul)

Tracking

({dev-doc-complete})

Trunk
Firefox 14
x86
All
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 11 obsolete attachments)

13.47 KB, patch
dao
: review+
Details | Diff | Splinter Review
13.19 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
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).
(Assignee)

Comment 1

5 years ago
Created attachment 606505 [details] [diff] [review]
patch v1
(Assignee)

Updated

5 years ago
Attachment #606505 - Flags: review?(jwalker)
(Assignee)

Updated

5 years ago
Attachment #606505 - Flags: review?(dao)
(Assignee)

Updated

5 years ago
Blocks: 736418
(Assignee)

Updated

5 years ago
Assignee: nobody → paul
Status: NEW → ASSIGNED
(Assignee)

Updated

5 years ago
Blocks: 724509
Attachment #606505 - Flags: review?(jwalker) → review+
(Assignee)

Updated

5 years ago
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-
(Assignee)

Comment 3

5 years ago
Created attachment 607146 [details] [diff] [review]
patch v1.1
(Assignee)

Updated

5 years ago
Attachment #606505 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
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+
(Assignee)

Comment 5

5 years ago
Created attachment 607528 [details] [diff] [review]
patch v1.2

Thank you Dao.
(Assignee)

Updated

5 years ago
Attachment #607146 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
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]
(Assignee)

Comment 7

5 years ago
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…
(Assignee)

Comment 8

5 years ago
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.
(Assignee)

Comment 10

5 years ago
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.

Comment 11

5 years ago
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).
(Assignee)

Comment 13

5 years ago
Created attachment 611432 [details] [diff] [review]
patch v1.3

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)
(Assignee)

Updated

5 years ago
Attachment #607528 - Attachment is obsolete: true
(Assignee)

Comment 14

5 years ago
Created attachment 611461 [details] [diff] [review]
patch v1.3 - (Part 2 - Tilt)

Tilt needs to refocus the previously focused element
(Assignee)

Updated

5 years ago
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+
(Assignee)

Comment 16

5 years ago
(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?
(Assignee)

Comment 19

5 years ago
(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.
(Assignee)

Comment 21

5 years ago
(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.
(Assignee)

Comment 22

5 years ago
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.
(Assignee)

Comment 24

5 years ago
Created attachment 611781 [details] [diff] [review]
patch v1.3.1 - (Part 2 - Tilt)
(Assignee)

Updated

5 years ago
Attachment #611461 - Attachment is obsolete: true
Attachment #611461 - Flags: review?(vporof)
(Assignee)

Updated

5 years ago
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+
(Assignee)

Comment 26

5 years ago
Created attachment 611784 [details] [diff] [review]
patch v1.3.2 - (Part 2 - Tilt)

thanks Victor
(Assignee)

Updated

5 years ago
Attachment #611781 - Attachment is obsolete: true
(Assignee)

Comment 27

5 years ago
For the keyboard shortcut, we will use ctrl-enter. Patch coming…
(Assignee)

Comment 28

5 years ago
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?
(Assignee)

Comment 29

5 years ago
We could re-use ctrl-shift-i (as mentioned by @dcamp).
(Assignee)

Comment 30

5 years ago
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).
(Assignee)

Comment 31

5 years ago
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.
(Assignee)

Comment 33

5 years ago
Created attachment 611899 [details] [diff] [review]
patch v2 - (Part A - shortcut)

Switching from Enter to Ctrl-Shift-I
(Assignee)

Comment 34

5 years ago
Created attachment 611900 [details] [diff] [review]
patch v2 - (Part B - tabbable toolbar)

rebased + Tilt fix
(Assignee)

Updated

5 years ago
Attachment #611432 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #611784 - Attachment is obsolete: true
(Assignee)

Comment 35

5 years ago
Comment on attachment 611899 [details] [diff] [review]
patch v2 - (Part A - shortcut)

(Stephen agrees with this change)
Attachment #611899 - Flags: review?(dcamp)
(Assignee)

Comment 36

5 years ago
Created attachment 611907 [details] [diff] [review]
patch v2 - (Part B - tabbable toolbar)

forgot to remove the tabindex=0 from the inspect button
(Assignee)

Updated

5 years ago
Attachment #611900 - Attachment is obsolete: true
(Assignee)

Comment 37

5 years ago
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.
(Assignee)

Updated

5 years ago
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.
(Assignee)

Comment 39

5 years ago
Created attachment 612147 [details] [diff] [review]
patch v2.1 - (Part A - shortcut)

Addressed Dao's comments
(Assignee)

Updated

5 years ago
Attachment #611899 - Attachment is obsolete: true
Attachment #611899 - Flags: review?(dcamp)
Attachment #611899 - Flags: review?(dao)
(Assignee)

Updated

5 years ago
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+
(Assignee)

Comment 41

5 years ago
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).
(Assignee)

Comment 42

5 years ago
Created attachment 612152 [details] [diff] [review]
patch v2.1 - (Part B - tabbable toolbar)

rebased
(Assignee)

Updated

5 years ago
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 :)
(Assignee)

Updated

5 years ago
Whiteboard: [land-in-fx-team]
http://hg.mozilla.org/integration/fx-team/rev/0ba7e290a08f
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
Wrong changeset above, real ones:

http://hg.mozilla.org/integration/fx-team/rev/f95f46afce27
http://hg.mozilla.org/integration/fx-team/rev/01a50802f2d7
(Assignee)

Comment 50

5 years ago
Backedout: https://hg.mozilla.org/integration/fx-team/rev/0e71d6eec4d3

https://tbpl.mozilla.org/?tree=Fx-Team&rev=01a50802f2d7
https://tbpl.mozilla.org/php/getParsedLog.php?id=10976534&tree=Fx-Team
(Assignee)

Comment 51

5 years ago
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

Updated

5 years ago
Whiteboard: [fixed-in-fx-team]
(Assignee)

Comment 52

5 years ago
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)
(Assignee)

Comment 53

5 years ago
Created attachment 616356 [details] [diff] [review]
patch v2.2 - (Part B - tabbable toolbar)

fixed test
(Assignee)

Updated

5 years ago
Attachment #612152 - Attachment is obsolete: true
(Assignee)

Comment 54

5 years ago
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 14
Depends on: 747600
No longer depends on: 747600
Blocks: 747603

Updated

5 years ago
No longer blocks: 747603
Depends on: 747603

Updated

5 years ago
Depends on: 751815
(Assignee)

Comment 56

5 years ago
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

Updated

5 years ago
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
You need to log in before you can comment on or make changes to this bug.