1023116 - Highlighter is not dismissed and doesn't highlight sometimes

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Paul Rouget [:paul]]

Attachment: screencast.mp4

I use the Simulator 2.0 (a recent build).

Attached a screencast.

To notice:
- sometimes (didn't figure out when yet), when a node is selected (here I use a keyboard shortcut), it's not highlighted in the page. It's some sort of combination of hovering/selecting a node first, and coming back to the node
- iirc, we were supposed to dismiss the higjlighter after 3 seconds. Here it's not happening.

Comment 1

[Paul Rouget [:paul]]

STR:
- If the DOM tree is browsed with the arrow keys, the node that is hovered by the mouse won't get highlighted.
- If the highlighter is showed because a node has been selected or hovered in the DOM tree, the highlighter is never dismissed.

Comment 2

[Patrick Brosset <:pbro>]

Not sure to fully understand the video and STRs, but here is what I can say:
- selecting a node highlights it for 1 second. Selection is done by either clicking on a node in the breadcrumbs or in the markup-view, or in the simulator with the element picker.
- hovering over a node in the markup-view highlights the node for as long as the mouse is over that node.

--> This means that if you hover over a node in the view, the highlighter won't be dismissed unless you move to another node, or leave the view, or trigger a new selection (in which case the highlighter will be shown for 1 second over that new selected node).
What seems to happen with the keyboard navigation is the following:
- you hover over a node --> highlighter is shown and stays there
- you press up or down --> that triggers a new node selection --> highlighter is shown over the new node for 1 second only, and then goes away
- even if your mouse didn't move from node 1, since there's been a selection in the meantime, this removed the highlighter-while-hover thing.

Maybe we could re-highlighter the hovered node as soon as the brief highlight of the new selection is done, but I think that may be strange.

Am I missing an error? When you say the highlighter is never dismissed, do you mean that it's never ever dismissed even if you move your mouse away from the toolbox?

One thing to note is the simulator 1.3 didn't have the highlighter actor, that only got introduced in 29 and with this actor, we changed the way the highlighter worked (highlight on hover only).
1.3 only had the local highlighter AND the highlight method on the WalkerActor. This method starts a setTimeout to hide the highlighter.
So it may be that what you're seeing the difference between 1.3 and 2.0.

Comment 3

[Paul Rouget [:paul]]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #2)
> What seems to happen with the keyboard navigation is the following:
> - you hover over a node --> highlighter is shown and stays there
> - you press up or down --> that triggers a new node selection -->
> highlighter is shown over the new node for 1 second only, and then goes away
> - even if your mouse didn't move from node 1, since there's been a selection
> in the meantime, this removed the highlighter-while-hover thing.

So far, yes. But then, press up to go back to the node that is hovered by the mouse, no highlight.

Imagine 2 nodes: A and B.

I click on A. A is highlighted. Mouse is over A (in the DOM tree). Press down. B is selected and highlighted. Press up. A is selected but not highlighted.

> Am I missing an error? When you say the highlighter is never dismissed, do
> you mean that it's never ever dismissed even if you move your mouse away
> from the toolbox?

I need to re-test that. I'll tell you.

Comment 4

[Patrick Brosset <:pbro>]

(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #3)
> Imagine 2 nodes: A and B.
> 
> I click on A. A is highlighted. Mouse is over A (in the DOM tree). Press
> down. B is selected and highlighted. Press up. A is selected but not
> highlighted.
Good point.
The code that determines if a node should be highlighted for a second upon being selected is:
http://mxr.mozilla.org/mozilla-central/source/browser/devtools/markupview/markup-view.js#294
And it checks for:
'let isHighlitNode = this._hoveredNode === this._inspector.selection.nodeFront;'
Since A was clicked on and since the mouse remained on A, that condition is true which tells the markup-view not to briefly highlight it on re-selection.
This condition normally behaves fine except after the highlight-on-hover was dismissed when using keyboard navigation.

Comment 5

[Patrick Brosset <:pbro>]

Attachment: bug1023116-always-highlight-on-select v1.patch

With this patch, we make sure to highlight again the previously hovered element, even if in the meantime, the keyboard was used to navigate to another node and then back.

Ongoing try build: https://tbpl.mozilla.org/?tree=Try&rev=8e59598d70c0

Comment 6

[Brian Grinstead [:bgrins]]

Comment on attachment 8444378 [details] [diff] [review]
bug1023116-always-highlight-on-select v1.patch

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

Changes look fine to me and it fixes the issue

Comment 7

[Patrick Brosset <:pbro>]

https://tbpl.mozilla.org/?tree=Try&rev=8e59598d70c0
https://hg.mozilla.org/integration/fx-team/rev/c4388b880938

Paul, with this patch now in, the keyboard navigation usecase you described is fixed.
Did you have a chance to re-test as you said in comment 3? Is there anything else we should use this bug for?

Comment 8

[Paul Rouget [:paul]]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #7)
> Is there anything else we should use this bug for?

I'll find a new bug if necessary.

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c4388b880938