Closed Bug 971967 Opened 10 years ago Closed 9 years ago

Console's right panel does not disappear on document refresh.

Categories

(DevTools :: Console, defect, P3)

30 Branch
defect

Tracking

(firefox40 fixed)

RESOLVED FIXED
Firefox 40
Tracking Status
firefox40 --- fixed

People

(Reporter: danemacmillan, Assigned: jfong)

References

(Blocks 1 open bug)

Details

(Whiteboard: [console-papercuts][polish-backlog][difficulty=easy])

Attachments

(1 file, 4 obsolete files)

The Console's right panel appears when clicking on an expandable Console entry, such as an object, for example. This right panel typically shows all the properties of the selected Console entry. When the document is refreshed, the Console is cleared, but any Console entry that was selected will still appear in the right panel with its properties listed, despite not existing in the Console anymore. There is also no way to manually close this right panel in such a circumstance. 

The right panel should disappear on document refresh, or should at least have a way to manually close it.
Thanks for the bug report.

You can close the panel by pressing Escape when the input or the panel has focus.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Mac OS X → All
Priority: -- → P3
Hardware: x86_64 → All
Whiteboard: [console-papercuts]
Whiteboard: [console-papercuts] → [console-papercuts][devedition-40]
Whiteboard: [console-papercuts][devedition-40] → [console-papercuts][devedition-40][difficulty=easy]
Assignee: nobody → jfong
Attached patch Bug971967.patch (obsolete) — Splinter Review
First attempt at guessing where to destroy the sidebar - works with both clear() and a document refresh. Is this is a good place to call it? 

Also, which test file should I add a test in?
Attachment #8598796 - Flags: feedback?(bgrinstead)
Comment on attachment 8598796 [details] [diff] [review]
Bug971967.patch

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

I think this is a very good place to put it.  Regarding where the test should be, I think we could add this to browser_console_clear_on_reload.js.  See  browser_bug_865871_variables_view_close_on_esc_key.js for an example of interacting with the variables view.
Attachment #8598796 - Flags: feedback?(bgrinstead) → feedback+
Status: NEW → ASSIGNED
Attached patch Bug971967.patch (obsolete) — Splinter Review
Here's the patch including the tests - not 100% sure this is the ideal way to write the two checks for sidebar removal but let me know if I need to change anything.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=816a9d1adb98
Attachment #8598796 - Attachment is obsolete: true
Attachment #8599396 - Flags: review?(bgrinstead)
Comment on attachment 8599396 [details] [diff] [review]
Bug971967.patch

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

::: browser/devtools/webconsole/test/browser_console_clear_on_reload.js
@@ +19,5 @@
>  
>    let hud = yield openConsole();
>    ok(hud, "Web Console opened");
>  
> +  let msg = yield execute("fooObj");

Can you pull this part out into a function (all the way down to the findVariableViewProperties call)?

It could take in a command ("fooObj") and an expected value object ({ name: "testProp", value: "testValue" })

@@ +82,5 @@
> +  executeSoon(() => {
> +    loadBrowser(gBrowser.selectedBrowser);
> +  });
> +
> +  yield hud.jsterm.once("sidebar-closed");

Note covering all of the `executeSoon` calls here.  I've seen the following pattern used more often in our tests:

let sidebarClosed = hud.jsterm.once("sidebar-closed");
loadBrowser(gBrowser.selectedBrowser);
yield sidebarClosed;

Not that the executeSoon way is wrong (it's actually neat that it saves the creation of the temporary variable).  But I think it fits in better with the majority of our tests to switch it to the other way.

@@ +104,5 @@
>       "foobarz1 has been removed from output");
> +
> +  function execute(str) {
> +    let deferred = promise.defer();
> +    hud.jsterm.execute(str, (msg) => {

hud.jsterm.execute now returns a promise so you won't need this helper function anymore (you can just yield on that call directly).

I must have missed this test when making that change before - can you please also update browser_bug_865871_variables_view_close_on_esc_key.js to match this behavior?
Attachment #8599396 - Flags: review?(bgrinstead)
Attached patch Bug971967.patch (obsolete) — Splinter Review
Here are the updated tests - I ended up also refactoring parts of browser_bug_865871_variables_view_close_on_esc_key.js to have similar changes.

Let me know if this needs any updates/changes otherwise I'll run try after.
Attachment #8599396 - Attachment is obsolete: true
Attachment #8599585 - Flags: feedback?(bgrinstead)
Comment on attachment 8599585 [details] [diff] [review]
Bug971967.patch

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

Nice!

::: browser/devtools/webconsole/test/browser_console_clear_on_reload.js
@@ +13,5 @@
>    const TEST_URI = "http://example.com/browser/browser/devtools/webconsole/test/test-console.html";
>  
>    Services.prefs.setBoolPref(PREF, false);
>    registerCleanupFunction(() => Services.prefs.clearUserPref(PREF));
> +  let hud;

Any reason for separating the initializer from the assignment below?
Attachment #8599585 - Flags: feedback?(bgrinstead) → feedback+
Attached patch Bug971967.patch (obsolete) — Splinter Review
Oops good catch! I had it there before when I was moving things around and forgot to undo the change. Updated and also:

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=744dcc53139a
Attachment #8599585 - Attachment is obsolete: true
Attachment #8599601 - Flags: review?(bgrinstead)
Attachment #8599601 - Flags: review?(bgrinstead) → review+
Keywords: checkin-needed
This patch needs rebasing:

Hunk #1 FAILED at 14
1 out of 1 hunks FAILED -- saving rejects to file browser/devtools/webconsole/test/browser_bug_865871_variables_view_close_on_esc_key.js.rej
Flags: needinfo?(jfong)
Keywords: checkin-needed
Attached patch Bug971967.patchSplinter Review
rebased.
Attachment #8599601 - Attachment is obsolete: true
Flags: needinfo?(jfong)
Attachment #8599941 - Flags: review+
Keywords: checkin-needed
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/84674e2bfcb2
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Whiteboard: [console-papercuts][devedition-40][difficulty=easy] → [console-papercuts][polish-backlog][difficulty=easy]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.