1203147 - Add a keyboard shortcut for the 'scroll into view' contextual menu action in the inspector

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Patrick Brosset <:pbro>]

STR:
- open the inspector
- click on a node that's not in the current viewport

Expected: It'd be great if from there, I could easily bring the node in the view, without either scrolling the page manually, or using the contextual menu and choosing 'scroll into view'.
This menu is nice, but it's 2 clicks, and I believe scrolling the element into view is frequent enough that a keyboard shortcut would be nice.

In fact, our old inspector used to auto-scroll into view as you'd hover nodes in it (or clicked?). It was really distracting + a lot of animations/markup mutations can happen on scroll, so we don't want this to be automatic. But it sure is something that a lot of people expect to be easy to do.

Comment 1

[Jon Droniak]

I want to try this, but have some questions:
What files need to be edited in order to set this up?
What do you suggest the specific key combination be for this command?
Thanks.

Comment 2

[Patrick Brosset <:pbro>]

(In reply to Jon Droniak from comment #1)
> I want to try this, but have some questions:
Hi Jon, thanks for your interest on this bug.
> What files need to be edited in order to set this up?
/browser/devtools/markupview/markup-view.js is the file you're looking for.
In particular, there's a keydown event handler defined in this file that we already use today for handling the "H" key (that hides/unhides the current element).
See: https://dxr.mozilla.org/mozilla-central/rev/9ed17db42e3e46f1c712e4dffd62d54e915e0fac/browser/devtools/markupview/markup-view.js#558
> What do you suggest the specific key combination be for this command?
I think it should just be "S", no key combination, so that it works like DELETE and H, etc.

Comment 3

[Patrick Brosset <:pbro>]

Oh and, I see you're new to bugzilla, so just in case you haven't done so yet, please go through our hacking guide: https://wiki.mozilla.org/DevTools/Hacking
And feel free to join #devtools on Mozilla's IRC server to ask any questions you might have.

Comment 4

[Jon Droniak]

Attachment: Bug 1203147 - Add a keyboard shortcut for scroll into view in the inspector.

It is working on my local machine. Let me know if there are any changes that need to be made.

Comment 5

[Patrick Brosset <:pbro>]

Thanks for that patch, I'll test this now.

Comment 6

[Patrick Brosset <:pbro>]

Comment on attachment 8662683 [details] [diff] [review]
Bug 1203147 - Add a keyboard shortcut for scroll into view in the inspector.

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

This looks great. I've tested this locally and it works fine.
3 comments though:
- You might have forgotten to add your username/email to your .hgrc configuration file because it's missing in the patch.
- Can you please rebase that patch on top of the latest version of the repository? I didn't apply cleanly for me, I had to merge it.
- Can you add ", r=pbro" at the end of the commit message?
Bug 1203147 - Add a keyboard shortcut for the 'scroll into view' contextual menu action in the inspector, r=pbro

Once done, please re-upload a new patch here and mark it as R+ (no need for me to re-review this).
In the meantime, I'll push the one I have locally to our TRY server infrastructure to make sure tests still pass:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb646ec88cf2

I don't think we need to add a new test for this. The scroll-into-view functionality is already well tested, this only adds an event handler.

Comment 7

[Jon Droniak]

Attachment: 2015-09-18_23-14-38_r263280+.diff

Ok, here is the final patch.

Comment 8

[Patrick Brosset <:pbro>]

Thanks. Try is green. Let's check this in!
Thanks for your contribution Jon. Feel free to ask on IRC #devtools or check out http://firefox-dev.tools if you want to find other similar bugs to work on.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d52326f55d2e

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for TEST-UNEXPECTED-FAIL | browser/devtools/markupview/test/browser_markupview_keybindings_02.js | Test timed out - expected PASS in https://hg.mozilla.org/integration/mozilla-inbound/rev/9a7520bca416

Comment 11

[Patrick Brosset <:pbro>]

I'm really surprised this test would fail after this change ... NI? myself to remember to take a look at this next week.

Comment 12

[Jon Droniak]

Yeah it is surprising because it worked for us both locally and it passed the try server. Perhaps we should try checking it in again?

Comment 13

[Patrick Brosset <:pbro>]

Attachment: Bug_1203147_-_Add_a_keyboard_shortcut_for_scrollin.diff

Rebased, fixed and pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=69d85b45db9e

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/4024fa5cb833

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4024fa5cb833

Comment 16

[Saurabh Nair [:jsx] (not reading bugmail. ni? if you need me)]

Updated:
https://developer.mozilla.org/en-US/docs/tools/Keyboard_shortcuts
https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to/Examine_and_edit_HTML
https://developer.mozilla.org/en-US/Firefox/Releases/44