Closed Bug 968316 Opened 6 years ago Closed 6 years ago

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


(DevTools :: Inspector, defect)

Not set


(firefox29 fixed, firefox30 fixed)

Firefox 30
Tracking Status
firefox29 --- fixed
firefox30 --- fixed


(Reporter: bgrins, Assigned: pbro)


(Blocks 1 open bug)


(Whiteboard: [good first verify])


(1 file, 2 obsolete files)

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

* 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

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

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.
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.
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.
Assignee: nobody → pbrosset
This should do it.
Ongoing try build

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
Attachment #8371013 - Flags: review?(bgrinstead)
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.
Definitely. Good catch. I'll investigate and add a fix for this here too.
New ongoing try build:
This fixes the minor problem you found in your last comment and adds a new test for it.
Attachment #8371013 - Attachment is obsolete: true
Attachment #8371013 - Flags: review?(bgrinstead)
Attachment #8371342 - Flags: review?(bgrinstead)
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.
Attachment #8371342 - Flags: review?(bgrinstead) → review+
Thanks for the quick review.
Sorry about the extra function, I was just experimenting and forgot to remove it.
Just removed the extra function.
Attachment #8371342 - Attachment is obsolete: true
Attachment #8371467 - Flags: review+
Fixed in fx-team:
Whiteboard: [fixed-in-fx-team]
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 30
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 (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
Attachment #8371467 - Flags: approval-mozilla-aurora?
Attachment #8371467 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [good first verify]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.