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

RESOLVED FIXED in Firefox 49

Status

enhancement
P3
normal
RESOLVED FIXED
4 years ago
a year ago

People

(Reporter: canuckistani, Assigned: moby, Mentored)

Tracking

36 Branch
Firefox 49
x86
macOS

Firefox Tracking Flags

(firefox49 fixed)

Details

(Whiteboard: [good first bug])

Attachments

(3 attachments, 1 obsolete attachment)

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.
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.
(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.
Inspector bug triage. Filter on CLIMBING SHOES.
Severity: normal → enhancement
Priority: -- → P3
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?
Attachment #8741493 - Flags: feedback?(pbrosset)
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.
Attachment #8741493 - Flags: feedback?(pbrosset) → feedback+
Mentor: jdescottes
Whiteboard: [good first bug]
Posted patch hide-shortcut.patch (obsolete) — Splinter Review
I'd like to take this bug. Could you have a look?
Attachment #8756095 - Flags: review?(jdescottes)
Assignee: nobody → mvonbriesen
Status: NEW → ASSIGNED
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" ?
Attachment #8756095 - Flags: review?(jdescottes) → review+
> 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.
(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).
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.
Assignee

Updated

3 years ago
Attachment #8756095 - Attachment is obsolete: true
Assignee

Updated

3 years ago
Keywords: checkin-needed

Comment 15

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4f75d0cc90d1
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49

Updated

a year ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.