Open Bug 959073 Opened 10 years ago Updated 2 years ago

Lock the highlighter on the currently selected element

Categories

(DevTools :: Inspector, enhancement, P2)

Product:
DevTools
Component:
Inspector
Type:
enhancement

Tracking

(Not tracked)

People

(Reporter: pbro, Unassigned)

References

Details

(Whiteboard: [good next bug][lang=js])

Attachments

(5 files, 10 obsolete files)

box-model-highlighter.png
290.25 KB, image/png
Details
Screen Shot 2015-12-04 at 11.59.47 AM.png
192.43 KB, image/png
Details
Screenshot from 2016-10-26 19-47-52.png
210.00 KB, image/png
Details
Screenshot from 2016-12-15 20-20-38.png
124.46 KB, image/png
Details
bug959073-patch71.diff
16.11 KB, patch
Details | Diff | Splinter Review 
With bug 916443 fixed, the new behavior of the highlighter is to show while you hover over nodes in the markup view.

Also, when you do select an element (either right-click/inspect on the page), or click in the markup view, or click in the breadcrumbs, or using the inspect button then the highlighter is shown for a second and disappears then.
At that point the only way to remember which node you selected is by either looking into the markup view to see which node has a different background color, or using you mouse, hover again in the markup view.

This was done on purpose as the highlighter outline was more often than not distracting when you wanted to see the element on the page (its border for example).

Having said this, I've received a couple of comments about the fact that the "lock" mode was helping in some situations.

We need to keep in mind too that the highlighter outline is going to visually change a lot very soon with bug 663778 (see this screenshot https://bug663778.bugzilla.mozilla.org/attachment.cgi?id=831589) and that keeping it visible at all times as it was done before may not be a viable option anymore anyway.

So, that's why I'd like a discussion to take place here.
And I would like to propose an idea:

The ability to pin a node so that its highlighter stays visible as long as it is pinned.
The pin/unpin option could be available from the markup view contextual menu.
On top of making it easy to see which is the currently selected element, it could also be pretty useful to track changes made to the content/border/padding/margin while they are being made.
With that option, it should be possible to pin many nodes, not just one.
To make it easier to unpin, a close button should be located in the nodeinfo bar.
While pinned nodes are being shown, the highlighter on hover should still work.
> Having said this, I've received a couple of comments
> about the fact that the "lock" mode was helping in some
> situations.

A "couple" of comments is not a lot.

Before jumping on a solution to address this issue, and introduce UI and code complexity, we need to understand how much this is needed. I'd prefer to keep the highlighter code and lock-logic as simple as possible.


How is this addressed in other browsers?
* Firebug highlights on hover of a node in the markup-view (or breadcrumb, debugger, console output, ...). There's no lock mode at all. The highlighter draws box-model regions.
* Safari's tools are very similar, no lock mode.
* Chrome's tools, same here again. Except that when you select a node by clicking on the page, it stays highlighted for about 2 seconds.
* Opera: same.

So, we've aligned our behavior with that of other browsers' devtools.
What we do a bit better though, is that as you move your mouse over the page after you've started the inspect mode, nodes are being shown in the markup view at the same time.

I agree, a couple of comments is not enough, let's hear it from other people.
Opera Dragonfly had the ability to do both, but I didn't feel that it helped much and could get confusing.
I commented on the google groups post about how no "lock mode" could be disturbing.
Here's my suggestion :
When an element is selected in the markup view. The inspector will be locked.

Lock mode :
- Keep the info bar. There's absolutely no reason to hide away convenient and accessible information. The inspect button and the dropdown button should also be back. 
- Hovering an element in the markup view will turn it in Preview mode

Preview mode :
- The info bar (without the buttons) will display until you mouse out the element in the markup view, in which case it will return to the previously locked element.
- When you tap the "ENTER" key on your keyboard, it will lock the element (which is currently hovered in the markup view)
- When you click the element in the markup view, it will lock it
- When you mouse away the inspector, it will quit the preview mode and keep the previously locked element

So basically, the behavior is nearly the same as the old one, just hovering an element in the markup view will change the info bar until you mouse out that markup view element.

I actually hate the current behavior as it's not obvious which element is selected.
Thanks for the feedback, Tim. We decided to change the model as a result of many requests to adopt this type of model (e.g. http://flailingmonkey.com/firefox-developer-tools-highlighter), and spent some time trying to work out what to do (for example, see this set of notes which detail many of the pros and cons https://etherpad.mozilla.org/highlighter-brain-dump)

Another key argument was that we want to display more information (padding, margins, etc) in the highlighter. The current highlight visualization is very lightweight so its persistence isn't too much of a problem, but as we add stuff (and we've got several things that we're working on) it's going to turn into a huge problem.

Could I ask you to take a look at the logic behind where we are, use it a bit more and see if we can come up with proposals that don't just revert where we are? The research that we did was fairly clear that this was a step in the right direction.
(In reply to Joe Walker [:jwalker] from comment #6)
> Thanks for the feedback, Tim. We decided to change the model as a result of
> many requests to adopt this type of model (e.g.
> http://flailingmonkey.com/firefox-developer-tools-highlighter), and spent
> some time trying to work out what to do (for example, see this set of notes
> which detail many of the pros and cons
> https://etherpad.mozilla.org/highlighter-brain-dump)
> 
> Another key argument was that we want to display more information (padding,
> margins, etc) in the highlighter. The current highlight visualization is
> very lightweight so its persistence isn't too much of a problem, but as we
> add stuff (and we've got several things that we're working on) it's going to
> turn into a huge problem.
> 
> Could I ask you to take a look at the logic behind where we are, use it a
> bit more and see if we can come up with proposals that don't just revert
> where we are? The research that we did was fairly clear that this was a step
> in the right direction.

Sure, I think showing padding, margin... is a good improvement. But I think the highlight on hover model really makes it confusing, plus it's not accident proof (accidental hover).
Also, there's still an actual selected element (the one that is selected in the markup view, which the styles are shown for that element), so it will be nice to have a better indication of that element, so I guess an infobar will be welcomed here.
The highlight on hover does have an advantage, but that advantage could be taken and be mixed with a lock mode. 
So my solution would be not making the infobar fade out for the element selected in the markup view, the quick inspect and the dropdown could be removed, but I think the infobar (with the borders/margin/padding around the element) should stay. Now for the highlight on hover advantage, you could make just the infobar update to the hovered element, and then go back to the selected element when the element is no longer hovered.
As you can see, my solution gives both information about the selected element, and adds highlight on hover in a non-disturbing way.
Thanks Tim. It's a good idea to make it easier to see what the selected element is. We thought about it when this was being designed, but decided not to do anything at the time, however that's not set in stone.

I'm keen that we don't do 2 things:
* confuse the display when we're highlighting
* be annoying when the selected element isn't interesting (which isn't an easy concept, for example $0 in the webconsole)
(In reply to Joe Walker [:jwalker] from comment #8)
> Thanks Tim. It's a good idea to make it easier to see what the selected
> element is. We thought about it when this was being designed, but decided
> not to do anything at the time, however that's not set in stone.
> 
> I'm keen that we don't do 2 things:
> * confuse the display when we're highlighting
> * be annoying when the selected element isn't interesting (which isn't an
> easy concept, for example $0 in the webconsole)

Well, my suggestion is a fix to this bug, so... if you're saying that the devtools team is not going to do anything, I don't get the purpose of this bug.
(In reply to Tim Nguyen [:ntim] from comment #9)
> Well, my suggestion is a fix to this bug, so... if you're saying that the
> devtools team is not going to do anything, I don't get the purpose of this
> bug.

I think it's a good idea to find ways to make currently selected element more visible. But it's not by just undoing the changes we've made, and when we do it we have to bear in mind a number of gotchas.
The biggest problem I see with the current state is that the color for the currently hovered or highlighted nodes is light grey. This need to be more obvious, and I suspect this is already in progress. Firebug and Chrome use a relatively bright blue. Let's not be creative here if just copying others helps developers. I suspect this is already being worked on?

Having said that I do think Tim's idea of having a way to lock the highlighter into remaining visible in some way is interesting. IMO it shouldn't be ENTER as that currently gets you into editing mode, but maybe something in the context menu?
Terminology: "lock the highlighter" could be confusing. Could we talk about "outline selected element" or something like that.

There's an easy way to see how this type of thing would feel, and that is to use Opera Dragonfly for a while.
(In reply to Joe Walker [:jwalker] from comment #12)

> There's an easy way to see how this type of thing would feel, and that is to
> use Opera Dragonfly for a while.

What version of Opera do I need to run Dragonfly? I assume it's an old one.
I have v12
(In reply to Jeff Griffiths (:canuckistani) from comment #13)

> What version of Opera do I need to run Dragonfly? I assume it's an old one.

Never mind, found an old download of Opera 12, futzed around with it for a bit.

It's interesting, and *very* opinionated. I think there are some good ideas in there:

* seeing the currently selected outline and the currently hovered outline at the same time, given their subtle blue colour scheme isn't terrible. It might be pretty annoying if either a) you were working with a site with similar colours, or b) your highlighter was more colourful. It's hard to tell how it would fare with the current chrome highlighter colours and this behaviour.

* it's interesting that they have toggle buttons for the behaviours 'highlight selected element' and 'select an element in the page to inspect it' - you could turn them off but they feel like things you would toggle once, not switch back and forth constantly.

Sticky highlighting of a node might be something you would want to do if you were tweaking css, but probably not want the rest of the time. This is the use case I think of when I say it might be good to be able to lock the (new, box-model) highlighter onto a node and have it remain visible. Curious to know what others think.
With Mike's patch for bug 663778 applied, you can see what our highlighter will look like.
The patch isn't final yet, so this might change, but it gives a feeling of what we're aiming to.
So, with that in mind, I think we all agree that the lock mode we had previously isn't going to work anymore.

I think a mode where you can choose the highlight a node temporarily would work.
As a web developer, I never found myself wanting to do that, but I think it could be interesting to some people.
(In reply to Joe Walker [:jwalker] from comment #10)
> (In reply to Tim Nguyen [:ntim] from comment #9)
> > Well, my suggestion is a fix to this bug, so... if you're saying that the
> > devtools team is not going to do anything, I don't get the purpose of this
> > bug.
> 
> I think it's a good idea to find ways to make currently selected element
> more visible. But it's not by just undoing the changes we've made, and when
> we do it we have to bear in mind a number of gotchas.

I'm not saying undo the changes you made. I'm just saying that when mousing out an element in the markup view, go back to the current element (that is shown in the rules view) instead of staying on an random element.
Also, the "lock" mode was useful when it comes to instant layout changes (such as hover, or drag,...). Now I can no longer watch for instant layout changes, I have to add a 1px border to see how it changes, it's not very convenient.
Maybe a checkbox with "Watch layout changes" would be nice (it would just show a small outline instead of the huge box model highlighter).
Also relevant to this bug: bug 971662, and especially this: https://www.youtube.com/watch?v=EZ-nWDRht68
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #30)
> Also relevant to this bug: bug 971662, and especially this:
> https://www.youtube.com/watch?v=EZ-nWDRht68

Maybe this bug could be about implementing the UI?
One idea from Brian (https://bugzilla.mozilla.org/show_bug.cgi?id=971662#c27) is to have a rule-view toolbar (primarily used for toggling pseudo-classes) that contains a 'lock highlighter' button.
Another idea is to have something similar via the ctx-menu.
So, with bug 971662, we now have a gcli command to highlight any number of elements with a css selector, and we will soon have highlight on hover of style-editor selectors.

So I think this bug should now focus on giving the inspector a simple way to lock the highlighter on the currently selected element, as we discussed in the past few comments.

There are several ways to achieve this in the UI, a toggle button, a contextual menu item.
When locking the highlighter on the currently selected node, it should probably be unlocked as soon as the current selection changes.

This + the gcli command should be enough for all use cases.
Summary: [highlighter] Hard to tell the currently selected element → Lock the highlighter on the currently selected element
Attachment #8399291 - Attachment is obsolete: true
Severity: normal → enhancement
OS: Mac OS X → All
Hardware: x86 → All
(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #34)
> Was just reminded of this in:
> https://twitter.com/flaugher/status/672168325548412928

Yup, me too. ni'ing Bryan and Helen on this bug to get it on their radar.

For context, this is often suggested as the one thing people really miss from Opera's old Dragonfly devtools before they got blinked.
Flags: needinfo?(hholmes)
Flags: needinfo?(clarkbw)
So, I'm going to propose we add a highlighter lock on one element, and one element only. You do it by right-clicking a node and adding it to the right-hand option menu. Handling multiple locks with a UI seems... excessive. (Regardless of how robust the features of GCLI highlight-locking are.)
Flags: needinfo?(hholmes)
Mentor: pbrosset
Putting it in the context menu gets us to shipping this faster but we'll need to circle back on how we could make this feature discoverable as well.  Agreed about only locking on the one element for now.

Should the GCLI highlight and this lock feature stay separate features?  Or are they similar enough that we should explore something else?  Probably need to look up metrics for the highlight feature if we have them.
Flags: needinfo?(clarkbw)
Priority: -- → P3
Patrick: wish this was P1
Flags: needinfo?(pbrosset)
(In reply to Jeff Griffiths (:canuckistani) (:⚡︎) from comment #40)
> Patrick: wish this was P1
Way, I'd like to have this too, however I don't think we have the bandwidth now.
We still had some untriaged "features", so I was setting P3 as a default for them, knowing that we're working mostly on bug fixes and devtools.html at the moment.
Q4 might be a better time for us to work on this, so P2 sounds about right.

However, this isn't too hard and I can mentor anyone who wants to get started on this.
Flags: needinfo?(pbrosset)
Priority: P3 → P2
Whiteboard: [good next bug][lang=js]
Attached patch bug959073-patch1.diff (obsolete) — — Splinter Review 
Hi Patrick, I would like to work on this bug if you can mentor me.

I attached a patch that adds a "Highlight Node" element to the context menu. By now, however, the highlight gets cleared when you hover another node. Any advice here?
Attachment #8799003 - Flags: feedback?(pbrosset)
Thanks for working on this Albert.
Worth noting: since bug 1297383, we in fact have a highlighter locking feature already: you can now click on the inspector icon next to the 'element { ... }' rule in the rule-view.
So we'll have to figure out how these 2 work together, and whether we need them both to even exist.

With regards to your code changes, they look good. The reason the highlighter goes away when you hover over nodes is because you're using the same highlighter than what the markup-view uses to highlight nodes on hover:
this._toolbox.highlighterUtils.highlightNodeFront(selection.nodeFront);

If you want the highlighter to stay locked you should create another instance of it, so that you can leave it ON even while other nodes become highlighted.

You can do this with this._toolbox.highlighterUtils.getHighlighterByType("BoxModel")
which returns a promise with, as an argument, the new instance of the BoxModel highlighter that you may want to use.
Comment on attachment 8799003 [details] [diff] [review]
bug959073-patch1.diff

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

See my previous comment.
Attachment #8799003 - Flags: feedback?(pbrosset)
(In reply to Patrick Brosset <:pbro> from comment #43)
...
> Worth noting: since bug 1297383, we in fact have a highlighter locking
> feature already: you can now click on the inspector icon next to the
> 'element { ... }' rule in the rule-view.

Oh interesting! This works pretty well, the one slightly strange thing is this workflow:

1. I select a node I want to be highlighted as a 'reference' node
2. I lock the highlighter on it by using the method above
3. I inspect another node somewhere else in the document
4. I make adjustments to the second node while visualizing it's position etc in relation to the first node
5. I now want to 'un-lock' the previously locked highlighter. If the node I locked is not immediately accessible in the inspector view, it can be difficult find the first node, wasting a lot of time.

I think the user story I'm looking for is something like: As a developer, if I lock the highlighter onto a node, it should be easy for me to get back to that node and unlock the highlighter.

Workaround: just lock the highlighter to a different node, you can only do one node at a time.
Good point Jeff. It's easy to loose track of which element has been locked if you select another one.
I wonder what an find-my-previous-element UI would look like, but I'll file a subsequent bug to discuss this workflow.
(In reply to Patrick Brosset <:pbro> from comment #46)
> Good point Jeff. It's easy to loose track of which element has been locked
> if you select another one.
> I wonder what an find-my-previous-element UI would look like, but I'll file
> a subsequent bug to discuss this workflow.
bug 1309611.
Attached patch bug959073-patch1.diff (obsolete) — — Splinter Review 
I uploaded a new version of my WIP.

I copied and adapted the implementation of the highlighter in the rule-view to make it work in the inspector.

It works as expected except in the following case (see screnshot attached in the next comment):

- Highlight a node (ie: #brandLogo).

- Without selecting the next node (ie: #topSection), click with the secondary button on it and open the context menu.

- Expected result: the Highlight menu item is enabled and Unhighlight is disabled.

- Actual result: the Unhighlight menu item is enabled and Highlight is disabled.

The reason for this is that I use 'selectionCssSelector' to get a node identifier to save the highlighted node reference. Then, when opening the menu before selecting the item, it doesn't update the selectionCssSelector value on time, so it still thinks #brandLogo is selected even though we are showing the menu for #topSection.

What do you think is the best solution for this? I would need a synchronous way to store the highlighted node reference and compare it with the selected node when opening the menu. I could JSON.stringify(this.selection) and then compare the two strings, but probably that's not the best solution. :)

Btw, I assign the bug to me, if it's ok.
Assignee: nobody → aljullu
Attachment #8799003 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8801588 - Flags: feedback?(pbrosset)
Attached image 959073-patch-issue.png (obsolete) — 

    
    

    
  
Comment on attachment 8801588 [details] [diff] [review]
bug959073-patch1.diff

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

::: devtools/client/inspector/inspector-panel.js
@@ +958,5 @@
>        hidden: !this._supportsDuplicateNode,
>        disabled: !isDuplicatableElement,
>        click: () => this.duplicateNode(),
>      }));
>      menu.append(new MenuItem({

This new menu item should go to the bottom section of the menu, just after node-menu-showdomproperties.

@@ +960,5 @@
>        click: () => this.duplicateNode(),
>      }));
>      menu.append(new MenuItem({
> +      id: "node-menu-highlight",
> +      label: INSPECTOR_L10N.getStr("inspectorHTMLHighlight.label"),

The label value should depend on whether or not the node is already highlighted. We don't need 2 menu items, just one will do.
Either we right click on a node that is *not* highlighted and show "highlight node", or we right click on a node that *is* highlighted already, and show "unhighlight node". So, just one menu item is enough.

@@ +961,5 @@
>      }));
>      menu.append(new MenuItem({
> +      id: "node-menu-highlight",
> +      label: INSPECTOR_L10N.getStr("inspectorHTMLHighlight.label"),
> +      disabled: isNodeHighlighted,

This menu item should only be disabled when right-clicking on a node that we technically can't highlight in the page, like a comment node, or doctype node for instance.
Any element or text node can be highlighted, so the menu should be enabled for those, and disabled for others.

@@ +1696,5 @@
>    },
>  
>    /**
> +   * Get an instance of BoxModelHighlighter (used to highlight nodes that match
> +   * selectors in the rule-view). A new instance is only created the first time

Please update this comment, it says "rule-view" but here you're using the highlighter in the inspector panel.

@@ +1707,5 @@
> +    if (!utils.supportsCustomHighlighters()) {
> +      return null;
> +    }
> +
> +    if (this.boxModelHighlighter) {

Suggestion to rename this to something a bit more specific. There are many highlighters in the inspector, used in various panels, so we want to make sure each one has a self-explanatory name.

this.elementLockedHighlighter for instance, and therefore the function should be renamed to something like getElementLockedHighlighter

@@ +1725,5 @@
> +
> +  /**
> +   * Highlight the selected node
> +   */
> +  highlightNode: Task.async(function* () {

Same here, please rename to something more specific, like lockHighlighterOnElement

@@ +1738,5 @@
> +    if (!highlighter) {
> +      return;
> +    }
> +
> +    yield highlighter.show(selection._nodeFront);

I think selection.nodeFront works fine, no need for leading underscore.

@@ +1740,5 @@
> +    }
> +
> +    yield highlighter.show(selection._nodeFront);
> +
> +    selection.nodeFront.getUniqueSelector().then((selector) => {

Why using the unique selector here. I think you could simply store the current selection.

this.currentHighlighterLockedElement = selection.nodeFront;

This way it is synchronous.
If you do this however, you'll need to do this in the destroy method too:

this.currentHighlighterLockedElement = null;

@@ +1751,5 @@
> +   */
> +  unhighlightNode: Task.async(function* () {
> +    let highlighter = yield this.getBoxModelHighlighter();
> +    if (!highlighter ||
> +        this.selectionCssSelector !== this.highlightedSelectionCssSelector) {

I don't think this second line in the condition is needed. Unhighlighting unconditionally should work just fine, right? The hide method doesn't do anything if no node is currently highlighted anyway.

::: devtools/client/locales/en-US/inspector.properties
@@ +190,5 @@
>  
> +# LOCALIZATION NOTE (inspectorHTMLHighlight.label): This is the label shown in
> +# the inspector contextual-menu for the item that lets users highlight the
> +# current node
> +inspectorHTMLHighlight.label=Highlight Node

Maybe "Lock Highlighter on Node" would be clearer?

@@ +195,5 @@
> +
> +# LOCALIZATION NOTE (inspectorHTMLUnhighlight.label): This is the label shown in
> +# the inspector contextual-menu for the item that lets users unhighlight the
> +# current node
> +inspectorHTMLUnhighlight.label=Unhighlight Node

Similar change here.

        Attachment #8801588 -
        Flags: feedback?(pbrosset)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        bug959073-patch3.diff (obsolete)
        — 
          — Splinter Review
      

    


  
Ok, I added a new version of the patch with all the suggestions implemented.

One comment:

I think there was a bug in client/framework/selection.js, it was not correctly detecting <head> and <body> elements if they were lower-case. I fixed it in this patch, but please, let me know if this should be done in another bug.

One issue missing:

I don't know how to remove the highlighter when deleting the node with the Del key. Any advice here?

        Attachment #8801588 -
        Attachment is obsolete: true

        Attachment #8801589 -
        Attachment is obsolete: true

        Attachment #8802622 -
        Flags: feedback?(pbrosset)

    
    

    
  
Comment on attachment 8802622 [details] [diff] [review]
bug959073-patch3.diff

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

Thanks for the patch. This looks good. I've made a few comments below.
I think my comment to your changes in deleteNode should fix the problem you mentioned in comment 51.

::: devtools/client/framework/selection.js
@@ +292,5 @@
>     */
>    isBodyNode: function () {
>      return this.isHTMLNode() &&
>             this.isConnected() &&
> +           this.nodeFront.nodeName.toUpperCase() === "BODY";

I'm not sure this is needed to be honest. I'm curious which test HTML page you used that required you to make that change.

nodeName, when the node is an ELEMENT_NODE type defaults to tagName. And inside HTML documents, tagNames are uppercase already.

https://developer.mozilla.org/en-US/docs/Web/API/Element/tagName

::: devtools/client/inspector/inspector-panel.js
@@ +880,5 @@
>      this._toolbox = null;
>      this.search.destroy();
>      this.search = null;
>      this.searchBox = null;
> +    this.currentHighlighterLockedElement = null;

You should also do this:

this.elementLockedHighlighter = null;

@@ +935,5 @@
>                             this.canGetUniqueSelector &&
>                             this.selection.nodeFront.isTreeDisplayed;
>  
> +    let isHighlightable = this.selection.isElementNode() &&
> +                          !this.selection.isHeadNode();

What is the logic for testing if the selection is the <head> node? What about element nodes that are inside <head>?
Only filtering out <head> seems a bit random I think. Why not filter out <script>s or <link>s?
Also, what if we don't support custom highlighters?

I would suggest we do this instead:

let isHighlightable = utils.supportsCustomHighlighters() &&
                      (this.selection.isElementNode() ||
                       this.selection.isTextNode());

The highlighter supports any Element and Text node, so it's safe to do this. The highlighter knows how to handle elements that are hidden already anyway, so we shouldn't care about this here too much.

@@ +938,5 @@
> +    let isHighlightable = this.selection.isElementNode() &&
> +                          !this.selection.isHeadNode();
> +
> +    let hasHighlighterLock = this.currentHighlighterLockedElement ?
> +      this.selection.nodeFront === this.currentHighlighterLockedElement : false;

I don't think you need to test that this.currentHighlighterLockedElement actually exist because it's just a property of an object, so if doesn't exist it will be undefined.

So this would work too:

let hasHighlighterLock = this.currentHighlighterLockedElement === this.selection.nodeFront;

@@ +1718,5 @@
> +    let hasHighlighterLock = this.currentHighlighterLockedElement ?
> +      this.selection.nodeFront === this.currentHighlighterLockedElement : false;
> +
> +    if (hasHighlighterLock) {
> +      this.unlockHighlighterOnElement();

You should do this before removing the node. Both removing the node and unhighlighting the node are asynchronous actions, so there'll be a race condition here.

You could do something like:

deleteNode: Task.async(function* () {
  if (this.selection.nodeFront === this.currentHighlighterLockElement) {
    yield this.unlockHighlighterOnElement();
  }

  // ... and then the rest of the function as it was before ...
}),

        Attachment #8802622 -
        Flags: feedback?(pbrosset) → feedback+

    
    

    
  


  
> I'm not sure this is needed to be honest. I'm curious which test HTML page you used that required you to make that change.
> 
> nodeName, when the node is an ELEMENT_NODE type defaults to tagName. And inside HTML documents, tagNames are uppercase already.

You're right. However it's not like that on the Firefox start page (see attachment). I was using that one to test. Is it not HTML?

    
    

    
  

      
      
      
      

        Attached patch
          
          
        bug959073-patch4.diff (obsolete)
        — 
          — Splinter Review
      

    


  
I uploaded a new patch with the implementation of the latest suggestions (no need to review it again). It doesn't fix the issue with the Del key, however. The point is that pressing Del deletes the node without executing the deleteNode function, but I couldn't find which other function is executed. Can I get some help, here?

Btw, should I start writing the tests too?

        Attachment #8802622 -
        Attachment is obsolete: true
Flags: needinfo?(pbrosset)

    
    

    
  
(In reply to Albert Juhé from comment #53)
> You're right. However it's not like that on the Firefox start page (see
> attachment). I was using that one to test. Is it not HTML?
Ah you're right, in XML-based pages like XHTML (which seems to be the case for about:home), then case is preserved. So body.nodeName is indeed lowercase. So your change does seem required.

(In reply to Albert Juhé from comment #54)
> I uploaded a new patch with the implementation of the latest suggestions (no
> need to review it again). It doesn't fix the issue with the Del key,
> however. The point is that pressing Del deletes the node without executing
> the deleteNode function, but I couldn't find which other function is
> executed. Can I get some help, here?
I see what's happening. There are 2 levels of responsibility here.
The inspector panel, which is the thing that contains all tools inside the inspector, and the markup-view, which is the thing that's only concerned about the DOM tree inside the inspector.

The context menu is the responsibility of the inspector-panel. So, when you click on the "delete node" item in this menu, that calls the "deleteNode" function inside the inspector-panel.

However, the delete (or backspace) key bindings are the responsibility of the markup-view, so when you press them, the "deleteNode" function inside the markup-view is called only. 

So if you put code to hide the highlighter inside the inspector-panel, then it's only going to be executed when the context menu "delete node" item is clicked on.

As a side note: the context menu is at the inspector-panel level because it used to be available from several places, not just from the markup-view, and so it made sense to have it there, at a higher level, so it could be reused by several different things. Now, it's only used by the markup-view, so it could be moved into the markup-view, so we wouldn't have that problem.
But don't do this here, this is outside of your bug.

My suggestion would be to move your code changes to the markup-view (so to devtools/client/inspector/markup/markup.js) instead. So the markup-view would be responsible for instantiating the highlighter and showing and hiding it. It would also be responsible for hiding it in the deleteNode function.
The inspector-panel would still worry about the context menu item, but when deleteNode is called, because it just calls through to the markup-view's deleteNode, then the highlighter would be hidden.

> Btw, should I start writing the tests too?
Yes that'd be great!
I think a new test file would be ideal here.
Something inside the devtools\client\inspector\test test directory. I think a file named browser_inspector_menu-07-highlighter-lock.js would be great.
This test should check the visibility of the menu item, depending on the element where the context click happened, and should also check that when the item is clicked, the highlighter does appear, etc.
Flags: needinfo?(pbrosset)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        bug959073-patch5.diff (obsolete)
        — 
          — Splinter Review
      

    


  
I uploaded a new version of the patch with the changes from comment 55 and the tests. browser_inspector_menu-01-sensitivity.js includes the test for menu item visibility and browser_inspector_menu-07-highlighter-lock.js tests for lock/unlock/delete the highlighter.

There is still one edge case where there are problems: when you lock the highlighter on an element (ie: #logo) and then that node is removed by JavaScript (ie: $('#logo').remove()).

        Attachment #8804840 -
        Attachment is obsolete: true

        Attachment #8813524 -
        Flags: feedback?(pbrosset)

    
    

    
  
Comment on attachment 8813524 [details] [diff] [review]
bug959073-patch5.diff

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

Looks great.

I have made a few comments in the test mostly.
I had to rebase this patch to apply it to an up to date version of mozilla-central, because code has changed. I will upload my rebased version to avoid you having to do it again, but you might want to make sure you are up to date too (hg pull -u).

I'm not yet giving R+ simply based on the fact that there are still a few comments to address. But I think this patch is mostly ready.

Now, I think we will need a second patch on top of this. The first one implemented the core functionality, now I think we need another one to add in a little bit of a better UX. I think we need to think about 3 things:
- should the locked highlighter be hidden in certain cases: for instance, when another highlighter is shown, it might make the page too busy? But having 2 highlighted elements might be useful if you're trying to compare something between 2 nodes. Maybe when you click on the picker icon, then the lock should be hidden until you select an element?
- should there be other cases where the lock is removed automatically (like we do when deleting the element)?
- should there be a way to remember which element is locked in the inspector. You might be expanding/collapsing nodes and scrolling around in the inspector and, as a result, lose the position of the locked node. Right now, we have something called pseudo-class locks (:hover, ...) which, when used, displays a little icon in the inspector's gutter. I think we might want to do the same thing here.

These are UX questions that I would like us to resolve before closing this bug.
Please let me know your thoughts, and we should also ask :helenvholmes to give hers.

::: devtools/client/inspector/markup/markup.js
@@ +835,5 @@
>    _isInputOrTextarea: function (element) {
>      let name = element.tagName.toLowerCase();
>      return name === "input" || name === "textarea";
>    },
> +  /**

nit: Please insert a new line before this comment.

@@ +866,5 @@
> +
> +  /**
> +   * Lock highlighter on the selected element
> +   */
> +  lockHighlighterOnElement: Task.async(function* () {

nit: maybe rename this to lockHighlighterOnSelection.
The current name implies that you can pass an element as a parameter, which you can't.

@@ +868,5 @@
> +   * Lock highlighter on the selected element
> +   */
> +  lockHighlighterOnElement: Task.async(function* () {
> +    let selection = this._inspector.selection;
> +    if (!selection.isElementNode()) {

Remember, the highlighter also supports text nodes, not just element nodes.

@@ +887,5 @@
> +
> +  /**
> +   * Unlock highlighter from the selected element
> +   */
> +  unlockHighlighterOnElement: Task.async(function* () {

Also change the name of this function like the other one.

::: devtools/client/inspector/test/browser_inspector_menu-07-highlighter-lock.js
@@ +16,5 @@
> +  yield testHighlighterUnlock();
> +
> +  // Lock/Manually Delete
> +  yield testHighlighterLock();
> +  yield testHighlighterUnlockOnDeleteNode();

Can you also add another set of steps where you lock the highlighter on one element, then lock it on another one and test that the first one has been unlocked.

@@ +19,5 @@
> +  yield testHighlighterLock();
> +  yield testHighlighterUnlockOnDeleteNode();
> +
> +  function* testHighlighterLock(test) {
> +    info("Test that highlighter lock is set " + 

nit: trailing whitespace here. For info, we have a set of eslint rules that can help you prevent this kind of minor formatting problems. Please take a look at this doc: https://wiki.mozilla.org/DevTools/CodingStandards

@@ +28,5 @@
> +      label: "inspectorLockHighlighterOnElement.label"
> +    });
> +
> +    is(inspector.markup.currentHighlighterLockedElement, container.node,
> +      'Highlighter is locked on element');

nit: Please use double quotes around this string.

@@ +32,5 @@
> +      'Highlighter is locked on element');
> +  }
> +
> +  function* testHighlighterUnlock(test) {
> +    info("Test that highlighter lock is removed " + 

trailing whitespace here too.

@@ +46,5 @@
> +  }
> +
> +  function* testHighlighterUnlockOnDeleteNode() {
> +    info("Test that highlighter lock is removed when the node is deleted.");
> +    

trailing whitespace here too.

@@ +51,5 @@
> +    yield selectNode("#lock-highlighter", inspector);
> +    let allMenuItems = openContextMenuAndGetAllItems(inspector);
> +    let menuItem = allMenuItems.find(i => i.id === "node-menu-delete");
> +    menuItem.click();
> +    

trailing whitespace here too.

@@ +52,5 @@
> +    let allMenuItems = openContextMenuAndGetAllItems(inspector);
> +    let menuItem = allMenuItems.find(i => i.id === "node-menu-delete");
> +    menuItem.click();
> +    
> +    yield inspector.once("inspector-updated");

Please listen to the event before clicking:

let onUpdated = inspector.once("inspector-updated");
menuItem.click();
yield onUpdated;

@@ +58,5 @@
> +    is(inspector.markup.currentHighlighterLockedElement, null,
> +      'Highlighter is not locked anywhere');
> +  }
> +
> +  function* testHighlighterClick(test) {

The name of this function could be a bit better I think. Something like clickAndCheckHighlighterMenuItem

@@ +59,5 @@
> +      'Highlighter is not locked anywhere');
> +  }
> +
> +  function* testHighlighterClick(test) {
> +    let { event, label } = test;

It's only 2 parameters, no I don't think it's really necessary to pass in an object here. But up to you.
It could simply be:
function* clickAndCheckHighlighterMenuItem(event, label)

Or if you keep the object, you can avoid this line with:
function* clickAndCheckHighlighterMenuItem({ event, label })

@@ +65,5 @@
> +    let allMenuItems = openContextMenuAndGetAllItems(inspector);
> +    let menuItem = allMenuItems.find(i => i.id === "node-menu-lock-highlighter");
> +    menuItem.click();
> +
> +    yield inspector.markup.once(event);

Please listen to the event before clicking:

let onEvent = inspector.markup.once(event);
menuItem.click();
yield onEvent;

        Attachment #8813524 -
        Flags: feedback?(pbrosset) → feedback+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        bug959073-rebased.diff (obsolete)
        — 
          — Splinter Review
      

    


  
Here's the rebased version.

        Attachment #8813524 -
        Attachment is obsolete: true

        Attachment #8814438 -
        Flags: feedback+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        bug959073-rebased.diff (obsolete)
        — 
          — Splinter Review
      

    


  
Sorry, here's the correct one.

        Attachment #8814438 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        bug959073-patch6.diff (obsolete)
        — 
          — Splinter Review
      

    


  
New version of the patch with the suggestions applied.

I had to split the deleteNode function from devtools/client/inspector/markup/markup.js into two functions.

I was getting this error in ES Lint:
> Too many nested callbacks (4). Maximum allowed is 3.

Please, review it because I was not sure what name/description I had to give to the new function.

        Attachment #8814440 -
        Attachment is obsolete: true

        Attachment #8819000 -
        Flags: review?(pbrosset)

    
    

    
  


  
My five cents about this:

> - should the locked highlighter be hidden in certain cases: for instance, when another
> highlighter is shown, it might make the page too busy? But having 2 highlighted
> elements might be useful if you're trying to compare something between 2 nodes. Maybe
> when you click on the picker icon, then the lock should be hidden until you select an
> element?

My answer here is absolutely no! From my point of view, locking the highlighter is specially useful if you can have another highlighter at the same time so you can visually see if two elements are aligned (see screenshot). Maybe we should remove the infobar, however.

> - should there be other cases where the lock is removed automatically (like we do when
> deleting the element)?

Can't think about any. But as I said in the previous comment, if the element with the highlighter is removed via JavaScript, the highlighter will stay there with no easy way to remove it.

> - should there be a way to remember which element is locked in the inspector. You might
> be expanding/collapsing nodes and scrolling around in the inspector and, as a result,
> lose the position of the locked node. Right now, we have something called pseudo-class
> locks (:hover, ...) which, when used, displays a little icon in the inspector's gutter.
> I think we might want to do the same thing here.

Sounds good. Maybe with another color/shape so the user can distinguish them? What if an element has the highlighter and the pseudo-class locked at the same time?

    
    

    
  
(In reply to Albert Juhé from comment #61)
> > - should there be a way to remember which element is locked in the inspector. You might
> > be expanding/collapsing nodes and scrolling around in the inspector and, as a result,
> > lose the position of the locked node. Right now, we have something called pseudo-class
> > locks (:hover, ...) which, when used, displays a little icon in the inspector's gutter.
> > I think we might want to do the same thing here.
> 
> Sounds good. Maybe with another color/shape so the user can distinguish
> them? What if an element has the highlighter and the pseudo-class locked at
> the same time?

A good start would be making the rule view highlighter lock icon next to "element" match the state of the context menu (blue if highlight is enabled/gray if not).

Also, if you're adding another markup-view indicator, please consider bug 1205371.

    
    

    
  
Comment on attachment 8819000 [details] [diff] [review]
bug959073-patch6.diff

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

Alright, this looks pretty good now, I'm R+'ing this patch. However, as I said before, we need another patch to work on the usability for this.
In particular, I think this feature should be linked to the highlighter lock feature in the rule-view (when you click on the small inspector icon next to selectors in the rule-view). See comment 62
> making the rule view highlighter lock icon next to "element" match the state of the context menu

I think :gl has some ideas about how to do this (he's done some refactoring on the rule-view highlighters recently, so he should be able to point you in the right direction).

::: devtools/client/inspector/inspector.js
@@ +1025,5 @@
>                             this.selection.nodeFront.isTreeDisplayed;
> +    let isHighlightable =
> +      this._toolbox.highlighterUtils.supportsCustomHighlighters() &&
> +      (this.selection.isElementNode() ||
> +      this.selection.isTextNode());

nit: our line maxlen is 90 characters, and it seems that this test would fit on 1 line, no need to wrap it:

let isHighlightable = 
  this._toolbox.highlighterUtils.supportsCustomHighlighters() &&
  (this.selection.isElementNode() || this.selection.isTextNode());

::: devtools/client/inspector/markup/markup.js
@@ +875,5 @@
> +    if (!highlighter) {
> +      return;
> +    }
> +
> +    yield highlighter.show(selection.nodeFront);

As you said, we might want to hide a few things when locking the highlighter on an element.
In particular, we hide the nodeinfobar and the guides when locking the highlighter on elements from the rule-view. For consistency reason, I would do the same here:

      yield highlighter.show(selection.nodeFront, {
        hideInfoBar: true,
        hideGuides: true
      });

@@ +928,5 @@
>     *         The node to remove.
>     * @param  {Boolean} moveBackward
>     *         If set to true, focus the previous sibling, otherwise the next one.
>     */
> +  deleteNode: Task.async(function* (node, moveBackward) {

You're getting ESLint's warning about too many nested callback because we changed this function to a Task.async. And Task.async takes a function as its parameter, so ESLint counts this as a callback. And because the function already had 3 nested callbacks, we now have 4.

There might be a nicer way to solve this without having to split it in 2 separate functions:

Now that you are using Task.async, any async function that returns a promise can be called with `yield` instead of chaining a callback with `.then(`.

So you could change this:
this.walker.retainNode(node).then(() => {

To this:
yield this.walker.retainNode(node);

No need for .then anymore, and you can just have the rest starting on the next line.
This way you get rid of one level of callback, and ESLint should be happy.

        Attachment #8819000 -
        Flags: review?(pbrosset) → review+

    
    

    
  
Gabriel, see comment 62 and the top of comment 63 please. Your help on this would be awesome!
Looks like we should maybe make the highlighter-overlay thing a more central helper that the markup-view can also use.
Flags: needinfo?(gl)

    
    

    
  
So, the highlighter-overlay should probably just be the central place for all highlighters in the inspector for initializing, displaying and destroying. The goal I want for the highlighter-overlay is to be a main store of the highlighters shown so we can use that state to maintain the UI of the various items in the inspector like the selector highlighter toggle. 

Looking at highlighters-overlay.js and adding a new highlighterShown that keeps of this lock would probably get you started.
Flags: needinfo?(gl)

    
    

    
  


  
I uploaded a new version of the patch with the changes from Comment 63. It would be great if you could review it again, specially the part where I replace the .then() callback with the yield, since I was not sure what to do with the second .then(). (Sorry, it's my first time working with some of these JS features).

>      yield highlighter.show(selection.nodeFront, {
>        hideInfoBar: true,
>        hideGuides: true
>      });

I know this is how it works in the rule view highlighter lock and probably this is not the best place to discuss this, but do we really want to hide the guides? I can see many use cases where it's more useful to lock the guides than locking the highlighter for itself.

I'm just starting to take a look at highlighters-overlay.js. I hope to upload the second patch in the following days. :)

        Attachment #8819000 -
        Attachment is obsolete: true

        Attachment #8820018 -
        Flags: review?(pbrosset)

    
    

    
  
Comment on attachment 8820018 [details] [diff] [review]
bug959073-patch71.diff

I'm really sorry about the delay reviewing. I'm only just back from a break and starting to look at reviews again.
I think it makes more sense if Gabriel does the reviews for this moving forward since it makes sense to integrate this feature with the highlighter-overlay as much as possible, and Gabriel has been working on this a lot recently.

        Attachment #8820018 -
        Flags: review?(pbrosset) → review?(gl)

    
    

    
  
Comment on attachment 8820018 [details] [diff] [review]
bug959073-patch71.diff

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

Hi Albert,

Thanks for the patch. While it looks good, we need to make sure it is behaving appropriately with some UI indicator that the element locked highlighter is turned. We should moving the highlighter logic into highlighters-overlay.js.

See my comments below and if you need further instructions please ping me at any time and I will get back to you.

Thanks,
Gabriel

::: devtools/client/framework/selection.js
@@ -219,5 @@
>     */
>    isBodyNode: function () {
>      return this.isHTMLNode() &&
>             this.isConnected() &&
> -           this.nodeFront.nodeName === "BODY";

I am wondering if this is really needed since it seems to always be upper case.

::: devtools/client/inspector/markup/markup.js
@@ +838,5 @@
> +   * this function is called. The same instance will then be returned.
> +   *
> +   * @return {Promise} Resolves to the instance of the highlighter.
> +   */
> +  getElementLockedHighlighter: Task.async(function* () {

This should probably go into highlighers-overlay.js. We want the highlighters-overlay to be  the 'store' which keeps track of the highlighter states in the inspector. If we initialize a highlighter, it should be done here, and the state of that should be stored there as well. We want to make sure this state is communicated to the front end as well to ensure that we are clearing/hiding the highlighters on page navigations and the appropriate front end highlighter indicator is active (this would be the highlighter button next to element { } in the rule view).

::: devtools/client/inspector/test/browser_inspector_menu-07-highlighter-lock.js
@@ +1,4 @@
> +/* vim: set ts=2 et sw=2 tw=80: */
> +/* Any copyright is dedicated to the Public Domain.
> +http://creativecommons.org/publicdomain/zero/1.0/ */
> +"use strict";

Needs a new line above "use strict";

@@ +2,5 @@
> +/* Any copyright is dedicated to the Public Domain.
> +http://creativecommons.org/publicdomain/zero/1.0/ */
> +"use strict";
> +
> +// Tests "Lock Highlighter" menu item

Needs a new line below the test comment

        Attachment #8820018 -
        Flags: review?(gl)

    
  
Product: Firefox → DevTools
Assignee: aljullu → nobody
Status: ASSIGNED → NEW
Mentor: patrickbrosset+bugzilla

    
  
Severity: normal → S3

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: