Closed Bug 1327121 Opened 8 years ago Closed 7 years ago

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

Categories

(DevTools :: Inspector, defect, P3)

50 Branch
defect

Tracking

(firefox51 wontfix, firefox52 wontfix, firefox53 verified, firefox54 verified)

VERIFIED FIXED
Firefox 54
Tracking Status
firefox51 --- wontfix
firefox52 --- wontfix
firefox53 --- verified
firefox54 --- verified

People

(Reporter: arni2033, Assigned: andrewxia2, Mentored)

References

Details

(Keywords: good-first-bug, regression)

Attachments

(1 file, 1 obsolete file)

>>>   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
No longer blocks: 1277113
Component: Untriaged → Developer Tools: Animation Inspector
Component: Developer Tools: Animation Inspector → Developer Tools: Inspector
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]
Mentor: jdescottes
Priority: -- → P3
Keywords: good-first-bug
Could I grab this as my good first bug?
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!
Assignee: nobody → andrewxia2
Status: NEW → ASSIGNED
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
Flags: needinfo?(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!
Flags: needinfo?(jdescottes)
Attached patch Bug1327121.git (obsolete) — Splinter Review
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.
Flags: needinfo?(jdescottes)
Attachment #8839632 - Flags: review+
Attachment #8839632 - Flags: feedback+
Attachment #8839632 - Flags: checkin+
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.
Attachment #8839632 - Flags: review?(jdescottes)
Attachment #8839632 - Flags: review+
Attachment #8839632 - Flags: feedback+
Attachment #8839632 - Flags: checkin+
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!
Attachment #8839632 - Flags: review?(jdescottes) → review+
Flags: needinfo?(jdescottes)
Attached patch Bug1327121.patchSplinter Review
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!
Attachment #8839632 - Attachment is obsolete: true
Attachment #8840585 - Flags: review?(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.
Attachment #8840585 - Flags: review?(jdescottes) → review+
(needinfo on myself to check the try run results and update the flags if needed)
Flags: needinfo?(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!
Flags: needinfo?(jdescottes)
Keywords: checkin-needed
Great! Thank you!
Pushed by philringnalda@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/99239977e6c3
open split console on esc press when inspector search selected r=jdescottes
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/99239977e6c3
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
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?
Flags: needinfo?(jdescottes)
Version: Trunk → 50 Branch
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
Flags: needinfo?(jdescottes)
Attachment #8840585 - Flags: approval-mozilla-aurora?
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 on attachment 8840585 [details] [diff] [review]
Bug1327121.patch

Nice ux improvement and regression fix, let's uplift to aurora.
Attachment #8840585 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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]
Status: RESOLVED → VERIFIED
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: