Closed Bug 1214754 Opened 9 years ago Closed 8 years ago

Element's highlight doesn't go away when I clear the console by Ctrl+L

Categories

(DevTools :: Console, defect)

Product:
DevTools
Component:
Console
Type:
defect
Priority:
Not set
Severity:
minor

Tracking

(firefox44 affected, firefox49 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 49
Tracking Flags:
Tracking Status
firefox44 --- affected
firefox49 --- verified

People

(Reporter: arni2033, Assigned: gregtatum)

References

Details

Attachments

(2 files, 2 obsolete files)

screenshot - Element's highlight doesn't go away when I clear the console by Ctrl+L.png
119.75 KB, image/png
Details
Clear hovered element's highlight when console is cleared
1.23 KB, patch
Details | Diff | Splinter Review
STR: (Win7_64, Nightly 44, 32bit, ID 20151012030612, new profile) 1. Open console on this page (Ctrl+Shift+K) 2. Type "document.body" w/o quotes in console, press Enter 3. Move mouse over <body> element logged in console to highlight it on page 4. Press Ctrl+L 5. (unnecessary) Move mouse to the page content Result: <body> is highlighted on page Expectations: <body>'s highlight should go away because there's nothing what could cause it. Workaround: Close devtools
Sorry. Better workaround: Open inspector, hover over any element to highlight it, and move pointer away
Severity: normal → minor
See Also: → 1214770
Thanks for filing this. For info, this happens in console-output.js: https://dxr.mozilla.org/mozilla-central/source/devtools/client/webconsole/console-output.js#3249 unhighlightDomNode is only called on mouseout of the element, but it should probably also be called on destroy.
Hm. This may be pointless work since bug 1026544 (console highlight) needs something stronger than that
Has STR: --- → yes
Assignee: nobody → gtatum
This should always unhighlight the dom element once it's unloaded.
Attachment #8744469 - Flags: review?(bgrinstead)
Comment on attachment 8744469 [details] [diff] [review] Element's highlight doesn't go away when I clear the console by Ctrl+L Review of attachment 8744469 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, this fixes the problem. I have a few comments, please see inline. Can you also update the commit message to explain what the fix is doing and not the problem itself? ::: devtools/client/webconsole/console-output.js @@ +3351,5 @@ > }), > > destroy: function() > { > if (this.toolbox && this._nodeFront) { There's a bit of async weirdness here in which the destroy function seems to be assumed as sync so if we were waiting for unhighlightDomNode to resolve and it was called again we'd enter this again. But I don't think that practically ever happens and refactoring this to make it async isn't worth it for this bug fix. So I think this should be fine, but see notes below @@ +3355,5 @@ > if (this.toolbox && this._nodeFront) { > this.element.removeEventListener("mouseover", this.highlightDomNode, false); > this.element.removeEventListener("mouseout", this.unhighlightDomNode, false); > this._openInspectorNode.removeEventListener("mousedown", this.openNodeInInspector, true); > + I don't think we want to call unhighlightDomNode here if it's never been highlighted / linked to the inspector in the first place (since that will cause extra work and try to connect nodes that don't ever get linked when created, like `document`). A quick way to guard against this would be to check if (this._linkedToInspector) { /* unhighlight and remove references */ } else { /* just remove references */ } @@ +3356,5 @@ > this.element.removeEventListener("mouseover", this.highlightDomNode, false); > this.element.removeEventListener("mouseout", this.unhighlightDomNode, false); > this._openInspectorNode.removeEventListener("mousedown", this.openNodeInInspector, true); > + > + this.unhighlightDomNode().catch(() => {}).then(() => { Don't need to catch this, unhighlightDomNode is already catching
Attachment #8744469 - Flags: review?(bgrinstead) → feedback+
Thanks for the review! Hopefully this will address everything.
Attachment #8745042 - Flags: review?(bgrinstead)
Attachment #8744469 - Attachment is obsolete: true
Comment on attachment 8745042 [details] [diff] [review] Clear hovered element's highlight when console is cleared Review of attachment 8745042 [details] [diff] [review]: ----------------------------------------------------------------- Works for me, thanks
Attachment #8745042 - Flags: review?(bgrinstead) → review+
Status: NEW → ASSIGNED
This is the rebase from the previous try run.
Attachment #8745042 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/25c5eee42f5a
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
I've managed to reproduce this bug on Nightly 44.0a1 (2015-10-12) (Build ID: 20151012030612) on Linux, 64 Bit. This Bug is now verified as fixed on Latest Firefox Developer Edition 49.0a2 (2016-06-29) Build ID: 20160629004019 User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:49.0) Gecko/20100101 Firefox/49.0 OS: Ubuntu 14.04 ; Linux 3.19.0-61-generic
QA Whiteboard: [bugday-20160629]
I have reproduced this bug with Firefox Nightly 44.0a1 (Build ID:20151014030223) on Windows 8.1, 64-bit. Verified as fixed with Firefox Developer edition 49.0a2 (Build ID: 20160723004004) Mozilla/5.0 (Windows NT 6.3; WOW64; rv:49.0) Gecko/20100101 Firefox/49.0 As this bug is also verified on Linux(comment 16), I am marking this as verified!
Status: RESOLVED → VERIFIED
QA Whiteboard: [bugday-20160629] → [bugday-20160629] [testday-20160722]
status-firefox49: fixed → 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: