Closed Bug 911209 Opened 11 years ago Closed 10 years ago

show differently nodes that are "display:none"

Categories

(DevTools :: Inspector, defect)

21 Branch
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 32

People

(Reporter: julienw, Assigned: pbro)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(1 file, 4 obsolete files)

Firebug shows these nodes in a paler color, which is fine for me.

It's very nice when inspecting because we see very clearly that these nodes are not currently shown.
Basically, we should just fade elements with `display:none`. I don't think we should fade nodes with `visibility:hidden|collapse`.
Assignee: nobody → pbrosset
In order to keep the same color theme as the other, non-faded, nodes, I propose to use a somewhat lowered CSS opacity on the nodes that have display: none. Something like 0.6 should be enough to spot the difference with other nodes, while keeping consistent with the global syntax highlighting of the markup-view.

Nodes should be faded whether they are hidden via their style attributes or any matching css selectors.

Nodes should update as changes are made:
- display:none style attributes can be added via javascript or via the devtools
- other attributes can also be added via javascript or the devtools and make nodes match css selectors that have display:none rules
- new stylesheets can be created/loaded via javascript
Depends on: 922146
Bug 922146 will give the ability to listen to new styles insertions and is therefore needed to fade nodes in the markup-view if these nodes match selectors with display:none in the newly inserted styles.
Comment from bug 970523:

"Nodes in the inspector that are either hidden by CSS (`display` or `visibility` properties), or nodes that are hidden inputs (`type="hidden"`) should be distinguished in some way from non-hidden nodes. A possible way to distinguish them would be by lowering their opacity in the inspector, so it's clear from a quick glance that those nodes are not viewable in the document."
Blocks: firebug-gaps
Would it make sense to switch between DOM tree and render tree in the inspector view?
(In reply to David Bruant from comment #6)
> Would it make sense to switch between DOM tree and render tree in the
> inspector view?
Being able to know when the render tree changes and check which DOM nodes aren't represented in it would make it very easy to fade out nodes that are not displayed in the inspector markup-view.
As for switching to the render tree in the markup-view, as a feature of its own, yeah that's interesting. I'm not sure exactly what use case this covers, but I definitely like the idea.
I guess if we're able to fade out all non displayed nodes in the DOM tree already, then it's kind of the same thing.
An excerpt from a discussion on IRC#layout:

01:32 < pbrosset> I want to display elements that have display:none differently in the inspector
01:32 <@tn> pbrosset: certainly you can get that info now, eg getComputedStyle
01:33 < pbrosset> yes definitely, only I was kind of hoping using the presshell could provide me with a bit of a more efficient way of doing this
01:34 < pbrosset> I want to update the display dynamically, so I need to listen for style changes, attribute changes, new stylesheets added, ... and then look at all nodes
01:46 <@tn> you'd basically need to update every style change, which is a lot
01:48 < pbrosset> I'd need to update indeed whenever there's a style change on the page, which could be a change the the developer is doing himself via the devtools
01:48 < pbrosset> like adding style="display:none;" as an attribute
01:48 < pbrosset> or creating a new stylesheet in the style-editor
01:48 < pbrosset> with, say, * {display:none}
01:50 < pbrosset> that's what Firebug does for info, and that's what I have started to do. I just wanted to learn a bit more about how mozilla does layout, see if there was something easier/more efficient I could yse
01:50 < pbrosset> *use
01:52 <@tn> i dont know of anything specific that does exactly what you want
01:57 < pbrosset> ok thanks anyway. Do you think adding a chrome-available event that would tell me when elements' display changes is something that could be added
01:57 < pbrosset> I mean, is it at all possible?
02:03 <@tn> a specific api just for display: none/some is quite possible, but that seems like a very specific hack that would only be useful in one case
02:05 < pbrosset> yeah, I guess it is quite specific indeed
What about nodes that are off screen? Or nodes that are 0x0? <script> tags?
I really like that idea (comment #9). It will probably be a non-trivial task to distinguish elements that are deliberately offscreen and outside the visible viewport (such as the old dropdown trick to be screen-reader friendly), and elements that can be simply scrolled into view (such as the bottom of a long page, or the top of a long page when scrolled to the bottom). There will be a number of situations and states to consider.

If this actually does make it in, I think it would be nice to show these types of "special" out of view nodes in a different color from ones that are stylistically hidden (display:none, visibility:hidden, opacity:0).


Just to add to the possible candidate list from comment #9:

- negative z-index
- <link>
- <meta>
- <noscript> (when no JavaScript)
- <input type="hidden">
- empty or broken <img> tags

While we're at it, what about a way to make <iframe> nodes stick out a bit more than other nodes, sort of as a hint that the containing content is a completely different window context? I know I often get lost trying to distinguish between the top window and a child window if, for example, I have to scroll up higher in the inspector momentarily--it always takes a few moments to find my place again.
From Roc: "You can call element.getClientRects() or element.getBoxQuads(). If it returns a zero-length list, that basically means there are no frames. Checking getComputedStyle("display") for "none" will also work in most cases, but probably misses a few rare cases."
The trickiest part here is to make this dynamic. Indeed, nodes that are not displayed at one time, may become displayed at another time, or vice versa.
Here is a list of events I think we should be listening to, and the corresponding nodes we should check as a result of these events:

- DOM Node appended or removed:
  - should check the node's visibility, as its new position may make it match a css selector that has display:none
  - should also check all of its parents and siblings, as its new position could make other elements match selectors like .foo:not(empty) or :first-child

- DOM Node's attributes changed/added/removed:
  - should check the node's visibility as a style="display:none" attribute may have been added or removed, or a class/id/... may have been changed, making it match a new selector

- CSS property added/changed/removed in a given CSS rule (using the CSSOM):
  - should check all nodes that match the selector of that rule

- Stylesheet added or removed, or text changed:
  - should ideally check all selectors in the stylesheet and all nodes that match them

- Stylesheet/rule changed via the devtools' rule-view and style-editor:
  - should check the corresponding selectors

Based on this list of nodes, we could then easily check their computed styles or boxQuads to know which ones are hidden.

I'm thinking a separate actor would be better here, and probably one that sends events to the client side, in batches.
If the boxQuads change, it means a reflow happened. We can track reflows already.
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #12)
> The trickiest part here is to make this dynamic. Indeed, nodes that are not
> displayed at one time, may become displayed at another time, or vice versa.
> Here is a list of events I think we should be listening to, and the
> corresponding nodes we should check as a result of these events:
This is complicated and subject to change as CSS/DOM APIs changes. All of these events are proxies for changes in the layout tree.
If the layout tree emits events, I'd recommend hooking into that. If it doesn't, it might make sense to ask the layout people to add the proper hooks.

needinfo'ing David Baron to get his eye and expertise on the topic (feel free to forward if you're not the right person or lack time)

> - DOM Node's attributes changed/added/removed:
You forgot the 'hidden' attribute ;-) (just fooling around to prove the point that an exhaustive list is hard to get to).
Flags: needinfo?(dbaron)
(In reply to David Bruant from comment #6)
> Would it make sense to switch between DOM tree and render tree in the
> inspector view?
The more I think about this, the more it makes sense to me that we should show both the DOM tree and the render tree, letting the user switch between the 2.
Having both of them would mean that it'd be easy to diff them and therefore show unrendered nodes differently in the DOM tree.
Also, I think having the render tree in the devtools would make it possible/easier to display paint and reflow events.

(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #13)
> If the boxQuads change, it means a reflow happened. We can track reflows
> already.
True, perhaps we should just listen to reflows (which we already can do as you said), but that wouldn't give us any information about which node(s) was/were reflowed, right? Which means we'd have to update all nodes in the inspector.
As the one who reported the issue, I'd be happy enough with only considering display:none and visibility:hidden DOM nodes. What you're discussing is really interesting but this is much more important work and could be done in another bug in my opinion.
(In reply to Julien Wajsberg [:julienw] from comment #16)
> As the one who reported the issue, I'd be happy enough with only considering
> display:none and visibility:hidden DOM nodes. What you're discussing is
> really interesting but this is much more important work and could be done in
> another bug in my opinion.
Definitely, the UI implications of displaying the render tree reach beyond this bug, however, I believe the ground work should take place in this bug, as this will make it possible to display display:none nodes differently in the inspector. The other solution is comment 12, which is doable, but it's worth waiting for dbaron's reply and see if we can do something smart using the render tree behind the scenes.
Bug 997198 should give us a standalone reflow actor that sends information about reflows and which nodes have been impacted, giving us what we need to update the impacted nodes' visibility in the markupview.
Depends on: 997198
Requires the patch in bug 997198 to be applied first.

How this works:
- listens to reflows using the reflow actor from bug 997198
- on each reflow (or batched reflows in fact), loop through the list of known nodes in the walker (the walker doesn't know of all the nodes in the page from the start, as not all nodes are displayed, so there's no need to loop through the whole DOM tree)
- on each known node, check if the visibility has changed (using the computedstyle)
- if at least one node has had its visibility changed, send an event to the client-side with the list of such nodes
- on the client, in the markup-view, toggle a "not-displayed" class on the elements representing the nodes that have their visibility changing
- apply a .7 opacity on nodes that are supposed to be hidden

Potential improvements:
- if/when bug 997092 lands, we'll be able to know which nodes have been reflowed, making it possible to only loop over these ones in the walker, instead of all the ones we know
- when a node is found to be hidden when looping over nodes in the walker, there's no need to loop over its children as they'll be hidden too.
What about performance? Any noticeable problem?
(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #20)
> What about performance? Any noticeable problem?
Nothing noticeable for now.
One good thing about using the computedstyle and checking if style.display==="none" is that if you set a node to display:none, only this node has the value "none", descendants remain as they were before, so the message sent to the client is fairly light, and the client-side processing is fast. And since, in the markup-view, nodes are structured in a tree, just like the DOM tree, fading the colors via css on the node that is now not displayed anymore will also fade the colors on its descendants, no need to loop through them.
I was thinking about the page performance.

Activating reflow logging slows down page noticeably (resizing the page, for example, is slower with reflow logging).
(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #22)
> I was thinking about the page performance.
> 
> Activating reflow logging slows down page noticeably (resizing the page, for
> example, is slower with reflow logging).
I batch the events by running a loop in the reflow actor which stacks events and only send them at each iteration of the loop, if there are any to be sent. And it seems that changing the loop's timeout impacts performance. The longer the timeout, the better the performance (less protocol packets to be treated).
So it all depends on what we want to use the reflow actor for on the client-side.
If we can accept a little delay (and I think we definitely can), then we can set the timeout to something like 250ms-500ms.
In the case of this bug, showing hidden nodes differently, I don't think it's a big deal if nodes are shown as hidden 250ms later than they really should.
One thing we'll need to do after bug 997198 is done is remove the reflow observer from the webconsole util and instead use the reflow actor.
I'm more worried by the impact of registering the observer, not really about the devtools code.

I don't know how we can test this though…

Bug 453650 is where we introduced these observers. bz reviewed the patch. We really need to make sure we won't slow down the page too seriously as these observers will be activated as soon as we open the toolbox.
So it's not clear to me exactly what conditions you want to hear about changes to, though I think all of them other than visibility:hidden, z-index, and opacity: 0 result in reflows.

The general mechanism for handling style changes is that we compare the new and old style data in nsStyleContext::CalcStyleDifference, and the different nsStyle*::CalcDifference methods (in nsStyleStruct.cpp) return a bitfield of nsChangeHint that tell us what changes need to happen; that bitfield is then processed in RestyleManager::ProcessRestyledFrames.  That code is reasonably performance-critical, and it's also the general mechanism for adding new special-case optimizations to layout to handle dynamic changes in new ways.  So we could add something there if it's needed, but...

It sounds like you're interested in showing this state in the tree view in the inspector -- perhaps performance problems could be avoided by only keeping the state up-to-date for the parts of that tree view that are currently visible?
Flags: needinfo?(dbaron)
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from comment #25)
> So it's not clear to me exactly what conditions you want to hear about
> changes to, though I think all of them other than visibility:hidden,
> z-index, and opacity: 0 result in reflows.
Indeed, reflows seem to be what we need to listen to for this. And we already have the possibility from the devtools to add reflow observers via the docshell. I'm by the way working on that part in bug 997198.

> The general mechanism for handling style changes is that we compare the new
> and old style data in nsStyleContext::CalcStyleDifference, and the different
> nsStyle*::CalcDifference methods (in nsStyleStruct.cpp) return a bitfield of
> nsChangeHint that tell us what changes need to happen; that bitfield is then
> processed in RestyleManager::ProcessRestyledFrames.  That code is reasonably
> performance-critical, and it's also the general mechanism for adding new
> special-case optimizations to layout to handle dynamic changes in new ways. 
> So we could add something there if it's needed, but...
> 
> It sounds like you're interested in showing this state in the tree view in
> the inspector -- perhaps performance problems could be avoided by only
> keeping the state up-to-date for the parts of that tree view that are
> currently visible?
True, and it's also what I've started to do here. Our tree-view only cares about the nodes that are shown (collapsed nodes aren't loaded at first).
When a reflow occurs, we look through the nodes we do know about and check if their visibility has changed.
So, potentially, if the DOM contains a large number of nodes and if the user has expanded all of them in the tree-view, then we're going to end up looping through all of these nodes at every reflow (although we don't treat all of them, we batch reflow events together).

So I think one optimization here could be if the reflow events we get through docshell.WeakReflowObserver() would give us information about what nodes have been reflowed, and maybe even what types of changes caused the reflow (visibility change, box-model change, ...). See bug 997092.
Based on the latest reflow actor from bug 997198.
Added tests.

TODO:
- This doesn't work inside iframes, because reflows aren't being observed in iframes. See bug 1007021.
- The 'isDisplayed' property is being passed as part of the NodeActor's form everytime, and resolved by reading the computedStyle everytime. I would like to find a way to only send the full form when needed. There's a risk that the NodeActor form grows and makes our tool a little bit slower if we don't pay attention to this part.
Attachment #8411621 - Attachment is obsolete: true
Comment on attachment 8421018 [details] [diff] [review]
bug911209-fade-display-none-nodes v2.patch

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

::: browser/devtools/markupview/markup-view.js
@@ +1287,5 @@
>  
>    // Prepare the image preview tooltip data if any
>    this._prepareImagePreview();
> +
> +  // The NodeActor post FF32 sends the isDisplayed information as part of its

IMO this backwards-compat check would be better inside of the NodeFront getter in case we want to use it anywhere else.

::: toolkit/devtools/server/actors/inspector.js
@@ +260,5 @@
> +
> +  /**
> +   * Is the node's display computed style value other than "none"
> +   */
> +  get isDisplayed() {

perf: doing two gets on computedStyle here for element nodes

@@ +934,5 @@
> +    // Going through the nodes the walker knows about, see which ones have
> +    // had their display changed and send a display-change event if any
> +    let changes = [];
> +    for (let [node, actor] of this._refMap) {
> +      if (actor.isDisplayed !== actor.wasDisplayed) {

perf: accessing isDisplayed twice here
- Now works in iframes (thanks to bug 1007021 which just landed)
- Addressed feedback from bgrins in the previous comment
- Ongoing try build: https://tbpl.mozilla.org/?tree=Try&rev=a6bd01e37c19
Attachment #8421018 - Attachment is obsolete: true
Attachment #8434145 - Flags: review?(mratcliffe)
Attachment #8434145 - Flags: review?(mratcliffe) → review+
Rebased and added a minor deadWrapper check for the node in InspectorActor.
Carrying R+ over.
Try is green.
Landed: https://hg.mozilla.org/integration/fx-team/rev/52068b669c2a
Attachment #8434145 - Attachment is obsolete: true
Attachment #8434401 - Flags: review+
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/52068b669c2a
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 32
Sorry patrick had to back this out since it seems this cause in tests type errors like https://tbpl.mozilla.org/php/getParsedLog.php?id=41078584&tree=Mozilla-Central
Backout from comment 32:
https://hg.mozilla.org/mozilla-central/rev/51b428be6213
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Added a very simple fix for the issue found on m-c and m-i.
It wasn't failing for me locally nor on try, so the only thing I could do was update with the latest fx-team, put my fix in and launch a new try with many retriggers to make sure the errors didn't occur again:

https://tbpl.mozilla.org/?tree=Try&rev=c704df8198f3

This looks pretty green to me, so I guess the next step would be to re-land the patch?
Attachment #8434401 - Attachment is obsolete: true
Attachment #8434862 - Flags: review+
Re-landed: https://hg.mozilla.org/integration/fx-team/rev/62385fdd6577
Status: REOPENED → ASSIGNED
OS: Linux → All
Hardware: x86_64 → All
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/62385fdd6577
Status: ASSIGNED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
It's great to see this land.

There is one thing I think could be improved: the contrast between visible and hidden elements is too subtle. On the light theme it's hard noticeable, while the dark theme is only marginally more distinguishable. I think the opacity level can be lowered a lot more.

New bug?
Flags: needinfo?(pbrosset)
(In reply to Dane MacMillan from comment #37)
> It's great to see this land.
> 
> There is one thing I think could be improved: the contrast between visible
> and hidden elements is too subtle. On the light theme it's hard noticeable,
> while the dark theme is only marginally more distinguishable. I think the
> opacity level can be lowered a lot more.
> 
> New bug?

There is an ongoing discussion on the list about this [0].  I have accessibility concerns about lowering the opacity any more than it already is [1], but I'd be fine with either increasing the starting contrast of the colors or coming up with another way to distinguish hidden nodes.  Feel free to chime in on the list about this.

[0]: https://groups.google.com/d/msg/mozilla.dev.developer-tools/
[1]: https://groups.google.com/d/msg/mozilla.dev.developer-tools/-rwnVspeSvI/Hwo2rJGbMVMJ
Flags: needinfo?(pbrosset)
See also bug 1021689
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.