1106272 - I opened the debugger and the highlighter went crazy

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Victor Porof [:vporof][:vp]]

Attachment: Screen Shot 2014-11-29 at 12.37.59 PM.png

This happened in nightly. I don't know how to reproduce it yet, but I've seen the highlighter get stuck in the past a few times too.

Comment 1

[Patrick Brosset <:pbro>]

What you're seeing here is the SelectorHighlighter trying to highlight all elements that match a given selector (seeing how many elements are highlighted, that selector might have been '*' or 'div' or something like this).
Now, why does it do that on the debugger is very weird. The only place where that highlighter is initialized is the style-inspector and the style-editor, and it's displayed when hovering over selectors in there.
So you probably where using the inspector or style-editor right before, hovered over a selector, then switched to the debugger.
Probably what happened is that it took time for the highlighter to appear, and it only really did appear when the debugger got switched to.
So the main problem is (and I've seen this too a couple of times) the highlighter shouldn't be displayed as soon as you mouseout of a selector, or switch to a different panel or whatever.

Comment 3

[Patrick Brosset <:pbro>]

Wow, it's far easier to reproduce as I first thought.
STR:
- open the inspector on the current page
- with the body element selected, you should see a selector in the rule-view like: "body, td, th, input, select, ..."
- move your mouse quickly over it and then quickly back towards the markup-view
- if you do this several times quickly, you'll get the SelectorHighlighter visible even if the mouse isn't over the selector anymore.

2 things to fix:
- get the show/hide logic right. The event handling is just wrong I think
- add a short delay before showing, I think it's quite frustrating when your mouse just passes quickly over a selector that matches a lot of elements and you suddenly see all the elements being highlighted. Forcing the user to stay on the selector for a while is probably better.
- even better would be to have an icon next to the selector to toggle the highlighter. That would require clicking and would therefore be easier to handle, there would be no chances for errors, and it would also be more obvious to the user. The downside is that this highlighter-selector-on-hover feature also exists in the computed-view and style-editor, so it would have to be changed there too (and for the style-editor, which is based on codemirror, I have no idea whether this even is possible).

Comment 4

[Patrick Brosset <:pbro>]

I'm thinking adding a short delay should fix most of the problems. Perhaps we should file a separate bug for adding the click-icon instead. Jeff, do you think moving away from the highlight on hover is a good idea?

Also, I won't be able to get to the bug quickly, but I can mentor anyone who's interested in fixing it.

Comment 5

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #4)
> I'm thinking adding a short delay should fix most of the problems. Perhaps
> we should file a separate bug for adding the click-icon instead. Jeff, do
> you think moving away from the highlight on hover is a good idea?

I'm interested in the idea. Having a mini icon there does a couple of things:

* the feature is only turned on when the user intends it to be
* the feature's discover-ability is probably going to be less surprising. Right now people find this by either having someone else show them, or accidentally hovering over a rule and having a 'wtf' moment. If we put in a button, people looking at the ruleview will see it and wonder what it is, click on it. Also - I assume is we do a button then wen the button is clicked the highlighter is *locked*, correct?

Right now highlighting on hover is *fast* to turn on and off, but not as deliberate. I could see this use case, borrowed from bug 959073 where the user might want to lock the highlighter for the rule ON, then adjust padding / margin / etc with the lines in place.

Comment 6

[Patrick Brosset <:pbro>]

Ok that makes sense. Let's go for an icon next to the selector that locks the highlighter then.

I think there are a couple of remaining questions though:
- Should there be a way to lock each individual selectors when the rule has several defined, separated by comas like `h1, h2, h3, p {...}`. The UI would be a little weird, but it might be a useful feature.
- Should *all* elements matching the selector always be highlighted, or just the current element. Right now, hovering the selectors highlights all matching elements, that was the whole purpose of the feature, but if we want to somehow merge this with bug 959073, it would be good to also be able to lock the highlighter only on the current node (but perhaps that's something that should be possible via the rule-view toolbar, when we have it, see this mockup: http://cl.ly/image/3p273w0S1f0S).

Regarding the code:

- Here is where the selectors get added to the DOM: http://mxr.mozilla.org/mozilla-central/source/browser/devtools/styleinspector/rule-view.js#1903

- This is part of the RuleEditor class which is responsible for displaying a rule in the rule-view: http://mxr.mozilla.org/mozilla-central/source/browser/devtools/styleinspector/rule-view.js#1827
Adding an icon in there shouldn't be too hard.

- However, right now, all highlighters and tooltips that appear when hovering stuff in the rule-view are managed from this other class: http://mxr.mozilla.org/mozilla-central/source/browser/devtools/styleinspector/style-inspector-overlays.js#46
What this essentially does is detect what type of data the user is currently hovering over in the rule-view (a property name, value, a selector, a url, ...) and uses this information to display the corresponding highlighter or tooltip.

The suggestion would be to modify this to also handle clicks on the new selector highlighter lock icon.

Comment 7

[Patrick Brosset <:pbro>]

I've started to work on this and have something working already.
Technically speaking, I ended up coding this feature directly in rule-view.js rather than style-inspector-overlays.js because the overlays thing doesn't really allowed for this easily. Which made me realize our style-inspector is very far from being easily extensible, so I've filed bug 1134551.

Comment 8

[Patrick Brosset <:pbro>]

Attachment: click-to-highlight.gif

What about something like this?

Comment 9

[Patrick Brosset <:pbro>]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #8)
> Created attachment 8566440 [details]
> click-to-highlight.gif
> 
> What about something like this?
Just to be really clear (because it's a gif, it may be hard to see what's going on): the highlighter locks on the matching nodes when you click on the icon next to the selector.
Clicking on the icon next to the "element" selector only highlights the current node.
Clicking on an icon while a previous selector highlighter was shown will first hide it, and then highlight the new matching nodes.

Comment 10

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #9)
> (In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #8)
> > Created attachment 8566440 [details]
> > click-to-highlight.gif
> > 
> > What about something like this?
> Just to be really clear (because it's a gif, it may be hard to see what's
> going on): the highlighter locks on the matching nodes when you click on the
> icon next to the selector.
> Clicking on the icon next to the "element" selector only highlights the
> current node.
> Clicking on an icon while a previous selector highlighter was shown will
> first hide it, and then highlight the new matching nodes.

Looks great!

Comment 11

[Patrick Brosset <:pbro>]

Attachment: Remove the selector highlight on hover and add a selector icon instead

Comment 12

[Stephanie Hobson [:shobson]]

Attachment: Screen Shot 2015-03-06 at 8.45.27.png

Not sure if this is related or should be a separate bug. In the style editor I get unexpected behaviour when hovering over attribute selectors. I've attached a screen shot of the rule. 

Hovering over the #quick-links-toggle part of the selector correctly highlights the matching elements, hovering over the attribute selector highlights all elements on the page that match the attribute selector, unconstrained by the ID.

Comment 13

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #11)
> Created attachment 8569785 [details] [diff] [review]
> Remove the selector highlight on hover and add a selector icon instead

Patrick - where is this in your queue? Was just shoulder-surfing Stephanie working and we saw some sketchy behaviour from the hover highlighter. I think the best thign for users is to have it enabled intentionally instead of magically.

Comment 14

[Patrick Brosset <:pbro>]

(In reply to Jeff Griffiths (:canuckistani) from comment #13)
> (In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #11)
> > Created attachment 8569785 [details] [diff] [review]
> > Remove the selector highlight on hover and add a selector icon instead
> 
> Patrick - where is this in your queue? Was just shoulder-surfing Stephanie
> working and we saw some sketchy behaviour from the hover highlighter. I
> think the best thign for users is to have it enabled intentionally instead
> of magically.
To be honest, it's not very high on my list right now since I'm busy with other things. But I agree it's important and I could switch priorities for a while.
I'll see when I can squeeze this in.

Comment 15

[Patrick Brosset <:pbro>]

Attachment: MozReview Request: bz://1106272/pbrosset

/r/5943 - Bug 1106272 - 1 -  Remove the selector highlight on hover and add a selector icon instead
/r/5945 - Bug 1106272 - 2 - Changing all rule-view events from custom dom events to eventemitter events
/r/5947 - Bug 1106272 - 3 - Tests for the selector highlighter feature in the rule-view

Pull down these commits:

hg pull review -r 411f575a34c9a1300fb94daf07ad013999b7db56

Comment 16

[Patrick Brosset <:pbro>]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=47767229165e

Comment 17

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8582455 [details]
MozReview Request: bz://1106272/pbrosset

https://reviewboard.mozilla.org/r/5941/#review5111

I wanted to do this for a while but didn't get around to it. I also like the fact that clicking on element highlights the current element... that makes a heckuvalot more sense than the current behaviour.

Comment 18

[Stephanie Hobson [:shobson]]

Yay! So pleased to see this get on the train :D

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/b078b546a896
https://hg.mozilla.org/integration/fx-team/rev/bcfa33e0315c
https://hg.mozilla.org/integration/fx-team/rev/6ddf505a7c22

Comment 20

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

https://hg.mozilla.org/mozilla-central/rev/b078b546a896
https://hg.mozilla.org/mozilla-central/rev/bcfa33e0315c
https://hg.mozilla.org/mozilla-central/rev/6ddf505a7c22

Comment 22

[Patrick Brosset <:pbro>]

Attachment: MozReview Request: Bug 1106272 - 2 - Changing all rule-view events from custom dom events to eventemitter events

Comment 23

[Patrick Brosset <:pbro>]

Attachment: MozReview Request: Bug 1106272 - 3 - Tests for the selector highlighter feature in the rule-view

Comment 24

[Patrick Brosset <:pbro>]

Attachment: MozReview Request: Bug 1106272 - 1 - Remove the selector highlight on hover and add a selector icon instead