Closed Bug 1254786 Opened 8 years ago Closed 8 years ago

Add a line outlining the parent-child relationship in the markup view

Categories

(DevTools :: Inspector, defect, P2)

defect

Tracking

(firefox48 fixed)

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: gl, Assigned: gl)

References

Details

Attachments

(5 files, 8 obsolete files)

Attached image Screenshot
      No description provided.
Priority: -- → P2
Attached image deep-nesting.png
That's an interesting idea. I'm just a bit concerned about adding visual clutter to this already crowded panel though. Especially when there are many nested nodes.

Maybe we could solve the same problem by just indenting children a little bit more?

I've made a quick test to try this out (adding a repeating-linear-gradient to the .children elements that draws the line, with a background-size of 1px 100%), and I'm attaching the result.

Helen should make the call on this.
Flags: needinfo?(hholmes)
Attached patch 1254786.patch (obsolete) — Splinter Review
@helen, we need an ui-review and possibly colour suggestions
@bgrins, let's go ahead with a review pending ui-review and possible colour changes
@pbro, we try to avoid adding paddings using box-shadow. As well, we only display it in contextual settings such as hovering over a parent or when a parent with children are selected. That way we aren't displaying a lot of outlines for no reason.
Attachment #8728484 - Flags: ui-review?(hholmes)
Attachment #8728484 - Flags: review?(bgrinstead)
Comment on attachment 8728484 [details] [diff] [review]
1254786.patch

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

The styling changes are simple enough so r=me but I wouldn't land anything until Helen / Patrick sign off on the change

::: devtools/client/themes/markup.css
@@ +132,5 @@
> +  content: "";
> +  background: var(--theme-highlight-red);
> +  width: 1px;
> +  height: 100%;
> +  left: -6px;

This'll need to be checked across platforms to make sure it lines up with the twisty arrow on each.  You can try pushing to mozscreenshots if you don't want to run it locally.  Something like:

try: -b o -p linux,linux64,macosx64,win32,win64 -u mochitest-browser-screenshots[Ubuntu,10.6,10.10,Windows XP,Windows 7,Windows 8] -t none --setenv MOZSCREENSHOTS_SETS=DevTools
Attachment #8728484 - Flags: review?(bgrinstead) → review+
Attachment #8728484 - Flags: feedback?(pbrosset)
Comment on attachment 8728484 [details] [diff] [review]
1254786.patch

If I'm brutally honest I think this adds too much noise to an already complex piece of UI.
My first reaction as well was that I wasn't sure what problem this would fix. I'm very seldom in need of a visual clue to tell me which is my parent or which nodes are descendants of a given parent.
But, after playing more with the browser toolbox, inspecting the browser, where we have a huge DOM tree, many levels of nesting, and where you often need to scroll to see the current parent, then I changed my mind. I do think this can be useful in those situations.
But I'd like to make a few comments on the implementation:

- First of all, I would remove the line on hover. I think this is what makes it look too cluttered for me. And although it's useful, when you have a node selected, to scroll around and still have the visual context of where is your parent, I don't think it's as useful when just hovering around.

- Secondly, I think we could make it look a bit nicer. Instead of having a 1px wide blue line, we could have a 4px wide var(--theme-selection-background-semitransparent) line. I think this adds way less visual noise and looks more consistent with the rest.

- Also, I think it is useful to extend the line all the way down to the closing tag as well. And this is easily done by adding padding-top: 1.4em; to the ::before element.
Attachment #8728484 - Flags: feedback?(pbrosset)
Attached image parent-child-relationship.png (obsolete) —
(In reply to Patrick Brosset [:pbro] from comment #4)
> If I'm brutally honest I think this adds too much noise to an already
> complex piece of UI.
Chrome is experimenting with this currently in Canary; I liked it when I first saw it, and it's obvious that Chrome is trying to innovate with how to make the inspector better. I think that we should try this in Nightly and ship it if it doesn't have valid criticism.

> - First of all, I would remove the line on hover. I think this is what makes
> it look too cluttered for me. And although it's useful, when you have a node
> selected, to scroll around and still have the visual context of where is
> your parent, I don't think it's as useful when just hovering around.
I'm going to suggest we keep the line on hover (I find the line on hover more useful than the selected one, actually), but we keep it pretty light. I've attached an image with what I'm thinking.

> - Secondly, I think we could make it look a bit nicer. Instead of having a
> 1px wide blue line, we could have a 4px wide
> var(--theme-selection-background-semitransparent) line. I think this adds
> way less visual noise and looks more consistent with the rest.
I suggested a 2px line; compromise?

> - Also, I think it is useful to extend the line all the way down to the
> closing tag as well. And this is easily done by adding padding-top: 1.4em;
> to the ::before element.
I suggested some padding on the top and the bottom, actually; I think the line looks best with a bit of room.
Flags: needinfo?(hholmes)
(In reply to Helen V. Holmes (:helenvholmes) (:✨)(Unavailable until 3/14)) from comment #5)
> Created attachment 8730372 [details]
> parent-child-relationship.png

Is this #b1b1b1 color meant for both the light and dark theme?  And, do you think there should be a new theme variable introduced for this color?
Attached image parent-child-lines.png (obsolete) —
(In reply to Brian Grinstead [:bgrins] from comment #6)
> (In reply to Helen V. Holmes (:helenvholmes) (:✨)(Unavailable until 3/14))
> from comment #5)
> > Created attachment 8730372 [details]
> > parent-child-relationship.png
> 
> Is this #b1b1b1 color meant for both the light and dark theme?  And, do you
> think there should be a new theme variable introduced for this color?

Definitely should have a new variable, I think (we're probably going to slowly end up with a series of grays we use for different situations, we might consider a slightly more scalable solution for this).

Attached a new screenshot with some thoughts for the current dark theme. Kinda hard since I think I'm going to change the dark theme a bit, but these work for now.
Attachment #8730372 - Attachment is obsolete: true
Comment on attachment 8728484 [details] [diff] [review]
1254786.patch

I've looked over this—I've added a view visual notes which I know you didn't have when I was originally ui-r?'d, so I'd look at Comment 5 for notes on design and Comment 7's attachment.
Attachment #8728484 - Flags: ui-review?(hholmes) → feedback+
Attached patch 1254786.patch [1.0] (obsolete) — Splinter Review
Attachment #8728484 - Attachment is obsolete: true
Attachment #8730905 - Flags: ui-review?(hholmes)
Attachment #8730905 - Flags: review?(bgrinstead)
Comment on attachment 8730905 [details] [diff] [review]
1254786.patch [1.0]

A small visual nit:

http://cl.ly/18102Y433X1H << can we add margin on both the top and bottom, and then round out the corners? (eyeballing it, it's probably a 2px margin, but we might need to tweak that once it's in the markup view, I'm unsure)
Attachment #8730905 - Flags: ui-review?(hholmes) → feedback+
Attached patch 1254786.patch [2.0] (obsolete) — Splinter Review
Attachment #8730905 - Attachment is obsolete: true
Attachment #8730905 - Flags: review?(bgrinstead)
Attached patch 1254786.patch [3.0] (obsolete) — Splinter Review
ui-review+ by helen in person
Attachment #8731285 - Attachment is obsolete: true
Attachment #8731341 - Flags: ui-review+
Attachment #8731341 - Flags: review?(bgrinstead)
Comment on attachment 8731341 [details] [diff] [review]
1254786.patch [3.0]

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

Clearing review as per comment 13
Attachment #8731341 - Flags: review?(bgrinstead)
Attached patch 1254786.patch [4.0] (obsolete) — Splinter Review
Attachment #8731341 - Attachment is obsolete: true
Attachment #8731362 - Flags: review?(bgrinstead)
So the hover state of a node appears on top of the expander arrow (which is fine), but on the dark theme it causes a sort of weird visual effect.  See screenshot although I'm not sure if it will explain it as well as applying the patch
Comment on attachment 8731362 [details] [diff] [review]
1254786.patch [4.0]

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

::: devtools/client/themes/markup.css
@@ +148,5 @@
> +  position: absolute;
> +  display: block;
> +}
> +
> +.tag-line:hover + .children::before {

Opacity should be set on :hover:not([selected]) so hover opacity doesn't override the selected state
Attachment #8731362 - Flags: review?(bgrinstead)
Attached patch 1254786.patch [5.0] (obsolete) — Splinter Review
Attachment #8731362 - Attachment is obsolete: true
Attachment #8731385 - Flags: review?(bgrinstead)
Helen, what do you think about Comment 16?
Flags: needinfo?(hholmes)
Comment on attachment 8731385 [details] [diff] [review]
1254786.patch [5.0]

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

r=me for code changes

::: devtools/client/themes/markup.css
@@ +1,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +.theme-light {

Please assign this to just :root and then override it for .theme-dark, so it will have a default value for other themes
Attachment #8731385 - Flags: review?(bgrinstead) → review+
Maybe as a follow-up if this is already ready to land, but I noticed that hovering over a closing tag would not show the border. It feels to me like it should.
(In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?)(workweek until 16/3)) from comment #5)
> > - First of all, I would remove the line on hover. I think this is what makes
> > it look too cluttered for me. And although it's useful, when you have a node
> > selected, to scroll around and still have the visual context of where is
> > your parent, I don't think it's as useful when just hovering around.
> I'm going to suggest we keep the line on hover (I find the line on hover
> more useful than the selected one, actually), but we keep it pretty light.
> I've attached an image with what I'm thinking.
Alright, I'm fine to try this out in nightly and see what happens, but I still think the line on hover isn't as useful as the line around the selected node.

To me, the line is especially useful for large/deep DOM trees that require scrolling in the markup-view. So, say I have a node with many descendants selected, it helps if I see this line while I scroll through the sub tree, because it gives me context. Like, after scrolling for a while, I might have lost track of what level of indentation my parent was and I might be wondering if the part of the tree I'm seeing is inside this parent or not. So having the line helps.

The line that appears on hover however will disappear as I scroll through the markup-view, so it won't help me for large sub trees. And I feel like it isn't as useful to have this extra context information for "small" elements that fit on one page of the markup-view without scrolling, especially because we already have the open and close tag highlighted on hover.
I'm fine with it being on both hover and hover + selected. Seems like maybe it should only be on selected, though, now that we've discovered a regression in experience.
Flags: needinfo?(hholmes)
Attachment #8731385 - Attachment is obsolete: true
Attachment #8732177 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/d211f362a8fa
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Depends on: 1260121
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.