938188 - Add highlight-on-hover to box model tab

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

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

When each region of the box model is hovered we should move the guides on the page to outline only that region e.g. content, padding, border or margin.

Comment 1

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

This will be landed as part of bug 663778

Comment 2

[Brian Grinstead [:bgrins]]

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #1)
> This will be landed as part of bug 663778

This doesn't seem to be working yet.  Was this functionality removed from bug 663778?

Comment 3

[Brian Grinstead [:bgrins]]

Patrick, any ideas how we should implement this with the latest highlighter?

Comment 4

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

(In reply to Brian Grinstead [:bgrins] from comment #2)
> (In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #1)
> > This will be landed as part of bug 663778
> 
> This doesn't seem to be working yet.  Was this functionality removed from
> bug 663778?

I don't understand... this is working fine for me, at least in dev edition.

Comment 5

[Brian Grinstead [:bgrins]]

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #4)
> (In reply to Brian Grinstead [:bgrins] from comment #2)
> > (In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #1)
> > > This will be landed as part of bug 663778
> > 
> > This doesn't seem to be working yet.  Was this functionality removed from
> > bug 663778?
> 
> I don't understand... this is working fine for me, at least in dev edition.

So looks like the guides move when hovering, but I was wanting / expecting the actual highlighter overlay to change to just show the relevant region.  The fact that the guides are moving makes me hopeful that this will be doable without any server API changes, but still not sure about that.

Comment 6

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

(In reply to Brian Grinstead [:bgrins] from comment #5)
> (In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #4)
> > (In reply to Brian Grinstead [:bgrins] from comment #2)
> > > (In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #1)
> > > > This will be landed as part of bug 663778
> > > 
> > > This doesn't seem to be working yet.  Was this functionality removed from
> > > bug 663778?
> > 
> > I don't understand... this is working fine for me, at least in dev edition.
> 
> So looks like the guides move when hovering, but I was wanting / expecting
> the actual highlighter overlay to change to just show the relevant region. 
> The fact that the guides are moving makes me hopeful that this will be
> doable without any server API changes, but still not sure about that.

Darrin Henein (UX) preferred only moving the guides because one or all of content, padding, border and margin may not be present.

Comment 7

[Patrick Brosset <:pbro>]

Yeah, I've also been wanting the highlighter overlay to only show the relevant region at times too. Mike's point is valid too though.
Fwiw, changing the current behavior to only show the relevant region is like 1 line of code:

http://mxr.mozilla.org/mozilla-central/source/browser/devtools/layoutview/view.js#517
instead of just passing the region option to showBoxModel (which tells the highlighter to move the guide around the right region), we would need to also pass the following option: showOnly.
All options accepted by the highlighter are described here: http://mxr.mozilla.org/mozilla-central/sourcehttp://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/highlighter.js#170

Comment 8

[Brian Grinstead [:bgrins]]

Oh great, that's a trivial change, nice :).  IMO we should be showing only the affected region, or maybe lower the opacity on the others down to .1 or something so they are still there but greyed out.  Although I think the fact that you are hovering over the region in the box model tab would help get rid of any confusion about why it isn't showing up on the page if it's not present.

Comment 9

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

I think Firebug users would prefer all regions to be visible whilst some Chrome users would prefer only the currently hovered region to be visible.

Because users prefer either one or the other maybe we should allow a choice from the options panel.

Comment 10

[Brian Grinstead [:bgrins]]

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #9)
> I think Firebug users would prefer all regions to be visible whilst some
> Chrome users would prefer only the currently hovered region to be visible.
> 
> Because users prefer either one or the other maybe we should allow a choice
> from the options panel.

Maybe.. it's such a hard-to-find feature that I'd prefer we just make a decision and not expose an option.  The thing is that with the guides we show that might be more familiar for firebug users even if the other regions get hidden.  Does a user really lose anything when the other regions are hidden, since it's only happening on hover?

Comment 11

[Patrick Brosset <:pbro>]

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #6)
> Darrin Henein (UX) preferred only moving the guides because one or all of
> content, padding, border and margin may not be present.
The thing is getBoxQuads will always return something even if the element has no margin or whatever.
For instance, on this page, open the console, and enter the following:

document.querySelector("#short_desc_nonedit_display").getBoxQuads({box: "margin"})[0]

this will return the same as

document.querySelector("#short_desc_nonedit_display").getBoxQuads({box: "content"})[0]

Since the highlighter is based on this, it means that asking the it to show the margin region only on a node that has no margin, nor border or padding, will still highlight the content region, and I think that's what it should do.

So I'm more in favor of making that change as described in comment 7.

Comment 12

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

(In reply to Brian Grinstead [:bgrins] from comment #10)
> Maybe.. it's such a hard-to-find feature that I'd prefer we just make a
> decision

We already had made a decision months ago. You bought this up before so I asked UX and implemented what they said was best after they had reviewed Chrome, Safari and Firebug's approaches.

If you want to ignore UX then I guess you can but then why did we ask them in the first place?

Comment 13

[Brian Grinstead [:bgrins]]

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #12)
> (In reply to Brian Grinstead [:bgrins] from comment #10)
> > Maybe.. it's such a hard-to-find feature that I'd prefer we just make a
> > decision
> 
> We already had made a decision months ago. You bought this up before so I
> asked UX and implemented what they said was best after they had reviewed
> Chrome, Safari and Firebug's approaches.
> 
> If you want to ignore UX then I guess you can but then why did we ask them
> in the first place?

I don't mean that it shouldn't have been implemented in this way, of course working through this with UX  was the right decision.  I forget the history here, what bug did we talk about this in?

I brought this up because I've wanted the other regions to disappear when using the tools on web content (although I do like the guides and would like to see them stay).

Comment 14

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

Sorry if I was short... I am feeling really irritable at the moment.

Can't remember the bug number - we discussed it at a work week and Darrin spent a couple of days experimenting and looking into it.

Personally I think the Chrome UX is terrible. Go to a new tab in Chrome DevTools, right-click and inspect the google logo and try hovering the box model regions... bad, bad, bad UX.

Also, when I hover the box model I want all of it to be highlighted on the page so that I can see everything in context. Not doing so is removing functionality from my point of view.

My point is that if I feel strongly about this then some of our users will do too as you are talking about changing a feature and that *always* upsets lots of people. We really shouldn't change things like this on a whim.

If we change a feature, especially when it removes functionality like this, we really should go through UX and Jeff to get their input.

Comment 15

[Brian Grinstead [:bgrins]]

Maybe it would be good if we changed it so that other box model regions are just made slightly more transparent instead of hidden.  Although I'm not sure how well that would play with the notes from comment 11.

It looks like it would be easy to hack together a demo of this so that UX / Jeff / whoever else could have a build to play with - which should make it easier to reason about / compare and contrast with how it currently works.

Comment 16

[Patrick Brosset <:pbro>]

Attachment: only-highlight-relevant-region.gif

Here's a screen capture with only highlighting the relevant region.
Also, I've made it so that if your mouse isn't over any of the regions (if it's over the grey background of the panel for instance), then nothing is highlighted.
I personally like it, as I said in a previous comment. But I agree with Mike that this decision shouldn't be made lightly, and I understand that what we have today is a result of a discussion with UX, so I'm fine with it.
Jeff, you think we could experiment with nightly? Our user base isn't very big there, but it would be easy to land and track feedback on nightly? Is this something we can do easily?

Comment 17

[Patrick Brosset <:pbro>]

I didn't forget to NI? UX, I just don't know who is UX these days...

Comment 18

[Brian Grinstead [:bgrins]]

I'd like to see what it looks when working as described comment 16 for comparison.  I believe that will let us keep better context with the other regions.  Specifically, knowing where the region starts could be important to figuring out some layout issue and that context is lost when the entire element is highlighted as that region.

Comment 19

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

So, I like it. Also, I think we have enough user experience chops on the team to argue this into the ground without waiting for input. Doesn't hurt to needinfo people, but it shouldn't stop us from shipping something we all agree is good.

Question - the pixel measures of this example are really small, what does this look like with a border of 200? It looks like you can't distinguish between the area covered by the border and the fill inside the border. The way things currently are, it is easier to distinguish the specific area that border is affecting and also the area inside the border.

Agree it would be good to get a try build of this, and also get feedback from some developers.

Comment 20

[Patrick Brosset <:pbro>]

Attachment: onl-box-model-regions-area.gif

So, here is what it would look like only highlighting the relevant region.

Comment 21

[Patrick Brosset <:pbro>]

Attachment: bug938188-only-highlight-relevant-region-in-boxmodel-ONLY-REGION-AREA.patch

Comment 22

[Brian Grinstead [:bgrins]]

Comment on attachment 8540759 [details] [diff] [review]
bug938188-only-highlight-relevant-region-in-boxmodel-ONLY-REGION-AREA.patch

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

::: toolkit/devtools/server/actors/highlighter.js
@@ +1086,5 @@
> +                   " L" + p2.x + " " + p2.y +
> +                   " L" + p3.x + " " + p3.y +
> +                   " L" + p4.x + " " + p4.y;
> +          } else {
> +            // Otherwise, just draw the region itself, not a filled rectangle.

Oh cool, so using something like this seems like it would make it possible to achieve what I mention in Comment 15 - make the other other regions more transparent than normal instead of fully hidden.

When I tried to get that working using polygons it was really tricky because setting the opacity on one would affect the visible colors of the others.  In fact, it seems like using paths should be a better solution even if nothing else changed since we won't get the colors bleeding into each other.

Comment 23

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #22)
> In fact, it seems like using paths should be a better solution even if
> nothing else changed since we won't get the colors bleeding into each other.

Never mind - this is only helpful if we are controlling visibility / opacity of specific regions - right now the opacity is set on the parent and each region is fully opaque.

Comment 24

[Brian Grinstead [:bgrins]]

Attachment: bug938188-fade-out-non-relevant-regions.patch

Here's what I was thinking in comment 15

Comment 25

[Patrick Brosset <:pbro>]

Attachment: Bug_938188_-_Make_highlighter_capable_of_highlight.diff

I think this is ready for review, but since there was some discussion in this bug, only asking for feedback for now.

I'll attach an animated gif in a minute, but in case you want to try this thing out for yourself, here's a try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0e72ccd925e0

So, what I did here is rebase the last patch that was added in this bug (by Brian). This means that all regions are shown now on box-model hover but the regions other than the hovered one are faded out to opacity 0.2.

I have to admit that I prefer the option of hiding entirely the other regions. I find it slightly confusing now, it's kind of hard to understand what exactly is being highlighted.
My opinion is that if people want to see everything again in context, they just have to move their mouse by about 100px to the left and hover the node in the markup-view. So I don't think it's a problem to hide other regions. If we only show the 1 region on box-model hover, then at least is really obvious what is being highlighted.

Comment 26

[Patrick Brosset <:pbro>]

Attachment: fade-out-other-regions.gif

Comment 27

[Patrick Brosset <:pbro>]

Attachment: hide-other-regions.gif

Here's what it looks like just highlighting one region (and just that region, not covering the nested regions)

Comment 28

[Patrick Brosset <:pbro>]

(In reply to Patrick Brosset [:pbrosset] [:patrick] [:pbro] from comment #26)
> Created attachment 8623668 [details]
> fade-out-other-regions.gif
Of course the compression of this image makes it really hard to see the faded out regions ... Better use the try builds.

Comment 29

[Brian Grinstead [:bgrins]]

Comment on attachment 8623667 [details] [diff] [review]
Bug_938188_-_Make_highlighter_capable_of_highlight.diff

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

::: toolkit/devtools/server/actors/highlighter.css
@@ +35,5 @@
>    opacity: 0.6;
>  }
>  
> +:-moz-native-anonymous .box-model-regions [faded] {
> +  opacity: 0.2;

I like having the [faded] attribute here, so that we can make the presentation decisions in CSS.  After playing with the patch, I believe that we should fully hide the non-hovered regions.  IMO, A really low opacity like this is really hard to see, and anything close to the normal level doesn't have enough contract.

By hovering a region, you've declared intent.  So I don't think that it's too surprising that the other regions hide.

::: toolkit/devtools/server/actors/highlighter.js
@@ +1474,2 @@
>          } else {
>            box.removeAttribute("d");

Do we need to support both showOnly with and without onlyRegionArea?  Could we take 'showOnly' to imply onlyRegionArea?  Especially since we are effectively going to be doing the same thing in both cases if we hide the non-selected regions.

Comment 30

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

(In reply to Patrick Brosset [:pbrosset] [:patrick] [:pbro] from comment #25)
...
> I have to admit that I prefer the option of hiding entirely the other
> regions. I find it slightly confusing now, it's kind of hard to understand
> what exactly is being highlighted.
> My opinion is that if people want to see everything again in context, they
> just have to move their mouse by about 100px to the left and hover the node
> in the markup-view. So I don't think it's a problem to hide other regions.
> If we only show the 1 region on box-model hover, then at least is really
> obvious what is being highlighted.

I think I agree - using the try build it's difficult to even see the faded out bits, and all I really want to do is focus on the bit I'm hovering on.

Comment 31

[Patrick Brosset <:pbro>]

(In reply to Brian Grinstead [:bgrins] from comment #29)
> Could we take 'showOnly' to imply onlyRegionArea?
I didn't want to take showOnly over for this because there are 2 other consumers for this already: the 'highlight' gcli command and the style-editor which highlights elements that match selectors on hover.
Both of them use the showOnly option highlight a simple rectangle.

Comment 32

[Brian Grinstead [:bgrins]]

(In reply to Patrick Brosset [:pbrosset] [:patrick] [:pbro] from comment #31)
> (In reply to Brian Grinstead [:bgrins] from comment #29)
> > Could we take 'showOnly' to imply onlyRegionArea?
> I didn't want to take showOnly over for this because there are 2 other
> consumers for this already: the 'highlight' gcli command and the
> style-editor which highlights elements that match selectors on hover.
> Both of them use the showOnly option highlight a simple rectangle.

OK, so if you call with showOnly on margins, then it will be a single rectangle (which consumes the border, padding, and content)?  Or is it a set of up to 4 rectangles (excluding the other regions), similar to the way the new hover feature will work?

Comment 33

[Patrick Brosset <:pbro>]

(In reply to Brian Grinstead [:bgrins] from comment #32)
> (In reply to Patrick Brosset [:pbrosset] [:patrick] [:pbro] from comment #31)
> > (In reply to Brian Grinstead [:bgrins] from comment #29)
> > > Could we take 'showOnly' to imply onlyRegionArea?
> > I didn't want to take showOnly over for this because there are 2 other
> > consumers for this already: the 'highlight' gcli command and the
> > style-editor which highlights elements that match selectors on hover.
> > Both of them use the showOnly option highlight a simple rectangle.
> 
> OK, so if you call with showOnly on margins, then it will be a single
> rectangle (which consumes the border, padding, and content)?  Or is it a set
> of up to 4 rectangles (excluding the other regions), similar to the way the
> new hover feature will work?
Just 1 rectangle-shaped <path> covering all 4 regions.

Comment 34

[Patrick Brosset <:pbro>]

Attachment: Bug_938188_-_Make_highlighter_capable_of_highlight.diff

Second revision - kept the [faded] attribute in the CSS and used it to set opacity to 0 (maybe I should just display:none though? Not sure if it makes any difference).

Comment 35

[Brian Grinstead [:bgrins]]

Comment on attachment 8624179 [details] [diff] [review]
Bug_938188_-_Make_highlighter_capable_of_highlight.diff

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

::: browser/devtools/inspector/test/browser_inspector_highlighter-options.js
@@ +144,5 @@
>        is(Math.ceil(leftX1), points[3][0], "Left guide's x1 is correct");
>      }
> +  },
> +  {
> +    desc: "When showOnly is used, other regions can be faded",

This looks like another case where we could move the bulk of the testing logic to the server and then make the browser test just check that the highlighter got hooked up properly.

We're already pretty far down this path for the highlighter test, so I'm not going to say we need to make this change now.  But if we ever end up needing to refactor things related to this, we should consider moving it over

::: toolkit/devtools/server/actors/highlighter.css
@@ +37,5 @@
>  
> +/* Box model regions can be faded (see the onlyRegionArea option in
> +   highlighter.js) in order to only display certain regions. */
> +:-moz-native-anonymous .box-model-regions [faded] {
> +  opacity: 0;

Don't know if there's a perf difference, but I think we should use `display: none` since that's really what we want.

::: toolkit/devtools/server/actors/highlighter.js
@@ +1503,5 @@
> +             "L" + p4.x + "," + p4.y;
> +    } else {
> +      // Otherwise, just draw the region itself, not a filled rectangle.
> +      let {p1: np1, p2: np2, p3: np3, p4: np4} = nextBoxQuad;
> +      path = "M" + p1.x + "," + p1.y + " " +

I haven't worked through this path to confirm that it's correct, but testing out the patch on a variety of nodes, it seems right.

Comment 36

[Patrick Brosset <:pbro>]

Attachment: Bug_938188_-_Make_highlighter_capable_of_highlight.diff

Comment 37

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/975d6d156f8e

Comment 38

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/975d6d156f8e