958176 - Split console: Escape should close console sidebar (when visible) without closing split console

Status: RESOLVED FIXED

(DevTools :: Console, defect, P3)

Description

[Brian Grinstead [:bgrins]]

Right now, if the variables view is opened, pressing escape closes it.  In the case of the console being split alongside another panel, it also hides the split console.  We should stopPropagation on the two escape keypresses in webconsole.js to prevent this from happening.

Comment 1

[Thomas Andersen]

Mihai, Brian

I would like to fix this one. Should be easy (famous last words). 
While I am at it I may take a quick look into issue 988278 in order to try to find any useful info about.

Thomas

Comment 2

[Mihai Sucan [:msucan]]

Thomas, to fix this issue please look into JST__onKeypressInVariablesView() in webconsole.js from browser/. Also check JST__keyPress() which can close the sidebar. If you have further questions, please ask.

Thanks a lot for your continued help.

(we recommend you use the needinfo? flag to ask mentors for help)

Comment 3

[Thomas Andersen]

Attachment: 958176.patch

Hi

This patch should solve this.
Should we add a test for this? Eg. add a test to tests/browser_webconsole_split.js

Thomas

Comment 4

[Mihai Sucan [:msucan]]

Comment on attachment 8398199 [details] [diff] [review]
958176.patch

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

This is fixing some use cases, but there's still one that goes wrong. See below.

::: browser/devtools/webconsole/webconsole.js
@@ +3927,4 @@
>          }
>          else if (this.sidebar) {
>            this._sidebarDestroy();
>            aEvent.preventDefault();

I think you need stopPropagation() here as well.

If I open the split console, then open the object inspector for some obj, then focus the js input, then press Escape the sidebar should close. Now the entire split console closes.

Comment 5

[Mihai Sucan [:msucan]]

(In reply to Thomas Andersen from comment #3)
> This patch should solve this.
> Should we add a test for this? Eg. add a test to
> tests/browser_webconsole_split.js

Thanks for the patch! Yes, we should have a test and it should be a new test file. The one you mention is already long.

Comment 6

[Brian Grinstead [:bgrins]]

(In reply to Mihai Sucan [:msucan] from comment #5)
> (In reply to Thomas Andersen from comment #3)
> > This patch should solve this.
> > Should we add a test for this? Eg. add a test to
> > tests/browser_webconsole_split.js
> 
> Thanks for the patch! Yes, we should have a test and it should be a new test
> file. The one you mention is already long.

FYI, here is some info about adding a new test file: https://developer.mozilla.org/en-US/docs/Browser_chrome_tests#Adding_a_new_browser_chrome_test_to_the_tree.  Basically after adding the file, the file name should be added to browser.ini and then you will have to run mach build before you can run it.

Comment 7

[Thomas Andersen]

Thank you for the tip Brian. 
I am familiar with the tests, but it is always nice with some extra information.

Comment 8

[Thomas Andersen]

Thank you for the review Mihai

> If I open the split console, then open the object inspector for some obj, then focus the js input, then press Escape the sidebar should close. Now the entire split console closes.

I am not entirely sure what you mean with "focus the js input". Can you please expand!?
I see a similar issue when the Console - filter output input element is focused. Maybe you are referring to this?

Thomas

Comment 9

[Mihai Sucan [:msucan]]

(In reply to Thomas Andersen from comment #8)
> Thank you for the review Mihai
> 
> > If I open the split console, then open the object inspector for some obj, then focus the js input, then press Escape the sidebar should close. Now the entire split console closes.
> 
> I am not entirely sure what you mean with "focus the js input". Can you
> please expand!?

Focus back the javascript input of the webconsole, where you can type more code.

When you click an object the object inspector is opened and it is given focus. This is why I added the step of focusing back the js input.

Comment 10

[Thomas Andersen]

ok, thanks. It should be fixed now.

> I see a similar issue when the Console - filter output input element is focused.
I'll wait with this one.

I have created a new test file. 
I am opening a panel (inspector), configure a listen once for the "split-console" event, but I am struggling with finding out how to access the opened split-console's hud in order to start the real testing.

Any suggestions?

Comment 11

[Mihai Sucan [:msucan]]

(In reply to Thomas Andersen from comment #10)
> ok, thanks. It should be fixed now.
> 
> > I see a similar issue when the Console - filter output input element is focused.
> I'll wait with this one.
> 
> I have created a new test file. 
> I am opening a panel (inspector), configure a listen once for the
> "split-console" event, but I am struggling with finding out how to access
> the opened split-console's hud in order to start the real testing.
> 
> Any suggestions?

You should be able to do toolbox.getPanel('webconsole').hud;

Comment 12

[Thomas Andersen]

> You should be able to do toolbox.getPanel('webconsole').hud;

ok, thanks. My toolbox object does not have the getPanel() function. I think I found another way using

  Services.obs.addObserver(callbackFn, "web-console-created", false);

Comment 13

[Thomas Andersen]

A quick question about the test/browser.ini file.
In the browser.ini setup for webconsole there are some 'run-if = os == "xxx"' conditionals.

Does that mean that only the test after the conditional is executed if it is true? or does it mean run all tests after the conditional if true?

Comment 14

[Mihai Sucan [:msucan]]

(In reply to Thomas Andersen from comment #12)
> > You should be able to do toolbox.getPanel('webconsole').hud;
> 
> ok, thanks. My toolbox object does not have the getPanel() function. I think
> I found another way using
> 
>   Services.obs.addObserver(callbackFn, "web-console-created", false);

If possible, avoid that notification - I'd like it deprecated someday. It predates the toolbox.

If your toolbox object doesn't have getPanel() it might not be the toolbox instance object. See browser_webconsole_split.js, search for openPanel(), see |toolbox = box|.


(In reply to Thomas Andersen from comment #13)
> A quick question about the test/browser.ini file.
> In the browser.ini setup for webconsole there are some 'run-if = os ==
> "xxx"' conditionals.
> 
> Does that mean that only the test after the conditional is executed if it is
> true? or does it mean run all tests after the conditional if true?

That condition applies only to one test, the one where it appears.

[foo_test.js]
run-if = os === 'bar'

foo_test.js will only run if os is 'bar'. It doesn't affect the other tests.

Comment 15

[Thomas Andersen]

Attachment: 958176.patch

Hi again
Here is a patch containing fix for comment #9 and an initial test file.

I am not sure about the general layout/patterns in the test file. I got a bit excited about Task.js and promises.
The idea is to have a test that tests the various situations where the escape key is pressed. I guess it can expand to eg. 

Press escape key when variables view is open and a property is being edited etc.

Awaiting feedback.

Thomas

Comment 16

[Mihai Sucan [:msucan]]

Comment on attachment 8399503 [details] [diff] [review]
958176.patch

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

This is looking good. My only gripe is with some promise usage in the test.

The test is fairly complete. The only addition I suggest: add an explicit inputNode.focus() call before you close vview. Then open it again and add an explicit vview focus, then press Escape - so we check both cases.

There are some trailing whitespaces... Please make sure you remove them.

Thanks for your good work!

::: browser/devtools/webconsole/test/browser_webconsole_split_escape_key.js
@@ +10,5 @@
> +  let hud;
> +  let jsterm;
> +  let hudMessages;
> +
> +  addTab("data:text/html;charset=utf-8,Web Console test for splitting");

This is deprecated. You can add let {tab} = yield loadTab(url); in the task function.

@@ +19,5 @@
> +
> +  function start() {
> +    Task.spawn(function*() {
> +      
> +      yield openPanel("jsprofiler");

You can do:
toolbox = yield gDevTools.showToolbox(target, "foo")

instead of openPanel(). You can add the oneliner to get the target here, so you dont do it every time you show a panel.

@@ +34,5 @@
> +  }
> +
> +  function testCreateSplitConsoleAfterEscape() {
> +    let deferred = promise.defer();
> +    toolbox.once("webconsole-ready", () => {

once() returns a promise, so you dont need to create your own promise. You can do:

let result = toolbox.once("webconsole-ready", () => { ... });
sendKey();
return result;

(please update all cases)

@@ +47,5 @@
> +
> +  function testOpenSplitConsoleAfterEscape() {
> +    let deferred = promise.defer();
> +    toolbox.once("split-console", () => {
> +      ok(toolbox._splitConsole, "Split console opened.");

there's toolbox.splitConsole as well.

@@ +107,5 @@
> +  }
> +
> +  function executeJS() {
> +    let deferred = promise.defer();
> +    waitForMessages({

you can do:

hudMessages = yield waitForMessages({...});

no need for the .then(), and no need for your own promise.

@@ +119,5 @@
> +      hudMessages = results;
> +      deferred.resolve();
> +    });
> +
> +    jsterm.execute("window");

Please work with a small object. window has many properties and debug builds can timeout on slow test servers.

@@ +153,5 @@
> +
> +  function testDestroy() {
> +    toolbox.destroy().then(function() {
> +      let target = TargetFactory.forTab(gBrowser.selectedTab);
> +      gDevTools.showToolbox(target).then(finish);

why showToolbox() again?

Comment 17

[Thomas Andersen]

Attachment: 958176.patch

Hi again

Thank you for the review. Here is a new patch that should address the issues.

> why showToolbox() again?
My bad. I borrowed it from the browser_webconsole_split.js while setting up the test and did not carefully read the implementation.

> There are some trailing whitespaces... Please make sure you remove them.
Ai! the bane of my existence.
I am trying to learn Sublime Text 2 as I have not have any success with using IntelliJ (too Java centric out of the box) for the Mozilla project. 
I guess with some fiddling it should be possible, but it is nice to learn a new editor too.
Other than raw self discipline are there any good tools/settings for this issue?

I found this: "draw_white_space": "all" which draws the spaces on the screen/document.

Thomas

Comment 18

[Mihai Sucan [:msucan]]

(In reply to Thomas Andersen from comment #17)
> Created attachment 8400796 [details] [diff] [review]
> 958176.patch
> 
> Hi again
> 
> Thank you for the review. Here is a new patch that should address the issues.

Thanks for your work. I will review ASAP (tomorrow).


> > why showToolbox() again?
> My bad. I borrowed it from the browser_webconsole_split.js while setting up
> the test and did not carefully read the implementation.

No worries.


> > There are some trailing whitespaces... Please make sure you remove them.
> Ai! the bane of my existence.
> I am trying to learn Sublime Text 2 as I have not have any success with
> using IntelliJ (too Java centric out of the box) for the Mozilla project. 
> I guess with some fiddling it should be possible, but it is nice to learn a
> new editor too.
> Other than raw self discipline are there any good tools/settings for this
> issue?

I use vim and I dont know sublime well. I just played a bit once with it and it crashed.

hg qdiff/diff/export can be configured to do colored output and it clearly highlights any new trailing whitespaces. See:

http://mercurial.selenic.com/wiki/ColorExtension

I always double check my patches when I'm done.


> I found this: "draw_white_space": "all" which draws the spaces on the
> screen/document.

Sounds good.

Comment 19

[Mihai Sucan [:msucan]]

Comment on attachment 8400796 [details] [diff] [review]
958176.patch

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

Patch looks good. Thanks for the updates.

I cant r+ the patch because the test fails when I run all of the webconsole tests:

https://pastebin.mozilla.org/4752863

... it gets stuck and then it times out.

A couple of nits below...

::: browser/devtools/webconsole/test/browser_webconsole_split_escape_key.js
@@ +1,4 @@
> +/* vim:set ts=2 sw=2 sts=2 et: */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

For tests we usually pick the public domain license.

https://www.mozilla.org/MPL/headers/

@@ +54,5 @@
> +  }
> +
> +  function testShowSplitConsoleAfterEscape() {
> +    let result = toolbox.once("split-console", () => {
> +      ok(toolbox._splitConsole, "Split console is shown.");

toolbox.splitConsole

Comment 20

[Thomas Andersen]

> ... it gets stuck and then it times out.

It seems like the preceding browser_webconsole_split.js test opens the toolbox in a separate window before finishing the test. See testWindowHost(). That window is never closed.

If I change the hostType to BOTTOM before finishing the test, all webconsole tests passes.

eg.: 

testWindowHost() {
  ...
  toolbox.switchHost(Toolbox.HostType.BOTTOM).then(testDestroy);
} 

I guess there is a way to more properly close the window.


Also, there are some errors coming from browser_webconsole_split.js,
https://pastebin.mozilla.org/4753301

Thomas

Comment 21

[Thomas Andersen]

Attachment: 958176.patch

This patch uses the public domain license and uses the public splitConsole property.

Comment 22

[Mihai Sucan [:msucan]]

Comment on attachment 8401241 [details] [diff] [review]
958176.patch

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

Good finding wrt. the other test that doesn't reset the toolbox host. Please fix that test. We cant land this patch without a fix. Thanks!

As for the non-fatal exceptions you see... I've reported some of those. It's sad we have so many failures. Ignore them.

Comment 23

[Brian Grinstead [:bgrins]]

(In reply to Thomas Andersen from comment #17)
> Created attachment 8400796 [details] [diff] [review]
> 958176.patch
> 
> Hi again
> 
> Thank you for the review. Here is a new patch that should address the issues.
> 
> > why showToolbox() again?
> My bad. I borrowed it from the browser_webconsole_split.js while setting up
> the test and did not carefully read the implementation.
> 
> > There are some trailing whitespaces... Please make sure you remove them.
> Ai! the bane of my existence.
> I am trying to learn Sublime Text 2 as I have not have any success with
> using IntelliJ (too Java centric out of the box) for the Mozilla project. 
> I guess with some fiddling it should be possible, but it is nice to learn a
> new editor too.
> Other than raw self discipline are there any good tools/settings for this
> issue?
> 
> I found this: "draw_white_space": "all" which draws the spaces on the
> screen/document.
> 
> Thomas

You can use a plugin called trailingspaces that can highlight all trailing spaces and remove them all at once: https://github.com/SublimeText/TrailingSpaces.  There is also an option called "trim_trailing_white_space_on_save" that you can set to true, which is easier but sometimes certain files have existing trailing spaces so you will end up modifying lines that you didn't want to.

Comment 24

[Thomas Andersen]

Attachment: 958176.patch

ok, this patch includes a fix for browser_webconsole_split.js that resets the toolbox position before finishing the test.

All webconsole tests passes locally.

Thomas

Comment 25

[Thomas Andersen]

> You can use a plugin called trailingspaces that can highlight all trailing spaces and remove them all at once: https://github.com/SublimeText/TrailingSpaces.

Yes I found that. It does a better job when draw the trailing spaces on the screen than the built-in option. Find with regular expression also works ;-)
Thanks!

Thomas

Comment 26

[Mihai Sucan [:msucan]]

Comment on attachment 8401354 [details] [diff] [review]
958176.patch

Thanks for the fix. Sorry for the trouble - we have bugs in tests as well. :)

Try push: https://tbpl.mozilla.org/?tree=Try&rev=71fe89a943ac

Once results are green, we can land the patch.

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/9e437936649a

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9e437936649a