1329302 - Expose stacking context information

Status: NEW

(DevTools :: Inspector, enhancement, P2)

Description

[Jeff Muizelaar [:jrmuizel]]

This is helpful for debugging the positioning of elements. z-context is a helpful add-on for Chrome that does shows this information: https://github.com/gwwar/z-context

Comment 1

[Brian Grinstead [:bgrins]]

How can we best get ahold of this info for an individual node?  Does it require dom traversal or is there / could there be a way to access it from the platform directly for a node?

Comment 2

[Jeff Muizelaar [:jrmuizel]]

I don't know of a way to get it from the platform directly. Perhaps mstange does.

It looks like the Chrome add on justs figures it out manually from the DOM:
https://github.com/gwwar/z-context/blob/master/devtools/index.js

Comment 3

[Gabriel Luong [:gl] (ΦωΦ)]

We've had a group of students work on this for the past couple of months, and they've come up with something really cool: https://github.com/gregtatum/z-index-devtool

I started working on actors to fetch the stacking context information to provide to the frontend in Bug 1328623, and I will be working on landing the z-index tool into our DevTools.

Currently, it all relies on a JS algorithm that walks the whole DOM and returns a tree of elements that are either stacking contexts or have a z-index defined. Something :pbro came up some time ago that probably isn't fully correct but works enough for this tool:
https://github.com/gregtatum/z-index-devtool/blob/master/src/stacking-context/index.js

Again, a platform API for this would be really cool! We have also reached out to Brad Werth to see if this is something platform can support.

Comment 4

[Patrick Brosset <:pbro>]

Inspector bug triage (filter on CLIMBING SHOES).

Comment 5

[Gabriel Luong [:gl] (ΦωΦ)]

I am working on introducing a WalkerActor like StackingContextWalkerActor. The following is how I think the frontend might work with the StackingContextWalkerActor:

refresh: Task.async(function* (treeWalkers) {
  // Stop refreshing if the inspector or store is already destroyed.
  if (!this.inspector || !this.store) {
    return;
  }

  if (!treeWalkers) {
    treeWalkers =
      yield this.layoutInspector.getAllStackingContextWalkers(this.walker.rootNode);
  }

  for (let treeWalker of treeWalkers) {
    let treeNodes = yield treeWalker.getTreeNodes();
    let state = [];

    for (let treeNode of treeNodes) {
      let el = yield this.walker.getNodeFromActor(treeNode.actorID, ["element"]);
      let { key, properties } = treeNode;

      let parentElement = yield treeWalker.getParentElement(treeNode);
      let stackingContextChildren = yield treeWalker.getStackingContextChildren(treeNode);

      state.push({
        el,
        key,
        parentElement,
        parentStackingContext: undefined,
        nodes: stackingContextChildren,
        properties,
        stackingContextChildren,
      });
     }

    this.store.dispatch(getStackingContext(state));
  }
})

I suspect the actor will go through some changes as well as how we do things in the frontend as we continue to develop this project.

Comment 6

[Gabriel Luong [:gl] (ΦωΦ)]

Attachment: Part 1: Implement StackingContextWalkerActor and StackingContextNodeActor to expose stacking context information.

Comment 7

[Markus Stange [:mstange]]

Our layout code for checking whether something is a stacking context is here: http://searchfox.org/mozilla-central/rev/790b2cb423ea1ecb5746722d51633caec9bab95a/layout/generic/nsFrame.cpp#2812-2826
It's in the middle of a function and there is no API for it.

Comment 8

[Patrick Brosset <:pbro>]

Comment on attachment 8827345 [details] [diff] [review]
Part 1: Implement StackingContextWalkerActor and StackingContextNodeActor to expose stacking context information.

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

First of all, really sorry about the delay reviewing this.
I've made a few minor comments below, but I guess my bigger issue with this is about the architecture approach. I'm not sure we need another set of actors for this stacking context feature.
I'd like to suggest using the WalkerActor instead, and NodeActors too.
There are several benefits to doing this:
- all DOMNodes already visited by the WalkerActor are already know on the front-end, so they don't need to be sent again, the client knows them,
- much of the inspector works with NodeFront instances, it just an easy object type to work with and is expected by a bunch of APIs already (like Selection, which might be useful, but soon the Reps too),

To me, it doesn't seem very useful to add a entirely new type of Actor for something that will only ever return the full tree. The StackingContextWalkerActor looks like the WalkerActor, but doesn't really behave like it. It finds out all nodes right from the start. The WalkerActor's idea was to walk the DOM little by little, only when needed (i.e. when the user expands nodes in the markup-view).
I think we need to assume that a stacking context tree can be as big and deep as a DOM tree. So, how is the client going to get this tree? Only on demand when a parent is expanded? Or all at once? (which can potentially lead to a lot of data being sent down).
I'm guessing the former, but that of course, depends a lot on how the UI is going to look like.

I think we should investigate something like this:
- add a getStackingContextRoot method to the WalkerActor, which returns the root of the stacking context tree (this should normally be html or body, right?)
- add a stackingContextTreeChildren method to the WalkerActor, which takes any stacking context node as an argument (as a NodeActor) and acts just like WalkerActor.children, except that it filters only stacking contexts and z-indexed elements (in fact, the children method could even be reused, by extending how the filter used in the DocumentWalker works).

About filtering in the DocumentWalker: see allAnonymousContentTreeWalkerFilter for instance, it's a filter function that gets a DOMNode and returns a flag that indicates if we want to see this node or not (btw, I changed it recently to return empty text nodes if they were inline). We could have a special filter like stackingContextTreeWalkerFilter, that only returns the ACCEPT flag if the node is a node we care about for the stacking context tree.

What do you think?

::: devtools/server/actors/layout.js
@@ +14,5 @@
> +  stackingContextNodeSpec,
> +  stackingContextWalkerSpec,
> +} = require("devtools/shared/specs/layout");
> +
> +const INCLUDE_HTML_TAGS = ["DIV", "SPAN", "P", "IMG", "TITLE"];

Why these tags only? What's the logic here?

@@ +349,5 @@
> +
> +/**
> + * Returns an object containing the stacking context properties of a given element.
> + *
> + * @param  {NodeActor} el

el seems to be on DOMNode in the function below, not a NodeActor. So the jsdoc should be changed.

@@ +387,5 @@
> +  return nodeProperties;
> +}
> +
> +/**
> + * Returns true if the given properties is a stacking context and false otherwise.

I think it might be worth adding a comment here that explains how contexts are formed:

/**
 * https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_Positioning/Understanding_z_index/The_stacking_context
 *
 * A stacking context is formed, anywhere in the document, by any element which is either
 *
 * - the root element (HTML),
 * - positioned (absolutely or relatively) with a z-index value other than "auto",
 * - a flex item with a z-index value other than "auto",that is the parent element display: flex|inline-flex,
 * - elements with an opacity value less than 1. (See the specification for opacity),
 * - elements with a transform value other than "none",
 * - elements with a mix-blend-mode value other than "normal",
 * - elements with a filter value other than "none",
 * - elements with a perspective value other than "none",
 * - elements with isolation set to "isolate",
 * - position: fixed
 * - specifying any attribute above in will-change even if you don't specify values for these attributes directly
 * - elements with -webkit-overflow-scrolling set to "touch"
 */

And maybe also adding a link to the platform bug (if there is one, if not it should be filed) where we're asking for a platform API for this instead.
I don't expect the rules to change often (if ever), but I'd still feel more confident if this code was the same C++ code that's in Gecko, rather than having our own version that potentially has bugs.

@@ +439,5 @@
> +    let newNode;
> +    // Filter for divs and spans only.
> +    // Easily change to include others. (Maybe make it a configurable setting in the
> +    // future)
> +    if (INCLUDE_HTML_TAGS.indexOf(child.tagName) !== -1) {

I don't understand what this INCLUDE_HTML_TAGS const is. And why it only contains a subset of elements. Can you explain?

@@ +495,5 @@
> + * @return {Array} Sorted array of StackingContextNodeActor objects according to their
> + *         zindex.
> + */
> +function sortNodesByZIndex(tree) {
> +  tree.sort(function(a, b){

nit: I think we use arrow functions for these types of things now.