1260235 - cmd+f doesn't focus the inspector search box on osx

Status: VERIFIED FIXED

(DevTools :: Inspector, defect, P1)

Description

[Brian Grinstead [:bgrins]]

STR:

Open inspector
Press cmd+f

Expected:

Search box is focused

Actual:

It's not focused

Comment 1

[Brian Grinstead [:bgrins]]

Seems to work for me in beta but not Dev Edition

Comment 2

[Brian Grinstead [:bgrins]]

As far as I can tell: Bug 1238133 made cmd+f start to focus the rule view only.  And then (somehow) the CSS change in Bug 1243598 seemed to make neither get focused.  At least, that looks like the only relevant change in https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=709f559b5406e8555cf84dd09bdc747b076f142c&tochange=3b35f0a98eba9f1c9be7d793650b3d5bec6c8fdb.

So, we need to be able to handle multiple instances of ctrl/cmd+f inside of a single frame.  I think what's actually happening is that the computed view's search box is getting focused when cmd+f is pressed.

Comment 3

[Brian Grinstead [:bgrins]]

Might be an issue with call to preventDefault() here: https://dxr.mozilla.org/mozilla-central/source/devtools/client/inspector/computed/computed.js#509

Comment 4

[Brian Grinstead [:bgrins]]

Attachment: cmd-key-shortcut-WIP.patch

WIP - testing this is proving hard since events sent from EventUtils.synthesizeKey doesn't seem to behave the same way as ones sent normally (i.e. event.code and event.keyCode aren't set).

Comment 5

[Brian Grinstead [:bgrins]]

Attachment: MozReview Request: Bug 1260235 - Only accept ctrl+f in computed and rule view if it happened in the relevant container;r=gl

Review commit: https://reviewboard.mozilla.org/r/42899/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/42899/

Comment 6

[Brian Grinstead [:bgrins]]

I don't love the solution, especially since it's hard to focus elements in the rule view (at least on osx if I click on various spots in the rule view I can make cmd+f focus the inspector search).   But it's much better than the current behavior IMO.

This might be a real reason to push some of this stuff back into iframes but keep a lot of the refactoring done originally to de-frame it (maybe one frame for all sidebar tabs now that we have a layout fix in Bug 1243598).

Anyway, try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dcac194ff650

Comment 7

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8735660 [details]
MozReview Request: Bug 1260235 - Only accept ctrl+f in computed and rule view if it happened in the relevant container;r=gl

https://reviewboard.mozilla.org/r/42899/#review39593

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/2b4d5580c46d

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2b4d5580c46d

Comment 10

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Brian, this seems to be a recent regression in Fx47. Should we consider uplifting this to Beta47?

Comment 11

[Brian Grinstead [:bgrins]]

(In reply to Ritu Kothari (:ritu) from comment #10)
> Hi Brian, this seems to be a recent regression in Fx47. Should we consider
> uplifting this to Beta47?

Sure, seems simple enough - I'll file a request

Comment 12

[Brian Grinstead [:bgrins]]

Comment on attachment 8735660 [details]
MozReview Request: Bug 1260235 - Only accept ctrl+f in computed and rule view if it happened in the relevant container;r=gl

Approval Request Comment
[Feature/regressing bug #]: 1243598
[User impact if declined]: Ctrl/Cmd+F in the devtools inspector won't focus the search box
[Describe test coverage new/current, TreeHerder]: Regression test added for this
[Risks and why]: This is dealing with keyboard handling.  The worst case is that it would break keyboard shortcuts in the inspector panel.  But it's not doing much - basically early returning in a couple of places so that the 'side panels' don't take over key events when they shouldn't: https://hg.mozilla.org/mozilla-central/rev/2b4d5580c46d#l1.12 and https://hg.mozilla.org/mozilla-central/rev/2b4d5580c46d#l3.12
[String/UUID change made/needed]: none

Comment 13

[Brian Grinstead [:bgrins]]

I've done manual testing on a beta build and this applied cleanly / fixes the problem

Comment 14

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Florin, I am planning to take this fix in 47.0b2. Could you please do some focused testing on keyboard shortcuts in case there are any unexpected regressions from this change? Thanks!

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8735660 [details]
MozReview Request: Bug 1260235 - Only accept ctrl+f in computed and rule view if it happened in the relevant container;r=gl

Recent P1 regression, Beta47+

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/ddef17d3f117

Comment 17

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

(In reply to Ritu Kothari (:ritu) from comment #14)
> Hi Florin, I am planning to take this fix in 47.0b2. Could you please do
> some focused testing on keyboard shortcuts in case there are any unexpected
> regressions from this change? Thanks!

Setting ni? for Andrei Vaida. This was included in the 47 Beta 2 sign-offs (https://goo.gl/jhXbqn). Will follow up with results once done.

Comment 18

[Cornel Ionce [:noni] [Hubs QA]]

Verified this issue as fixed on Windows XP x86, Windows 8.1 x64, Mac OS X 10.10.4 and Ubuntu 13.04 x86 using Fx 47 beta 2.
No new issues were found while performing exploratory testing around the keyboard shortcuts.

Comment 19

[Alexandru Simonca [:asimonca], Release Desktop QA]

Verified as fixed on Windows XP x64, Windows 10 x64, Mac OS X 10.11.1 and Ubuntu 14.04 x64 using Fx 48 beta 2 (20160620091522).