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

VERIFIED FIXED in Firefox 49

Status

defect
--
minor
VERIFIED FIXED
4 years ago
10 months ago

People

(Reporter: arni2033, Assigned: gregtatum)

Tracking

Trunk
Firefox 49

Firefox Tracking Flags

(firefox44 affected, firefox49 verified)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

4 years ago
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
(Reporter)

Comment 1

4 years ago
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.
(Reporter)

Comment 3

4 years ago
Hm. This may be pointless work since bug 1026544 (console highlight) needs something stronger than that
(Reporter)

Updated

3 years ago
Has STR: --- → yes
(Assignee)

Updated

3 years ago
Assignee: nobody → gtatum
(Assignee)

Comment 6

3 years ago
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+
(Assignee)

Comment 8

3 years ago
Thanks for the review! Hopefully this will address everything.
Attachment #8745042 - Flags: review?(bgrinstead)
(Assignee)

Updated

3 years ago
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
(Assignee)

Comment 13

3 years ago
This is the rebase from the previous try run.
(Assignee)

Updated

3 years ago
Attachment #8745042 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Keywords: checkin-needed

Comment 15

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/25c5eee42f5a
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
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]

Updated

10 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.