796065 - [markup panel] Any un-expanded node with content should show a "..." between open and close tags

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Heather Arthur [:harth]]

To show that there's content in a node without needing to expand it, we should indicate with a "...", like Chrome.

It should be a grey or something that's not the same color as text content.

<div class="content">...</div>

Comment 1

[Girish Sharma [:Optimizer]]

Attachment: patch v1.0

using an ellipses with surrounding space, as simply using ellipses was too narrow.

IMO, "..." feels better, but not sure to use that or not.

Comment 2

[Dave Camp (:dcamp)]

Comment on attachment 703329 [details] [diff] [review]
patch v1.0

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

::: browser/devtools/markupview/MarkupView.jsm
@@ +787,5 @@
> +    this.editor.summaryElt.addEventListener("click", function(evt) {
> +      this.markup.navigate(this);
> +
> +      if (this.expanded) {
> +        this.markup.collapseNode(this.node);

Is this ever going to happen?  The summary element is hidden when expanded, no?

@@ +837,5 @@
>        this.expander.setAttribute("expanded", "");
>        this.children.setAttribute("expanded", "");
> +      if (this.editor.summaryElt) {
> +        this.editor.summaryElt.style.display = "none";
> +      }

The other nodes here use an expanded attribute with a :not([expanded]) rule in markup-view.css.  I don't particularly care whether we use direct manipulation or attribute+style here, but would be nice to be consistent.

Comment 3

[Girish Sharma [:Optimizer]]

(In reply to Dave Camp (:dcamp) from comment #2)
> Comment on attachment 703329 [details] [diff] [review]
> patch v1.0
> 
> Review of attachment 703329 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/markupview/MarkupView.jsm
> @@ +787,5 @@
> > +    this.editor.summaryElt.addEventListener("click", function(evt) {
> > +      this.markup.navigate(this);
> > +
> > +      if (this.expanded) {
> > +        this.markup.collapseNode(this.node);
> 
> Is this ever going to happen?  The summary element is hidden when expanded,
> no?
> 
> @@ +837,5 @@
> >        this.expander.setAttribute("expanded", "");
> >        this.children.setAttribute("expanded", "");
> > +      if (this.editor.summaryElt) {
> > +        this.editor.summaryElt.style.display = "none";
> > +      }
> 
> The other nodes here use an expanded attribute with a :not([expanded]) rule
> in markup-view.css.  I don't particularly care whether we use direct
> manipulation or attribute+style here, but would be nice to be consistent.

Agreed on both. reattaching.

Comment 4

[Girish Sharma [:Optimizer]]

Attachment: patch v1.1

comments addressed.

Comment 5

[Girish Sharma [:Optimizer]]

Before landing, please make a note that this patch uses " … " instead of "...".

I personally like "..." though :)

Comment 6

[Paul Rouget [:paul]]

Why is this patch a mix of 2 bugs? Please split that in 2 patches in 2 different bugs.

Comment 7

[Girish Sharma [:Optimizer]]

Attachment: patch v1.2

carry forward r+
removed the other bug's patch from this one.

Comment 8

[Girish Sharma [:Optimizer]]

Attachment: patch v1.3

my editor has some serious indentation issues -_-

fixed an unintentional indentation.

Comment 9

[Paul Rouget [:paul]]

https://hg.mozilla.org/integration/fx-team/rev/2bff5a9d9783

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/2bff5a9d9783