968316 - Highlighter: keep highlight overlay visible while selected node is hovered in markup view

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Brian Grinstead [:bgrins]]

This is a minor polish thing I've noticed with the highlighter.

STR:
* Click on an element in the markup view to select an element
* Notice that the overlay highlighting the element on the page appears as you hover the node
* Keep your mouse still, on top of the newly selected node that was just clicked in the markup view

Expected:
As long as my mouse is above the element, the overlay will never disappear.

Actual
The overlay disappears after a couple of seconds.  In order to get it to show and stay back up, I must move the mouse off of the element and back onto it.

Comment 1

[Patrick Brosset <:pbro>]

Clicking on an element in the markup-view or in the breadcrumbs selects it and triggers a brieflyShowBoxModel, which uses a timeout to unhighlight after 1 second.

On the other hand, we also highlight/unhighlight on mousemove of the markup-view.

So what's happening here is that even if the mousemove event on the markup-view is detected and used to highlight the node, the 1 second timeout is still running from having selected the element before, and when it expires, will hide the highlighter, even if the mouse is still over the markup-view.

That should be an easy fix. I haven't thought of it much, but it should be possible to clear the timeout in the highlighter actor if we receive a request to show the highlighter again after it's started.

Comment 2

[Patrick Brosset <:pbro>]

Actually I was wrong, the fix should be client side since the events are going to be triggered in the opposite order: first the node gets hovered over (highlighter shown) and then clicked (highlighter shown then hidden after 1 sec).
Therefore the fix is to not call brieflyShowBoxModel if the node is already being hovered over.

Comment 3

[Patrick Brosset <:pbro>]

Attachment: bug968316-keep-highlighter-visible-on-selecting-hovered-node-in-markupview v1.patch

This should do it.
Ongoing try build https://tbpl.mozilla.org/?tree=Try&rev=886096ddb1dd

The patch contains:
- a simple change to markup-view's _shouldNewSelectionBeHighlighted that checks if the node isn't already hovered when it gets selected
- a new test
- some refactoring of the markup-view's test head.js file

Comment 4

[Brian Grinstead [:bgrins]]

The patch definitely fixes the reported issue - I'm still looking at the code.  There is another issue I've just noticed (with and without the patch applied).  Select a node, then mouse out of the markup view (while keeping the mouse over the selected node before leaving).  Then mouse back into the markupview on the selected node, and the highlight will not show back up.  This is really a pretty minor polish thing, and this fix should be good to go even without that.  I mention it here because if it is something closely related it may be worth adding it.

Comment 5

[Patrick Brosset <:pbro>]

Definitely. Good catch. I'll investigate and add a fix for this here too.

Comment 6

[Patrick Brosset <:pbro>]

Attachment: bug968316-keep-highlighter-visible-on-selecting-hovered-node-in-markupview v2.patch

New ongoing try build: https://tbpl.mozilla.org/?tree=Try&rev=4ba791d8238a
This fixes the minor problem you found in your last comment and adds a new test for it.

Comment 7

[Brian Grinstead [:bgrins]]

Comment on attachment 8371342 [details] [diff] [review]
bug968316-keep-highlighter-visible-on-selecting-hovered-node-in-markupview v2.patch

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

This looks good

::: browser/devtools/markupview/test/browser_inspector_markup_968316_highlight_node_after_mouseleave_mousemove.js
@@ +19,5 @@
> +}
> +
> +function startTests(aInspector, aToolbox) {
> +  let p = content.document.querySelector("p");
> +  Task.spawn(function() {

I like this format for the test - very readable and no globals to clean up

@@ +40,5 @@
> +    ok(isHighlighterVisible(), "the highlighter is visible again");
> +  }).then(null, ok.bind(null, false)).then(endTests);
> +}
> +
> +function waitFor(ms) {

This function isn't being used in the test

::: browser/devtools/markupview/test/browser_inspector_markup_968316_highlit_node_on_hover_then_select.js
@@ +46,5 @@
> +}
> +
> +function waitForTheBrieflyShowBoxModelTimeout() {
> +  let deferred = promise.defer();
> +  // Note that the current timeout is 1 sec and is neither configurable nor

We should definitely have this constant exported at some point... I can just imagine compatibility issues we run when we decide to switch that to 2 seconds or something and are running tests against old servers.  In this case it doesn't really matter though, because we could always make the test wait longer and it would still work.

Comment 8

[Patrick Brosset <:pbro>]

Thanks for the quick review.
Sorry about the extra function, I was just experimenting and forgot to remove it.

Comment 9

[Patrick Brosset <:pbro>]

Attachment: bug968316-keep-highlighter-visible-on-selecting-hovered-node-in-markupview v3.patch

Just removed the extra function.

Comment 10

[Patrick Brosset <:pbro>]

Fixed in fx-team: https://hg.mozilla.org/integration/fx-team/rev/8c2a612f3d15

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8c2a612f3d15

Comment 12

[Patrick Brosset <:pbro>]

Comment on attachment 8371467 [details] [diff] [review]
bug968316-keep-highlighter-visible-on-selecting-hovered-node-in-markupview v3.patch

[Approval Request Comment]

Bug caused by (feature/regressing bug #):
In Firefox 29, we refactored the way the devtools highlighter generally works. The interactions are different now, as seen here https://hacks.mozilla.org/2014/01/upcoming-changes-to-the-firefox-developer-tools-node-picker/ (done in bug 916443).
After that bug landed, a few follow-up bugs were fixed, including this one, which fixes the way the highlighter appears when hovering over the markup-view.

User impact if declined:
If declined, users may not always see the highlighter when hovering over nodes in the markup-view, which could be frustrating especially with the new way the highlighter behaves.
Also, I'm about to request uplift of bug 962647 too, and this one is a dependency of bug 962647.

Testing completed (on m-c, etc.):
Tested on mozilla-central. It's been in nightly for several weeks.
New browser mochitest added and passing.

Risk to taking this patch (and alternatives if risky):
The patch consists of lines changes in markup-view.js (the rest is tests), and looking at the code, I don't see risks involved with applying this to Aurora.

String or IDL/UUID changes made by this patch: None

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/32b887b8aab5