1127572 - When I use the 'H' key to hide an element in the DOM, the markup view should grey it out

Status: RESOLVED FIXED

(DevTools :: Inspector, enhancement, P3)

Description

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

Attachment: CodePen_-_Explode_rhombic_dodecahedron_into_its_first_stellation__pure_CSS_2.png

If I'm working with a bunch of elements, sometimes I want to use the h key shortcut to hide a bunch of them, to focus on just on one. When I'm looking at the markup view, it would be much nicer to look at if the elements that are hidden are also faded out visually. 

Side note - I think the dark theme has regressed, in bug 911209 I thought we'd handled this better in the dark theme, but if you look at the screenshot I can't distinguish between an element marked display:none and other elements, see the second screenshot.

Comment 1

[Patrick Brosset <:pbro>]

You're right, the elements aren't faded out visually.
I investigated a little bit and it turns out that what the H key does is hide the element by applying "visibility:hidden". This property doesn't cause a reflow, and listening to reflows is the mechanism we use to detect when nodes should be faded out or not.
In fact, if you open the inspector then, using the rule-view, add "visibility:hidden" to a node, you'll see it does not appear faded out either.

So it's not a problem with the way the H shortcut works, but with the fact that right now, we only fade out nodes that aren't rendered at all (nodes that don't have any corresponding frames). Nodes that have their visibility set to hidden are still rendered, although not visible.

So, we could :
- change the way H works by applying display:none instead, but that would impact the page layout,
- or also fade out visibility:hidden nodes, but that's not a simple task. I'm not aware of any notification we could use to be alerted of dynamic visibility changes, and polling would be a bad idea.

Comment 2

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

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #1)

> So, we could :
> - change the way H works by applying display:none instead, but that would
> impact the page layout,

This is a non-starter, the feature is useless unless we maintain the layout.

> - or also fade out visibility:hidden nodes, but that's not a simple task.
> I'm not aware of any notification we could use to be alerted of dynamic
> visibility changes, and polling would be a bad idea.

Let's consider this a nice-to-have. The user experience currently is workable but IMO not great, because only some kinds of invisible elements are visually indicated by the fading.

The H key was really, really useful for working with Ana's demo fyi, so I could isolate only one of the shapes and then display how tweaking the bezier curve affected that item's animation. The not-so-great thing is in the markup view, tracking which element is not hidden in a big list of hidden ones. having a face would be nicer on the eyes.

It sounds to me like what we want is an event whenever something becomes 'invisible', eg display:none, visibility:hidden, or opacity set to zero ( or below a certain threshold? ) - this would be more direct than listening for reflows? I assume this involves graphics layer work and therefore either implies the shaving of many yaks or the invention of cold fusion to get working.

Comment 3

[Julian Descottes [:jdescottes]]

Inspector bug triage. Filter on CLIMBING SHOES.

Comment 4

[Julian Descottes [:jdescottes]]

Attachment: bug1127572.proto.patch

I was just quickly looking at this, and I have a small patch that updates the display status on markupmutation (and also checks node.hidden on top of node.isDisplayed of course).

Seems to work, worth cleaning up in your opinion?

Comment 5

[Patrick Brosset <:pbro>]

Comment on attachment 8741493 [details] [diff] [review]
bug1127572.proto.patch

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

Sure, that'd be nice. Worth adding/modifying a test for it too.

Comment 6

[Maximillian von Briesen [:moby]]

Attachment: hide-shortcut.patch

I'd like to take this bug. Could you have a look?

Comment 7

[Brian Grinstead [:bgrins]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f8c1b6e9375

Comment 8

[Julian Descottes [:jdescottes]]

Comment on attachment 8756095 [details] [diff] [review]
hide-shortcut.patch

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

Thanks for the patch moby! 

This looks very good, I just have a few naming nits and a comment.
I don't think this needs another round of review though (unless you would like to discuss something).

::: devtools/client/inspector/markup/markup.js
@@ +966,5 @@
>          // we're not viewing.
>          continue;
>        }
> +
> +      container.refreshDisplay();

We should only call refreshDisplay if the mutation might have an impact on the "displayed" status. 

If the display (or an ancestor's display) changed, this will be handled by _onDisplayChange.

Here the only mutation of interest should come from a className modification, which means a mutation of type "attributes".

@@ +1901,5 @@
>      return false;
>    },
>  
>    /**
>     * Show the element has displayed or not.

nit: Let's update the comment to explain which nodes are rendered as "displayed".

@@ +1903,5 @@
>  
>    /**
>     * Show the element has displayed or not.
>     */
> +  refreshDisplay: function() {

nit: Please add a space before parens. We use eslint to check our syntax. You can check out https://wiki.mozilla.org/DevTools/CodingStandards to see how to setup and run eslint.

For consistency, we already have several update* methods in the container (update, updateExpander, updateLevel). Maybe reuse this naming convention here?

::: devtools/client/inspector/markup/test/browser_markup_node_not_displayed_01.js
@@ +18,5 @@
>    {selector: "head", isDisplayed: false},
>    {selector: "#display-none", isDisplayed: false},
>    {selector: "#hidden-true", isDisplayed: false},
> +  {selector: "#visibility-hidden", isDisplayed: true},
> +  {selector: "#hidden-via-class", isDisplayed: false},

nit: "hidden-via-class" could be misleading because this only works with the classname used when pressing 'H'.

Maybe "#hide-shortcut" or "#hidden-via-hide-shortcut" ?

Comment 9

[Maximillian von Briesen [:moby]]

> We should only call refreshDisplay if the mutation might have an impact on the "displayed" status. 
> If the display (or an ancestor's display) changed, this will be handled by _onDisplayChange.
> Here the only mutation of interest should come from a className modification, which means a mutation of type "attributes".

We still need to call refreshDisplay when the __fx-devtools-hide-shortcut__ class is added to an element, because in that case the element should still be faded in the markup view, but its display CSS attribute was not modified. I could change refreshDisplay to updateIsFaded or something if you want, so its purpose is less ambiguous.

Comment 10

[Brian Grinstead [:bgrins]]

(In reply to Maximillian von Briesen [:moby] from comment #9)
> > We should only call refreshDisplay if the mutation might have an impact on the "displayed" status. 
> > If the display (or an ancestor's display) changed, this will be handled by _onDisplayChange.
> > Here the only mutation of interest should come from a className modification, which means a mutation of type "attributes".
> 
> We still need to call refreshDisplay when the __fx-devtools-hide-shortcut__
> class is added to an element, because in that case the element should still
> be faded in the markup view, but its display CSS attribute was not modified.
> I could change refreshDisplay to updateIsFaded or something if you want, so
> its purpose is less ambiguous.

My reading of the review was that the call to refreshDisplay should only happen if the type of the mutation was "attributes" (since there are other mutations in which the display won't be changed like "characterData", "pseudoClassLock", etc).

Comment 11

[Julian Descottes [:jdescottes]]

Sorry about the unclear comments, I should really go straight to the point.
Brian is right, I meant that "refreshDisplay" should only be called when the mutation is of type "attributes".

(In reply to Maximillian von Briesen [:moby] from comment #9)
> 
> I could change refreshDisplay to updateIsFaded or something if you want, so
> its purpose is less ambiguous.

I think it would be good. 
"refreshDisplay" can easily be misinterpreted, and as I said in another comment we already have "update" methods in this class.

Comment 12

[Maximillian von Briesen [:moby]]

Attachment: hide-shortcut.patch

Comment 13

[Brian Grinstead [:bgrins]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=41dd4262a859

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/4f75d0cc90d1

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4f75d0cc90d1