1327121 - Escape key to open console doesn't work in search field in inspector

Status: VERIFIED FIXED

(DevTools :: Inspector, defect, P3)

Description

[arni2033]

>>>   My Info:   Win7_64, Nightly 50, 32bit, ID 20160714030208 (2016-07-14)
STR_1:
1. Open inspector
2. Focus search field "Search HTML"
3. Press Escape key

AR:  No visible action
ER:  Console should open on the bottom side of devtools

This is regression from bug 1266456. Regression range:
> https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=679118259e91f40d4a8f968f03ec4cff066cdb5b&tochange=1de829f2f1f03e23ca0159bee473d36b9989e62b

Comment 1

[Julian Descottes [:jdescottes]]

I think it can be useful for users to open the split console after doing a HTML search, so we should make sure ESCAPE opens the split console.

We listen to keypress events in inspector-search.js, the callback is _onSearchKeypress.

In case the key is ESCAPE [1] _and_ if the autocomplete popup is already hidden, we should directly emit "processing-done" and return, to avoid going through preventDefault/stopPropagation at the end of the method. Same as what is done for the TAB key.

Adding a test would also be nice here. 

[1]

Comment 2

[Tim Nguyen :ntim]

The relevant function is here: https://dxr.mozilla.org/mozilla-central/source/devtools/client/inspector/inspector-search.js#331

Comment 3

[andrewxia2]

Could I grab this as my good first bug?

Comment 4

[Julian Descottes [:jdescottes]]

Thanks for offering to work on this! Assigning the bug to you.

Take a look to the following guidelines to help you setup your dev-environment:
- https://developer.mozilla.org/en-US/docs/Tools/Contributing
(there is also https://wiki.mozilla.org/DevTools/Hacking if you prefer to use mercurial over git)

First of all make sure you can build Firefox, make a change in devtools and see this change reflected locally so that you can test it. Then there are some hints about the solution in comment 1 and ntim added a link to the exact code I was referring to in comment 2.
I also mentioned adding a new test to cover this, but let's first focus on fixing the issue.

If you have any question or issue, please ask here or on IRC (channel #devtools).
When asking a question here, make sure to use the "Need more information from" form right below. This way the person you ping will get a notification, and you're more likely to get a fast answer!

Comment 5

[andrewxia2]

Hey there, an update and a quick question:

I've finished setting up a development environment and was able to successfully build (from nightly on GitHub). I also found the function you were referring to and added code to emit "processing-done" when the escape key is pressed and popup.isOpen is false. The current behavior after escape key is pressed is in the attached screenshot. 

I'm just wondering what the split console is and how do I go about opening it?

Image: https://gyazo.com/040ae30b7986dd80beb7250a1b2c1a2a

Comment 6

[Julian Descottes [:jdescottes]]

Based on your screenshot, this looks good! 

The "split console" is the way we call the Webconsole tool when it's displayed in "split screen" mode, below the current tool. So in your screenshot, it is already opened: that's the lower half of the devtools. 

Normally the webconsole can be displayed in split screen mode together with any tool, when you press on Escape. If you press Escape again, it should disappear.

Feel free to attach your diff if you want me to look at the code!

Comment 7

[andrewxia2]

Attachment: Bug1327121.git

Hey there, please see attached for patch file. I'm using git and followed the contributing guide to generate it in the mercurial format. Please let me know if there's anything I need to change.

Comment 8

[Tim Nguyen :ntim]

Comment on attachment 8839632 [details] [diff] [review]
Bug1327121.git

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

::: devtools/client/inspector/inspector-search.js
@@ +371,5 @@
>  
>        case KeyCodes.DOM_VK_ESCAPE:
>          if (popup.isOpen) {
>            this.hidePopup();
> +        } else if (!popup.isOpen) {

`else {` is enough in this case.

Comment 9

[Julian Descottes [:jdescottes]]

Comment on attachment 8839632 [details] [diff] [review]
Bug1327121.git

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

Thanks for the patch, it seems to fix the issue!

Please address the comment from ntim and fix the changeset message (replace "selectedb" by "selected")

A small note on the flags:
- when asking for a review, you should use review "?". 
- review+ and feedback+ are set as answers to review? and feedback? (here I am setting review+ for instance)
- don't use checkin+ on an attachment that has not been reviewed

Thanks!

Comment 10

[andrewxia2]

Attachment: Bug1327121.patch

Hey there,

I've amended the changes. Apologies for the flags, I was a little confused. Let me know if any changes need to be made. Thanks!

Comment 11

[Julian Descottes [:jdescottes]]

Comment on attachment 8840585 [details] [diff] [review]
Bug1327121.patch

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

Great, thank you!

I pushed your changeset to try, which is our continuous integration platform.
The devtools tests will run on a various set of platforms to see if your change didn't introduce unexpected issues.

The results will be displayed on the following page: https://treeherder.mozilla.org/#/jobs?repo=try&revision=93307622b7e85d193c5c70336571a68b2118efef
(can take a few hours for the slowest platforms).

We expect most of the test suites to appear "green", but some of our tests can intermittently fail. 
This means that if you see "orange" test suites in the result page, it doesn't necessarily mean it was introduced by your change.
We would just need to check if it corresponds to a known intermittent and/or if it could be related to the code touched by the changeset.

After the tests pass, we will add the keyword checkin-needed to this bug so that a sheriff can pick your patch and land it on an integration branch.

Comment 12

[Julian Descottes [:jdescottes]]

(needinfo on myself to check the try run results and update the flags if needed)

Comment 13

[Julian Descottes [:jdescottes]]

Try is green, adding checkin-needed.

andrewxia: your patch will soon land on an integration branch and will then be released with Firefox 54.
Thanks for fixing this!

Comment 14

[andrewxia2]

Great! Thank you!

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/99239977e6c3
open split console on esc press when inspector search selected r=jdescottes

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/99239977e6c3

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Too late for 52 at this point, but maybe worth an uplift request to Aurora for 53 still. Can you please help shepherd that through?

Comment 18

[Julian Descottes [:jdescottes]]

Comment on attachment 8840585 [details] [diff] [review]
Bug1327121.patch

Approval Request Comment
[Feature/Bug causing the regression]:1266456
[User impact if declined]: devtools users can not easily open the console when using the HTML search input of the Inspector panel (ux improvement)
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: yes
- go to any page
- open devtools
- open inspector
- focus the HTML search input
- press ESCAPE
-> the webconsole should open

[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: devtools only, minor js change
[String changes made/needed]:none

Comment 19

[Mohammad Maruf Rahman]

I have reproduced this bug with Nightly  50.0a1 (2016-07-14) on WIndows 7 , 64 Bit!

This bug's fix is verified with latest Nightly!

Build   ID : 20170301030202
User Agent : Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:54.0) Gecko/20100101 Firefox/54.0

[bugday-20170301]

Comment 20

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8840585 [details] [diff] [review]
Bug1327121.patch

Nice ux improvement and regression fix, let's uplift to aurora.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ff92ddc47071

Comment 22

[Mohammad Maruf Rahman]

I have reproduced this bug with Nightly  50.0a1 (2016-07-14) on WIndows 7 , 64 Bit!

This bug's fix is verified with latest Beta!

Build   ID : 20170307064827
User Agent : Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:53.0) Gecko/20100101 Firefox/53.0

[bugday-20170308]