Highlight all nodes that match a given selector in the rule-view and style-editor

RESOLVED FIXED in Firefox 34

Status

defect
RESOLVED FIXED
5 years ago
5 months ago

People

(Reporter: pbro, Assigned: pbro)

Tracking

(Depends on 4 bugs)

unspecified
Firefox 34
Dependency tree / graph

Firefox Tracking Flags

(relnote-firefox 34+)

Details

(Whiteboard: [dev-docs-needed])

Attachments

(4 attachments, 19 obsolete attachments)

198.03 KB, image/png
Details
23.50 KB, patch
pbro
: review+
pbro
: checkin+
Details | Diff | Splinter Review
25.88 KB, patch
pbro
: review+
pbro
: checkin+
Details | Diff | Splinter Review
32.70 KB, patch
pbro
: review+
pbro
: checkin+
Details | Diff | Splinter Review
Assignee

Description

5 years ago
This bug both belongs to the |Developer Tools: Inspector| component and the |Developer Tools: Style Editor| component, unless we want to file 2 separate bugs.

The goal here is make our HighlighterActor support multiple highlighter instances.
Today, it only provides a single showBoxModel method which will highlight one element only until hideBoxModel is called.

Making it possible to highlight several elements at once would be super useful in the following cases:
- mouseover a selector in the rule-view would highlight all matching elements
- mouseover a selector in the style editor would do the same
- running a gcli command like `highlight p.class` would do the same
Assignee

Updated

5 years ago
Assignee: nobody → pbrosset
Assignee

Comment 1

5 years ago
Joe, we talked about that the other day. 
This is a rough WIP patch which only adds mouseover/out multi-highlight to the rule-view for now.
What do you think (functionally, not of the code, cause it's a mess right now)?
Attachment #8374749 - Flags: feedback?(jwalker)
Assignee

Updated

5 years ago
Summary: highlight matched nodes selector → Highlight all nodes that match a given selector in the rule-view, style-editor and gcli command
Assignee

Comment 2

5 years ago
Heather, would you mind advising me on how this would work with the style-editor?
I know it uses code-mirror, wraps into the source editor interface, but I don't know who to go about adding mouseover/out listeners to selectors.
Thanks for your help.
Flags: needinfo?(fayearthur)
Assignee

Comment 3

5 years ago
The style-editor part depends on bug 971702
Depends on: 971702
Flags: needinfo?(fayearthur)
Assignee

Comment 5

5 years ago
Depends on bug 663778 so I can reuse the box-model highlighter to highlight the content area of each matched node.
Depends on: 663778
Assignee

Updated

5 years ago
Blocks: 971723
Assignee

Comment 6

5 years ago
Making this bug solely about highlighting nodes when hovering over selectors in the style editor and rule view. 
See bug 971723 for a similar feature in gcli.
Summary: Highlight all nodes that match a given selector in the rule-view, style-editor and gcli command → Highlight all nodes that match a given selector in the rule-view and style-editor
Assignee

Comment 7

5 years ago
Video teaser 2 (this one uses the BoxModelHighlighter code from bug 663778): http://www.youtube.com/watch?v=NXXHi99XQmg
Assignee

Comment 8

5 years ago
Ok, still a WIP, but a lot cleaner.
- This patch reuses the BoxModelHighlighter (introducing new options so we can now control whether guides are shown and which region is drawn),
- the toolbox's highlighterUtils is the client-side entry point to this, so that any tool can use this feature,
- older targets that don't have a remote highlighter will not crash,
- b2g/fennec targets that can't attach the BoxModelHighlighter will get the simple outline instead,
- there is now a limit to the number of results returned by querySelectorAll. Indeed, creating many BoxModelHighlighter instances and their associated SVG markup is costly in terms of processing time and is noticeably slow in the UI (the highlighters won't show up immediately on certain very general selectors).

==> For the last point, either we live with this limitation (a random const I've added in the highlighterActor file), or we find a better way of highlighting many nodes at once.
Attachment #8374749 - Attachment is obsolete: true
Attachment #8374749 - Flags: feedback?(jwalker)
Assignee

Comment 9

5 years ago
When it comes to highlighting nodes in the style-editor, we depend on the API that is worked on in bug 971702. This bug should land pretty soon.
Since the API has to parse the stylesheet text, and since we want to highlight on mousemove/mouseover, we might need to run some performance related tests, and perhaps cache the API's returned values to avoid calling it again and again when the mouse moves over the stylesheet.
Assignee

Updated

5 years ago
Depends on: 1014547
Assignee

Updated

5 years ago
Depends on: 1020291
Assignee

Comment 10

5 years ago
Works on top of the patch in bug 1020291.
Should be a lot better performance-wise, especially when the selector matches a lot of nodes.
Attachment #8374924 - Attachment is obsolete: true
Assignee

Comment 12

5 years ago
Removes some useless references in the highlighter actor
Assignee

Comment 13

5 years ago
gcli command to highlight nodes.
Assignee

Comment 14

5 years ago
Here's a patch which will highlight nodes as you type a GCLI parameter of type 'node'.

Joe, I need some feedback about this one. In particular, here are the points I have in mind:
- First of all, I find this very cool, it definitely helps when entering commands and the code that you had prepared for this was great, I really only had to fill the blanks.
- The SelectorHighlighter I have create in patch part 1 isn't really adapted for this though. It takes a String selector as argument, not just a node. So for now, in the patch, I've used the BoxModelHighlighter. But it's not really adapted for the task I think (it shows too much information). One solution is to change the 'node' type and the Highlighter in gcli/host.js to pass the selector as a string instead of the nodeList, and let SelectorHighlighter do the rest.
- In order to initialize the SelectorHighlighter, I need some information from the TabActor. Really I just need 'browser' and 'window', which is easily found from the context object that is passed to commands exec functions, so I had to pass it from inputter to the node's onEnter function. Is there a way to get this information without having to do this?
Attachment #8442048 - Flags: feedback?(jwalker)
Assignee

Comment 15

5 years ago
Cleaned up part 3 patch a little bit and removed unrelated code changes.

Joe, this patch makes use of the new SelectorHighlighter class added in part 1.
It works somewhat like what I did in part 4, but introduces a new command.

Your feedback on this patch would help.
Attachment #8441987 - Attachment is obsolete: true
Attachment #8442052 - Flags: feedback?(jwalker)
Assignee

Updated

5 years ago
Duplicate of this bug: 971723
Assignee

Comment 17

5 years ago
part 1 again, fixed several bugs.
Attachment #8441983 - Attachment is obsolete: true
Comment on attachment 8442052 [details] [diff] [review]
bug971662-part3-gcli-cmd-highlight-selector-matches v2.patch

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

::: toolkit/devtools/gcli/commands/highlight.js
@@ +20,5 @@
> +    manual: gcli.lookup("highlightManual"),
> +    params: [
> +      {
> +        name: "selector",
> +        type: "string",

We should fix the single-document problem with the current node type so we can use it here. Lets talk about how/when to do this.

@@ +41,5 @@
> +      // Build a tab context for the highlighter (which normally takes a
> +      // TabActor as parameter to its constructor)
> +      let gBrowser = context.environment.chromeDocument.defaultView.gBrowser;
> +      let tabContext = {
> +        browser: gBrowser.selectedBrowser,

We're making the assumption that the selected browser is the one we're targetting. I think it would be better to use getBrowserForDocument() wouldn't it? i.e. https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Method/getBrowserForDocument
Attachment #8442052 - Flags: feedback?(jwalker) → feedback+
Comment on attachment 8442048 [details] [diff] [review]
bug971662-part4-gcli-highlight-node-types-while-typing v1.patch

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

::: toolkit/devtools/gcli/source/lib/gcli/types/node.js
@@ +76,5 @@
>    assignment.highlighter.nodelist = assignment.conversion.matches;
>  }
>  
>  /** @see #onEnter() */
>  function onLeave(assignment) {

My gut reaction is that we should have some symmetry between onEnter and onLeave, so we should add context to onLeave too.

::: toolkit/devtools/gcli/source/lib/gcli/util/host.js
@@ +24,5 @@
>  var Promise = require('../util/promise').Promise;
>  var util = require('./util');
>  
> +require("devtools/server/actors/inspector");
> +var {HIGHLIGHTER_CLASSES} = require("devtools/server/actors/highlighter");

Nit on these 2 lines, which I would ignore except we're probably updating this patch anyway: GCLI uses '

@@ +32,4 @@
>    this._document = document;
>    this._nodes = util.createEmptyNodeList(this._document);
> +
> +  let gBrowser = context.environment.chromeDocument.defaultView.gBrowser;

In theory GCLI shouldn't care about the contents of environment because it's defined by the environment. But here host is synonymous with the environment, so it feels ok. Except that passing the context into onEnter/onLeave admits to the burring.

I think we should just ignore it for now. But it's bugging me :)
Attachment #8442048 - Flags: feedback?(jwalker) → feedback+
Comments from having a play.

Firstly, I'm in love. It's super awesome.

Even things like 'inspect *' which I thought was going to be pointless turn out to be interesting, answering questions like:

- what lines up with what?
- where is the page dense with nodes?
- how is that thing made up?

I wonder if it's obvious how to turn of highlighting. Maybe the 'highlight' command should have output like "15 nodes highlighted. Type 'highlight' to turn off highlighting".

I wonder if we shouldn't use box model highlighing for multiple nodes up to some limit. Maybe at 50 nodes it goes into blue-box mode or something.

It would be good to have the node info bars float above the box models.

highlight on hover in the rule view could be confusing when someone hovers over selectors like 'html'. If you don't know what's going on - suddenly the page flashes blue for no apparent reason. Not sure how much of a problem that is. Maybe one people get used to the extra level of hover that we're doing it won't be a surprise. Try www.mozilla.org and hover the big reset rule for example.

I'd love this in the style editor.
Also, much console spew while testing.

System JS : ERROR resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/highlighter.js:391 - TypeError: event.clientRects is undefined

console.error:
DOMException
    - ...
    - filename = resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/inspector.js
    - inner = null
    - lineNumber = 1535

console.error:
  Message: TypeError: this.nodeList is null
  Stack:
    exports.NodeListActor<.form@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/inspector.js:756:7
Assignee

Comment 22

5 years ago
Thanks for trying it out.
(In reply to Joe Walker [:jwalker] from comment #20)
> Comments from having a play.
> 
> Firstly, I'm in love. It's super awesome.
> 
> Even things like 'inspect *' which I thought was going to be pointless turn
> out to be interesting, answering questions like:
> 
> - what lines up with what?
> - where is the page dense with nodes?
> - how is that thing made up?
Interesting idea indeed. I was originally tempted to only use the blue box for multiple highlighters. But if guides actually help in this situation, then we'd better keep them.
> I wonder if it's obvious how to turn of highlighting. Maybe the 'highlight'
> command should have output like "15 nodes highlighted. Type 'highlight' to
> turn off highlighting".
Good idea. will do this.
> I wonder if we shouldn't use box model highlighing for multiple nodes up to
> some limit. Maybe at 50 nodes it goes into blue-box mode or something.
Yeah, that's why I thought about only using the blue-box, but as said before, if the box-model helps solving some use cases, we could do as you said, limit it to some manageable number.
> It would be good to have the node info bars float above the box models.
I've had cases where infobars would cover other infobars, making it really hard to read. I agree the information in the infobar would help though. I'll experiment some more.
> highlight on hover in the rule view could be confusing when someone hovers
> over selectors like 'html'. If you don't know what's going on - suddenly the
> page flashes blue for no apparent reason. Not sure how much of a problem
> that is. Maybe one people get used to the extra level of hover that we're
> doing it won't be a surprise. Try www.mozilla.org and hover the big reset
> rule for example.
Very good point. One idea would be to have an inspector icon next to the selector instead. And maybe highlighting would only be done when clicking the icon? Or maybe we should introduce the command only first.
> I'd love this in the style editor.
Planned, it should be relatively easy now that we have a source-editor API that gives us information about hovered nodes.
Assignee

Comment 23

5 years ago
Thinking a little bit more about this, I think highlighting nodes based on a selector is more useful in the style-editor and as a command. I'm tempted to say the rule-view use case is less interesting because the view corresponds to a node that was selected, so that's the node you care about.

The style-editor use case is more interesting because you don't have that node context. You just have styles and you may be interested in knowing which nodes does a selector correspond to.

The command use case is really nice because it allows to highlight nodes and keep them highlighted while doing something else (lining up nodes using the rule-view, ...).
Assignee

Comment 24

5 years ago
(In reply to Joe Walker [:jwalker] from comment #18)
> Comment on attachment 8442052 [details] [diff] [review]
> bug971662-part3-gcli-cmd-highlight-selector-matches v2.patch
> 
> Review of attachment 8442052 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/devtools/gcli/commands/highlight.js
> @@ +20,5 @@
> > +    manual: gcli.lookup("highlightManual"),
> > +    params: [
> > +      {
> > +        name: "selector",
> > +        type: "string",
> 
> We should fix the single-document problem with the current node type so we
> can use it here. Lets talk about how/when to do this.
Agreed, this way nodes would be highlighted as you type and would remain highlighted when the command is committed. I guess I can even use the node type right now, even before highlight-as-you-type is done.
> @@ +41,5 @@
> > +      // Build a tab context for the highlighter (which normally takes a
> > +      // TabActor as parameter to its constructor)
> > +      let gBrowser = context.environment.chromeDocument.defaultView.gBrowser;
> > +      let tabContext = {
> > +        browser: gBrowser.selectedBrowser,
> 
> We're making the assumption that the selected browser is the one we're
> targetting. I think it would be better to use getBrowserForDocument()
> wouldn't it? i.e.
> https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Method/
> getBrowserForDocument
Done. Thanks.
Assignee

Comment 25

5 years ago
Independent patch that makes the BoxModelHighlighter more configurable. It now supports:
- hiding/showing the guides
- hiding/showing the infobar
- choosing just one region to be highlighted
- choosing which region the guides should surround (this was already in place).

I've changed a little bit the markup-view code to remove unnecessary option objects that were passed, for nothing.

Note that this patch doesn't use these new options. It's only required for the other patches I will add to this bug.

Ongoing try build: https://tbpl.mozilla.org/?tree=Try&rev=dcff44c775f5
Attachment #8442817 - Flags: review?(mratcliffe)
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #26)
> https://www.youtube.com/watch?v=EZ-nWDRht68

Nice, I hadn't thought of that as the primary use-case but that looks very helpful!  I wonder what the best way to expose that in the UI would be.  Maybe we could have a tiny toolbar at the top of the rule view that lets you toggle hover/active/focus and also lock the highlighting (on this element only - if you want to do it for an arbitrary selector maybe you will use the toolbar or style editor).
Assignee

Comment 28

5 years ago
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #25)
> Created attachment 8442817 [details] [diff] [review]
> bug971662-part0-config-options-for-boxmodelhighlighter v1.patch
> Ongoing try build: https://tbpl.mozilla.org/?tree=Try&rev=dcff44c775f5
I don't know what happened with that try build link, but results are not showing up.
Here's a new one for part0: https://tbpl.mozilla.org/?tree=Try&rev=950af71a22f0
Assignee

Comment 29

5 years ago
(In reply to Brian Grinstead [:bgrins] from comment #27)
> Maybe we could have a tiny toolbar at the top of the rule view that lets you
> toggle hover/active/focus and also lock the highlighting (on this element
> only - if you want to do it for an arbitrary selector maybe you will use the
> toolbar or style editor).
If we want to rework the pseudo-classes UI as a toolbar in the rule-view, then yeah, that would be a pretty good place to have a button to lock the highlighter.
Another (admittedly less discoverable) way would be in the right-click ctx menu. The good thing here though is that the menu could have both "lock highlighter on current element" AND "lock highlight for this selector" items.
And yeah, highlighting on style-editor selector hover (or click/ctx-menu/whatever) is definitely planned in this bug.
Assignee

Comment 30

5 years ago
Marking the original part1 - rule-view highlight on hover as obsolete for now. Let's focus on the gcli command and the style-editor for now.
Also marking part2 as obsolete, I moved this standalone small cleanup into bug 1028043.

Joe, here's the new patch for the highlight command.

- It now has a bunch of options that let users decide exactly how to highlight. I hope the default values make sense so that you don't really have to use the options.

- The highlighter is dismissed on page navigation.

- A maximum of 100 nodes will be highlighted at the same time (unless the --showall option is passed).

- It won't work for nodes in iframes, but that shouldn't be solved here as discussed, but rather in part4 where I'll try and make the "node" and "nodelist" types aware of all frames.

Could you give me pointers on how to write new tests for the command?
Attachment #8441986 - Attachment is obsolete: true
Attachment #8442052 - Attachment is obsolete: true
Attachment #8442093 - Attachment is obsolete: true
Attachment #8443315 - Flags: review?(jwalker)
Assignee

Comment 31

5 years ago
Fixed a typo in the properties file
Attachment #8443315 - Attachment is obsolete: true
Attachment #8443315 - Flags: review?(jwalker)
Attachment #8443317 - Flags: review?(jwalker)
Comment on attachment 8443317 [details] [diff] [review]
bug971662-part1-gcli-highlight-command v4.patch

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

::: toolkit/devtools/gcli/commands/highlight.js
@@ +13,5 @@
> +// How many maximum nodes can be highlighted in parallel
> +const MAX_HIGHLIGHTED_ELEMENTS = 100;
> +
> +// Stores the highlighters instances so they can be destroyed
> +let highlighters = [];

I'm wondering if it's good that there are these stores of highlighters tucked away in all corners of our code. It makes a command like "clearhighlighters" very hard. I'm not sure if that's a useful command though, and failure to think of something more significant makes me think it's not a big problem.

@@ +63,5 @@
> +          {
> +            name: "region",
> +            type: {
> +               name: "selection",
> +               data: ["content", "padding", "border", "margin"]

Indent nit. Ignore unless you're changing the patch anyway
Attachment #8443317 - Flags: review?(jwalker) → review+
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #30)
> Created attachment 8443315 [details] [diff] [review]
> bug971662-part1-gcli-highlight-command v3.patch
> 
> Marking the original part1 - rule-view highlight on hover as obsolete for
> now. Let's focus on the gcli command and the style-editor for now.
> Also marking part2 as obsolete, I moved this standalone small cleanup into
> bug 1028043.
> 
> Joe, here's the new patch for the highlight command.
> 
> - It now has a bunch of options that let users decide exactly how to
> highlight. I hope the default values make sense so that you don't really
> have to use the options.

Makes sense to me.

At the risk of option overload, and just thinking aloud ...

* I was wondering if an option to change the color of the content region would be useful. You have an element and it shouldn't be visible so you do "highlight #thing --region content --contentcolor red" and then use the page, and are alerted to red bits right away.

* Also "highlight --keep #otherthing" which keeps the existing highlighters, so now you can track 2 things and have them easily separated by color.

I suspect this is a step too far

> - It won't work for nodes in iframes, but that shouldn't be solved here as
> discussed, but rather in part4 where I'll try and make the "node" and
> "nodelist" types aware of all frames.
> 
> Could you give me pointers on how to write new tests for the command?

I hope that a combination of an example [1] and the doc comments [2] should get you all you need. If not, it's a documentation bug and you should yell at me until I fix it.

[1]: http://dxr.mozilla.org/mozilla-central/source/browser/devtools/commandline/test/browser_cmd_cookie.js
[2]: http://dxr.mozilla.org/mozilla-central/source/browser/devtools/commandline/test/helpers.js#1103
Assignee

Comment 34

5 years ago
(In reply to Joe Walker [:jwalker] from comment #32)
> Comment on attachment 8443317 [details] [diff] [review]
> bug971662-part1-gcli-highlight-command v4.patch
> 
> Review of attachment 8443317 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/devtools/gcli/commands/highlight.js
> @@ +13,5 @@
> > +// How many maximum nodes can be highlighted in parallel
> > +const MAX_HIGHLIGHTED_ELEMENTS = 100;
> > +
> > +// Stores the highlighters instances so they can be destroyed
> > +let highlighters = [];
> 
> I'm wondering if it's good that there are these stores of highlighters
> tucked away in all corners of our code. It makes a command like
> "clearhighlighters" very hard. I'm not sure if that's a useful command
> though, and failure to think of something more significant makes me think
> it's not a big problem.
Agreed, though I don't know how to make this better either.
> @@ +63,5 @@
> > +          {
> > +            name: "region",
> > +            type: {
> > +               name: "selection",
> > +               data: ["content", "padding", "border", "margin"]
> 
> Indent nit. Ignore unless you're changing the patch anyway
Done.


(In reply to Joe Walker [:jwalker] from comment #33)
> (In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #30)
> > Created attachment 8443315 [details] [diff] [review]
> > bug971662-part1-gcli-highlight-command v3.patch
> > 
> > Marking the original part1 - rule-view highlight on hover as obsolete for
> > now. Let's focus on the gcli command and the style-editor for now.
> > Also marking part2 as obsolete, I moved this standalone small cleanup into
> > bug 1028043.
> > 
> > Joe, here's the new patch for the highlight command.
> > 
> > - It now has a bunch of options that let users decide exactly how to
> > highlight. I hope the default values make sense so that you don't really
> > have to use the options.
> 
> Makes sense to me.
> 
> At the risk of option overload, and just thinking aloud ...
> 
> * I was wondering if an option to change the color of the content region
> would be useful. You have an element and it shouldn't be visible so you do
> "highlight #thing --region content --contentcolor red" and then use the
> page, and are alerted to red bits right away.
Great idea. I've done it (will upload a patch soon) and it's actually pretty useful.
For instance: `highlight * --fill rgba(255,255,0,.5) --hideguides --showall` gives you a sort of heatmap of your page density (see screenshot).
> * Also "highlight --keep #otherthing" which keeps the existing highlighters,
> so now you can track 2 things and have them easily separated by color.
> 
> I suspect this is a step too far
I like this idea too, but will probably keep it for another bug.
> > - It won't work for nodes in iframes, but that shouldn't be solved here as
> > discussed, but rather in part4 where I'll try and make the "node" and
> > "nodelist" types aware of all frames.
For info, I'm working on another page for this, which seems to work fine so far.
> > Could you give me pointers on how to write new tests for the command?
> 
> I hope that a combination of an example [1] and the doc comments [2] should
> get you all you need. If not, it's a documentation bug and you should yell
> at me until I fix it.
> 
> [1]:
> http://dxr.mozilla.org/mozilla-central/source/browser/devtools/commandline/
> test/browser_cmd_cookie.js
> [2]:
> http://dxr.mozilla.org/mozilla-central/source/browser/devtools/commandline/
> test/helpers.js#1103
Thanks. That should be enough.
Assignee

Comment 35

5 years ago
Posted image page density heatmap
Assignee

Comment 36

5 years ago
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #34)
> > * Also "highlight --keep #otherthing" which keeps the existing highlighters,
> > so now you can track 2 things and have them easily separated by color.
> > 
> > I suspect this is a step too far
> I like this idea too, but will probably keep it for another bug.
Or, we do it this way: existing highlighters are only removed when you call the 'unhighlight' command. This way you can incrementally highlight other nodes, with different colors easily.
The drawback is that it's easier to create a mess of highlighters.
Assignee

Comment 37

5 years ago
I think there are enough changes to this patch to require a new review.
Here are the main changes:

- Moar options!! I think the command remains simple to use, everything's optional anyway, but the output can be customized pretty heavily now for power-users. It does what jwalker suggested: it's possible to keep previous highlighters visible and choose a different fill color.
- I've set the infobar visibility to false by default (there's an option to show it). We can always change this later but in most of my tests, infobars ended up behind other infobars or highlighters, so they were most of the time useless. I was tempted to do the same with guides, but since one of the most probable usecase for this is to line things up, better keep them visible by default.
- I've added a couple of tests
Attachment #8443317 - Attachment is obsolete: true
Attachment #8443476 - Flags: review?(jwalker)
Assignee

Comment 38

5 years ago
part 0 - rebased after highlighter changes just landed
Attachment #8442817 - Attachment is obsolete: true
Attachment #8442817 - Flags: review?(mratcliffe)
Attachment #8443530 - Flags: review?(mratcliffe)
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #35)
> page density heatmap

Nice.
At some stage we should probably have a 'color' type, but not now.
Comment on attachment 8443476 [details] [diff] [review]
bug971662-part1-gcli-highlight-command v5.patch

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

::: browser/devtools/commandline/commands-index.js
@@ +22,5 @@
>    "gcli/commands/paintflashing",
>    "gcli/commands/restart",
>    "gcli/commands/screenshot",
>    "gcli/commands/tools",
> +  "gcli/commands/highlight",

I'm not sure it's worth re-spinning the patch for this - we can fix it next time, but I think we should keep these lists alphabetical because always appending to the list is more likely to cause patch collisions.
Attachment #8443476 - Flags: review?(jwalker) → review+
Assignee

Comment 42

5 years ago
part 1 - rebased and changed the order in commands-index.js to be alphabetical
Attachment #8443476 - Attachment is obsolete: true
Attachment #8444135 - Flags: review+
Comment on attachment 8443530 [details] [diff] [review]
bug971662-part0-config-options-for-boxmodelhighlighter v2.patch

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

Seems simple enough, I like the fact that we are storing the options object as it makes it available throughout.

We should probably have a test though in case this gets broken in the future.
Attachment #8443530 - Flags: review?(mratcliffe)
Assignee

Comment 44

5 years ago
part 0 - v3

- added 2 tests for the box model highlighter
- fixed the svg box-model guides to be crisper
- also made sure the rightGuide is indeed on the right, and leftGuide on the left, to simplify testing.

New try build: https://tbpl.mozilla.org/?tree=Try&rev=1479b468ca7c
Attachment #8443530 - Attachment is obsolete: true
Attachment #8444420 - Flags: review?(mratcliffe)
Assignee

Comment 46

5 years ago
Will require doc changes to https://developer.mozilla.org/en-US/docs/Tools/GCLI
Whiteboard: [dev-docs-needed]
Attachment #8444420 - Flags: review?(mratcliffe) → review+
Assignee

Updated

5 years ago
Attachment #8444420 - Flags: checkin+
Assignee

Comment 48

5 years ago
Sorry, that's the part0 patch that landed. I rebased it just before and forgot to upload it to the bug.
Attachment #8444420 - Attachment is obsolete: true
Attachment #8445023 - Flags: review+
Attachment #8445023 - Flags: checkin+
Assignee

Comment 49

5 years ago
Part 1 - rebased too.
New ongoing try build: https://tbpl.mozilla.org/?tree=Try&rev=9ec2f5e82d89
The previous one was mostly green but also suffered from a few windows opt xpcshell test failures like bug 1024932 (which went away on update), so I'm re triggering a new one for good measure.
Attachment #8444135 - Attachment is obsolete: true
Attachment #8445024 - Flags: review+
Backed out for causing errors in the logs of form:
System JS : ERROR resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/actors/highlighter.js:783 - NS_ERROR_NOT_AVAILABLE
https://tbpl.mozilla.org/php/getParsedLog.php?id=42339010&tree=Fx-Team

Note the runs are still mostly green due to bug 920191, however there is the odd orange that appears as a result:
https://tbpl.mozilla.org/php/getParsedLog.php?id=42341155&tree=Fx-Team

Note this is the same as the one on the try run in this bug:
https://tbpl.mozilla.org/php/getParsedLog.php?id=42268674&tree=Try

remote:   https://hg.mozilla.org/integration/fx-team/rev/42241a890ff1
Assignee

Comment 51

5 years ago
The reason why these error logs would show up with that patch applied is eluding me.
The code that throws the error was here before, and my patch doesn't change the way it is called at all.
Having said that, I think I know why it does fail and will try and provide a fix in my patch.
Assignee

Comment 52

5 years ago
Trigerring many oth test runs with a fix I just did: https://tbpl.mozilla.org/?tree=Try&rev=73012559054c
Assignee

Comment 53

5 years ago
The problem turned out to be a simple test mistake I made. When ran individually, the test worked fine, but when part of the suite, it was leaving a highlighter instance behind that was making other tests fail.

This new part0 patch is rebased and contains this simple test fix.

New ongoing try build (with just 'oth' tests to make sure the errors are gone from the logs): https://tbpl.mozilla.org/?tree=Try&rev=4362204ef5c6
Attachment #8445023 - Attachment is obsolete: true
Attachment #8445766 - Flags: review+
Assignee

Comment 54

5 years ago
And a final try build with both patches applied: https://tbpl.mozilla.org/?tree=Try&rev=0c9ae94922b8
The problem that caused the backout is gone and try is green.
I'll land both patches now.
Assignee

Updated

5 years ago
Attachment #8445766 - Flags: checkin+
Assignee

Updated

5 years ago
Attachment #8445024 - Flags: checkin+
Assignee

Comment 57

5 years ago
This may help for the documentation: https://www.youtube.com/watch?v=ImRQQb71gEI
Depends on: 1043896
Assignee

Updated

5 years ago
Depends on: 1051900
Assignee

Comment 58

5 years ago
Heather, here's an attempt at adding nodes highlighting on style-editor selector hover.

It uses the source editor's getInfoAt parser function to retrieve information from hovered text in the editor.

It then uses the toolbox-highlighter-utils to retrieve a new, custom, SelectorHighlighter which it uses to highlight the matching nodes.

There's one limitation to this feature right now which I'd like your feedback on: it won't work correctly with iFrames. Indeed, the new SelectorHighlighter needs both a selector string to execute querySelectorAll, but also a node from which it can get node.ownerDocument, so it knows *where* to execute querySelectorAll.
I think the correct thing to do here is to get that information from the StyleSheet since its styles apply to the document it is loaded in (stylesheet.ownerNode.ownerDocument), but there's no way today to get the ownerDocument from the stylesheet.
I could add a new method to return the ownerDocument node, but I'd have to make sure it's a NodeFront object, because that's what the highlighter expects.
Attachment #8471434 - Flags: feedback?(fayearthur)
Assignee

Comment 59

5 years ago
This patch fixes the iframe-related problems. The way I solved this was adding a new method on the walker actor that accepts a stylesheet actorID and, from this, retrieves the stylesheet object, then its ownerNode, and responds with the corresponding NodeActor.

I've also added tests in this patch.
Attachment #8471434 - Attachment is obsolete: true
Attachment #8471434 - Flags: feedback?(fayearthur)
Attachment #8474391 - Flags: review?(fayearthur)
Comment on attachment 8474391 [details] [diff] [review]
bug971662-part2-highlight-nodes-on-styleeditor-selectors v2.patch

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

Looks good!
Attachment #8474391 - Flags: review?(fayearthur) → review+
Assignee

Comment 61

5 years ago
Thanks Heather for the review!
I just fixed a couple of minor issues that appeared during my last try run.
Here is a new one with these fixes: https://tbpl.mozilla.org/?tree=Try&rev=74a3cb22ab0d
Attachment #8474391 - Attachment is obsolete: true
Attachment #8475907 - Flags: review+
Assignee

Updated

5 years ago
Blocks: 1056115
Assignee

Comment 62

5 years ago
Comment on attachment 8442048 [details] [diff] [review]
bug971662-part4-gcli-highlight-node-types-while-typing v1.patch

Marking this as obsolete. I filed bug 1056115 to work on this instead.
Attachment #8442048 - Attachment is obsolete: true
Assignee

Updated

5 years ago
Keywords: leave-open
Assignee

Updated

5 years ago
Attachment #8475907 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/c469c25e0e45
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 34
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #6)
> Making this bug solely about highlighting nodes when hovering over selectors
> in the style editor and rule view. 
> See bug 971723 for a similar feature in gcli.

Okay - is there a bug also to allow hover in the ruleview?

I think this is cool to expose in the style editor but much more impactful to expose in the ruleview because usage of the inspector / ruleview is so much higher.
Flags: needinfo?(pbrosset)
Assignee

Comment 66

5 years ago
(In reply to Jeff Griffiths (:canuckistani) from comment #65)
> Okay - is there a bug also to allow hover in the ruleview?
Yes, there is now :) bug 1056990
> I think this is cool to expose in the style editor but much more impactful
> to expose in the ruleview because usage of the inspector / ruleview is so
> much higher.
You're right. Also, given recent refactors of the highlighters and rule-view, this will be very easy to do. I'll give it a shot now.
Flags: needinfo?(pbrosset)
Release Note Request (optional, but appreciated)
[Why is this notable]:
[Suggested wording]:
[Links (documentation, blog post, etc)]:

This seems pretty cool. Do you think it should be in the release notes?
relnote-firefox: --- → ?
Flags: qe-verify+
I do agree with you Liz, that is an amazing feature.

Added in the release notes with "Highlight all nodes that match a given selector in the rule-view and style-editor" as wording.
Assignee

Comment 69

5 years ago
(In reply to Sylvestre Ledru [:sylvestre] from comment #68)
> I do agree with you Liz, that is an amazing feature.
> 
> Added in the release notes with "Highlight all nodes that match a given
> selector in the rule-view and style-editor" as wording.
Thanks. I think the wording should be: "Highlight all nodes that match a given selector in the Style Editor and the Inspector's Rules panel".
Ok. Updated! Thanks for the feedback.

Comment 71

5 years ago
I've noticed that, the relevant nodes are highlighted with some delay (~1 sec) 
when hovering the mouse on a selector in the Style Editor,
while in Inpector'Rules panel, the highlighting is instant.

Is this delay the expected behavior?
I believe it would be better if it wasn't any delay.
Assignee

Comment 72

5 years ago
(In reply to Kostas from comment #71)
> I've noticed that, the relevant nodes are highlighted with some delay (~1
> sec) 
> when hovering the mouse on a selector in the Style Editor,
> while in Inpector'Rules panel, the highlighting is instant.
> 
> Is this delay the expected behavior?
> I believe it would be better if it wasn't any delay.

There is indeed a delay in the style-editor. Parsing css text in the style-editor can take a substantial amount of time, and so when we receive a mousemove event, we start a 500ms timeout that will result in showing highlighting nodes only if when it expires the mouse is still at the same place.
So 500ms + the time it takes to parse the css text is what you have to wait.
There's no parsing needed in the style-inspector.
I think we could always file a bug to make the editor 'getInfoAt' function faster.
Depends on: 1099254
Depends on: 1099258

Comment 73

5 years ago
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #72)
> (In reply to Kostas from comment #71)
> > I've noticed that, the relevant nodes are highlighted with some delay (~1
> > sec) 
> > when hovering the mouse on a selector in the Style Editor,
> > while in Inpector'Rules panel, the highlighting is instant.
> > 
> > Is this delay the expected behavior?
> > I believe it would be better if it wasn't any delay.
> 
> There is indeed a delay in the style-editor. Parsing css text in the
> style-editor can take a substantial amount of time, and so when we receive a
> mousemove event, we start a 500ms timeout that will result in showing
> highlighting nodes only if when it expires the mouse is still at the same
> place.
> So 500ms + the time it takes to parse the css text is what you have to wait.
> There's no parsing needed in the style-inspector.
> I think we could always file a bug to make the editor 'getInfoAt' function
> faster.
Thanks for your detailed reple.

What about making the 500ms timeout interval smaller? Eg. 250ms (or even less)?
I find that the current delay is too much, 
and it gets in the way of quickly finding the rule you want by hovering the mouse over the rules.
Hi I've tested this feature using FF 34b.10. Also I've used FF 33.1.1 to compare the behavior to an older version.

I didn't find any differences between highlight behavior in FF 34b.10 and FF 33.1.1. Should this feature be supported in FF 33.1.1? 
The only guideline that I found of this feature is this video https://www.youtube.com/watch?v=ImRQQb71gEI . Is there a more thorough guide ?
Flags: needinfo?(pbrosset)
Assignee

Comment 75

5 years ago
(In reply to Catalin Varga [QA][:VarCat] from comment #74)
> Hi I've tested this feature using FF 34b.10. Also I've used FF 33.1.1 to
> compare the behavior to an older version.
> 
> I didn't find any differences between highlight behavior in FF 34b.10 and FF
> 33.1.1. Should this feature be supported in FF 33.1.1? 
The highlighter itself exists in 33, but the ability to highlight nodes when hovering selectors in the style-editor panel isn't there. This was only added in 34.
> The only guideline that I found of this feature is this video
> https://www.youtube.com/watch?v=ImRQQb71gEI . Is there a more thorough guide
> ?
This video isn't about the style-editor, it only illustrates how the gcli command "highlight" works.
This is the style-editor documentation: https://developer.mozilla.org/en-US/docs/Tools/Style_Editor but it doesn't mention the highlight-on-hover feature yet.
Flags: needinfo?(pbrosset)

Updated

3 years ago
Depends on: 1240338

Updated

3 years ago
Depends on: 1245070, 1223050

Updated

a year ago
Product: Firefox → DevTools
Patrick, could you please advise if the qe+ flag should still apply for this issue?
Flags: needinfo?(pbrosset)
Assignee

Comment 77

5 months ago
Removing the flag on this old bug.
Flags: qe-verify+
Flags: needinfo?(pbrosset)
You need to log in before you can comment on or make changes to this bug.