1431900 - Add a display node to indicate the display value of an element in the markup view.

Status: RESOLVED FIXED

(DevTools :: Inspector, enhancement, P3)

Description

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

This is to add display bubble similar to the event bubble that will indicate "grid", "flex", "inline-grid", and "inline-flex" in the markup view. I don't think the display bubble will need to do anything initially other than to serve as indicator that the element is either a flex or grid container.

Comment 1

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

Attachment: Bug 1431900 - Add a display node to indicate the display value of an element in the markup view.

Review commit: https://reviewboard.mozilla.org/r/215218/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/215218/

Comment 2

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=490aa8fa905cf3bf781a8126985663bb3857c453

Comment 3

[Patrick Brosset <:pbro>]

Comment on attachment 8945022 [details]
Bug 1431900 - Add a display node to indicate the display value of an element in the markup view.

https://reviewboard.mozilla.org/r/215218/#review220958

As I said in the past multiple time, I love the direction here, because I'm convinced our inspector should be better at navigating the page in more ways than just by showing the DOM tree.
Heck, I even created an add-on to search for elements based on any CSS rule: https://addons.mozilla.org/en-US/firefox/addon/devtools-highlighter/

Now, I do have a couple of issues with this patch though:

- If you change the display type of a node from flex to grid, the marker still reads flex. So we miss an update mechanism.
- I'm not convinced about the design for it yet. I don't have anything specific to say about it, but I guess I need more time and more points of view before we go ahead and land something like this. I'd like us to get our entire story sraight before going ahead. I think there's more we could do in this space, and I'd prefer if we took the time to talk about it all before moving ahead.

::: devtools/client/inspector/markup/views/element-editor.js:34
(Diff revision 1)
>    "hr", "img", "input", "keygen", "link", "meta", "param", "source",
>    "track", "wbr" ];
>  
> +// Contains only valid computed display values of the node to display in the element
> +// markup
> +const DISPLAY_VALUES = ["flex", "inline-flex", "grid", "inline-grid"];

I think we usually call these "display types". It's more easily understandable than "display values", which (to me at least) is more vague.
At least if it was "css display property value" then it'd be clearer, but it's so long compared to "display types".
So, everywhere you have some form of display and value together, could you change to display and type?

::: devtools/client/themes/markup.css:417
(Diff revision 1)
>  
> -/* Events */
> +/* Display and Events Bubble */
> +.markupview-display,
>  .markupview-events {
> +  display: none;
> +  cursor: pointer;

The marker you added doesn't do anything when clicked. So having `cursor:pointer` on it is confusing.

::: devtools/server/tests/mochitest/test_inspector-displayvalue.html:35
(Diff revision 1)
> +      gWalker = walker;
> +    }).then(runNextTest));
> +  });
> +});
> +
> +addTest(function testInlineBlockDisplayValue() {

Why aren't all of these functions generators?
You've made one into a generator, and it's nicer to read. Please make the other ones the same.

::: devtools/server/tests/mochitest/test_inspector-displayvalue.html:36
(Diff revision 1)
> +    }).then(runNextTest));
> +  });
> +});
> +
> +addTest(function testInlineBlockDisplayValue() {
> +  info("Test the getting the display value of an inline block element.");

nit: "Test the getting ..." should be "Test getting ..." (same in other places below).

Comment 4

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

I showed this to Victoria and got a quick I like it response. I would like to try and land this and iterate more on it.

Comment 5

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

Comment on attachment 8945022 [details]
Bug 1431900 - Add a display node to indicate the display value of an element in the markup view.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215218/diff/1-2/

Comment 6

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

Comment on attachment 8945022 [details]
Bug 1431900 - Add a display node to indicate the display value of an element in the markup view.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/215218/diff/2-3/

Comment 7

[Patrick Brosset <:pbro>]

Comment on attachment 8945022 [details]
Bug 1431900 - Add a display node to indicate the display value of an element in the markup view.

https://reviewboard.mozilla.org/r/215218/#review223730

::: devtools/client/inspector/markup/views/element-editor.js:34
(Diff revision 3)
>    "hr", "img", "input", "keygen", "link", "meta", "param", "source",
>    "track", "wbr" ];
>  
> +// Contains only valid computed display types of the node to display in the element
> +// markup
> +const DISPLAY_TYPES = ["flex", "inline-flex", "grid", "inline-grid"];

`flow-root` and `contents` are 2 other very interesting display types that I think we should mark too.

::: devtools/client/inspector/markup/views/element-editor.js:268
(Diff revision 3)
> +    let showDisplayNode = DISPLAY_TYPES.includes(this.node.displayType);
> +    this.displayNode.textContent = this.node.displayType;
> +    this.displayNode.dataset.display = showDisplayNode ?
> +      this.node.displayType : "";
> +    this.displayNode.style.display = showDisplayNode ?
> +      "inline-block" : "none";

I think we should also add a title attribute to this node so that a tooltip appears when hovering over it.
Obviously its content should be localized and updated dynamically when the display type changes.

::: devtools/server/actors/inspector/node-actor.js:222
(Diff revision 3)
> +        this.isAfterPseudoElement ||
> +        this.isBeforePseudoElement) {

::before and ::after pseudo-elements can have any display type, why filter them out here?

Comment 8

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

I am wondering if we should rename "display types" to "display property".

Any idea what you want for the title attribute here?
- Display Type
- Display Property
- Display Property Value

Comment 9

[Patrick Brosset <:pbro>]

I think the title attribute should say something user friendly like:
For display:grid/flex: "this element is a [flexbox|grid] container"
For display:flow-root: "this element is a flow-root, it establishes a block formatting context"
For display:contents: "this element isn't rendered, but its content is"

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed63734c2be4
Add a display node to indicate the display value of an element in the markup view. r=pbro

Comment 11

[Dorel Luca [:dluca]]

Backed out changeset ed63734c2be4 (bug 1431900) for Browser-Chrome failures on browser/base/content/test/static/browser_misused_characters_in_strings.js

https://treeherder.mozilla.org/logviewer.html#?job_id=160769966&repo=mozilla-inbound&lineNumber=2358

https://hg.mozilla.org/integration/mozilla-inbound/rev/ea5e2a77362eabe55106bc001e14dabb4e3792bd

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=ed63734c2be459ef10a2967ba7b6d6019a1b3ade

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/782e8d580a84
Add a display node to indicate the display value of an element in the markup view. r=pbro

Comment 13

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/782e8d580a84

Comment 14

[Karl Dubost💡 :karlcow]

fyi https://twitter.com/mozwebcompat/status/963303986932588544
A lot of love for this.