Patrick Brosset <:pbro> Reporter
	Description • 9 years ago

The pref devtools.markup.collapseAttributeLength was added in bug 1198073 to allow people to choose whether they want attributes to always be displayed in full (-1) or to be collapsed after a certain number of characters (120 by default).

We should add a new setting in the devtools settings panel, in the inspector section, that allows to switch between a positive integer that the user would enter, or no collapsing (which would set the pref to -1).

	Hugo Assignee
	Comment 1 • 9 years ago

I would like to work on this, can you assign me the issue please?

Also, I need a small clarification if you don't mind: I'm not totally sure how do you want to handle the custom pref, should I provide just a checkbox to toggle between -1/120 and then leave to the user to manually change the pref in case he wants to use a custom integer? Or should I provide a textbox somewhere when the user choose to enable collapsing? (or perhaps just the textbox and no checkbox at all?)

	Patrick Brosset <:pbro> Reporter
	Comment 2 • 9 years ago

Not sure ...

Only adding a checkbox could be a first simple step, and maybe that's all we need (in this case, its label would read something like "Cut long attributes" and it would be checked by default).
But then we'd also have to make sure that checking it after the pref has been changed to another limit, does not override that limit.

Having a text input next to it would make it easier to use the feature of course, but would also add more form controls to an already over-populated panel, and I don't expect many people to use this.

Maybe Helen can decide.

In any case, I think we agree there should be a checkbox. Unchecked = -1, Checked = whatever the value of the pref is.
At least this would make it very simple in the future to add a text field to change the pref.

	Patrick Brosset <:pbro> Reporter
	Comment 3 • 9 years ago

Brian, do you want to mentor Hugo on this bug?

	Helen V. Holmes (:helenvholmes) (:✨)
	Comment 4 • 9 years ago

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #2)
> Having a text input next to it would make it easier to use the feature of
> course, but would also add more form controls to an already over-populated
> panel, and I don't expect many people to use this.
> 
> Maybe Helen can decide.

My recommendation is just to have a checkbox:

[ ] Never truncate Inspector attributes

and leave it at that. Super duper ✨ power users ✨ can change the number of characters they want truncated in about:preferences.

	Brian Grinstead [:bgrins]
	Comment 5 • 9 years ago

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #3)
> Brian, do you want to mentor Hugo on this bug?

Sure.  You should be able to add a new option to the options panel from here: https://dxr.mozilla.org/mozilla-central/source/devtools/client/framework/toolbox-options.xul#46.  Localized strings should be added here: https://dxr.mozilla.org/mozilla-central/source/devtools/client/locales/en-US/toolbox.dtd#59.

Unfortunately since this isn't a bool pref it won't be as simple as:

  <checkbox data-pref="devtools.markup.collapseAttributeLength"/>

Since the js is passing 'this.checked' in as the new value for a checkbox when it changes: https://dxr.mozilla.org/mozilla-central/source/devtools/client/framework/toolbox-options.js#284.

So we either need to add some new data attributes to tell the framework that the checkbox should set the default value when checked and set -1 when unchecked, or we need to special case this control.  My initial thought is to introduce a couple of data attributes to specify the value that should be set when it's checked and unchecked:

  <checkbox data-pref="devtools.markup.collapseAttributeLength"
            data-checked-value="default"
            data-unchecked-value="-1"/>

	Hugo Assignee
	Comment 6 • 9 years ago

Thanks all!

Brian, your suggestion about the checked/unchecked values works perfectly fine to set the pref properly, but I think we have a problem when populating the checkbox at the beginning, let me show you some code, this is the trivial implementation (assuming checked/unchecked attrs are both optional):

      let prefValue = GetPref(checkbox.getAttribute("data-pref"));
      let checked = prefValue;
      if (checkbox.hasAttribute("data-checked-value")) {
        checked = checkbox.getAttribute("data-checked-value") == prefValue;
      } else if (checkbox.hasAttribute("data-unchecked-value")) {
        checked = checkbox.getAttribute("data-unchecked-value") != prefValue;
      }
      checkbox.checked = checked;
      checkbox.addEventListener("command", function() {
        let attr = this.checked ? "data-checked-value" : "data-unchecked-value";
        setPrefAndEmit(this.getAttribute("data-pref"), this.hasAttribute(attr)
          ? this.getAttribute(attr)
          : this.checked);
      }.bind(checkbox));

I think this will work for the normal case, but what if the user changes manually the pref to, let's say, length = 100. It would be nice to kept the checkbox unchecked but we don't have the flexibility to do so. A suggestion to kept this idea would be to add yet another attr, something like 'data-check-against' (values: "checked"/"unchecked"), and perhaps we can change the code to something like this:

      let checked = prefValue;
      if (checkbox.hasAttribute("data-check-against")) {
        if (checkbox.getAttribute("data-check-against") == "checked") {
          checked = checkbox.getAttribute("data-checked-value") == prefValue;
        } else {
          checked = checkbox.getAttribute("data-unchecked-value") != prefValue;
        }
      }

So we can define our checkbox in the xul file as:

          <checkbox label="&options.collapseAttrs.label;"
                    tooltiptext="&options.collapseAttrs.tooltip;"
                    data-pref="devtools.markup.collapseAttributeLength"
                    data-checked-value="-1"
                    data-unchecked-value="120"
                    data-check-against="checked"/>

But this might be an overcomplex solution for this problem. What do you think?

	Brian Grinstead [:bgrins]
	Comment 7 • 9 years ago

(In reply to Hugo from comment #6)
> I think this will work for the normal case, but what if the user changes
> manually the pref to, let's say, length = 100. It would be nice to kept the
> checkbox unchecked but we don't have the flexibility to do so. A suggestion
> to kept this idea would be to add yet another attr, something like
> 'data-check-against' (values: "checked"/"unchecked"), and perhaps we can
> change the code to something like this:
>
> But this might be an overcomplex solution for this problem. What do you
> think?

I see the issue but I think if the user changed to a custom length it's OK for odd behavior with the checkbox since it's trying to represent something it can't.

One option is: '☑ Truncate long attributes' and we can have these things default to checked if they don't match either of the options.

  <checkbox label="Truncate long attributes"
            data-pref="devtools.markup.collapseAttributeLength"
            data-checked-value="default"
            data-unchecked-value="-1"/>

Another thought - why are we are using an int attribute to specify a boolean condition?  What about devtools.markup.collapseAttributes=true and devtools.markup.collapseAttributeLength=120.  Then we can have a simple option checkbox with data-pref="devtools.markup.collapseAttributes" and power users can go in and tweak the length pref if they want.  Patrick, Helen, Hugo, any thoughts?

	Hugo Assignee
	Comment 8 • 9 years ago

(In reply to Brian Grinstead [:bgrins] from comment #7)
> Another thought - why are we are using an int attribute to specify a boolean
> condition?  What about devtools.markup.collapseAttributes=true and
> devtools.markup.collapseAttributeLength=120.  Then we can have a simple
> option checkbox with data-pref="devtools.markup.collapseAttributes" and
> power users can go in and tweak the length pref if they want.  Patrick,
> Helen, Hugo, any thoughts?

+1, IMHO is the best option

	Patrick Brosset <:pbro> Reporter
	Comment 9 • 9 years ago

(In reply to Brian Grinstead [:bgrins] from comment #7)
> Another thought - why are we are using an int attribute to specify a boolean
> condition?  What about devtools.markup.collapseAttributes=true and
> devtools.markup.collapseAttributeLength=120.  Then we can have a simple
> option checkbox with data-pref="devtools.markup.collapseAttributes" and
> power users can go in and tweak the length pref if they want.  Patrick,
> Helen, Hugo, any thoughts?
Sounds good to me.

	Hugo Assignee
	Comment 10 • 9 years ago

Ok, here is a patch with the two prefs, I also added a PrefObserver to be able to reload the markup view when the pref changes, because I thought it was a bit ugly to have to re-open the inspector now, when you can change the pref directly on the settings panel.

	Helen V. Holmes (:helenvholmes) (:✨)
	Comment 11 • 9 years ago

(In reply to Hugo from comment #8)
> > Another thought - why are we are using an int attribute to specify a boolean
> > condition?  What about devtools.markup.collapseAttributes=true and
> > devtools.markup.collapseAttributeLength=120.  Then we can have a simple
> > option checkbox with data-pref="devtools.markup.collapseAttributes" and
> > power users can go in and tweak the length pref if they want.  Patrick,
> > Helen, Hugo, any thoughts?

Also agree, +1~

	Brian Grinstead [:bgrins]
	Comment 12 • 9 years ago

Comment on attachment 8691917 [details] [diff] [review]
0001-Bug-1225063-add-option-devtools.markup.collapseAttri.patch

Sorry for the delay.  This patch looks pretty reasonable, going to flag myself for review

	Brian Grinstead [:bgrins]
	Comment 13 • 9 years ago

Comment on attachment 8691917 [details] [diff] [review]
0001-Bug-1225063-add-option-devtools.markup.collapseAttri.patch

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

Thanks for putting this together, it's a good start.  I have some notes - see below

::: devtools/client/locales/en-US/toolbox.dtd
@@ +61,5 @@
>  
> +<!-- LOCALIZATION NOTE (options.collapseAttrs.label): This is the label
> +  -  for the checkbox option to enable collapse attributes in the Inspector
> +  -  panel. -->
> +<!ENTITY options.collapseAttrs.label "Truncate Inspector attributes">

You can remove the word "Inspector" from this label

@@ +62,5 @@
> +<!-- LOCALIZATION NOTE (options.collapseAttrs.label): This is the label
> +  -  for the checkbox option to enable collapse attributes in the Inspector
> +  -  panel. -->
> +<!ENTITY options.collapseAttrs.label "Truncate Inspector attributes">
> +<!ENTITY options.collapseAttrs.tooltip "Turning this on will enable attributes collapsing in the inspector.">

Nit: no period needed at the end of tooltips IIRC

::: devtools/client/markupview/markup-view.js
@@ +75,4 @@
>    this.doc = this._frame.contentDocument;
>    this._elt = this.doc.querySelector("#root");
>    this.htmlEditor = new HTMLEditor(this.doc);
> +  this.globalAttrChange = false;

As noted below, I think we can get rid oo this.globalAttrChange

@@ +1571,4 @@
>      this._inspector.selection.off("new-node-front", this._onNewSelection);
>      this._inspector.toolbox.off("picker-node-hovered", this._onToolboxPickerHover);
>  
> +    this._prefObserver.off(ATTR_COLLAPSE_ENABLED_PREF, 

Nit: trailing whitespace

@@ +2660,5 @@
>      // attributes have already been removed at this point.
>      for (let attr of nodeAttributes) {
>        let el = this.attrElements.get(attr.name);
> +      let valueChanged = el &&
> +        (this.markup.globalAttrChange || el.querySelector(".attr-value").textContent !== attr.value);

I don't think we need globalAttrChange in this case.  I think what we need is to move the 'collapse' function out of the function below and onto an object that can be called from here, then do something like `el.querySelector(".attr-value").textContent !== this.collapseAttribute(attr.value)`.

Either that or we need to somehow map the full attribute that belongs to the element.  So maybe a WeakMap <DOMElement, string> where the element is `el` and the string is the full attribute.  Then we could do a lookup on the map with el and see if it matches attr.value.

Sorry, I wish this was more straightforward..  Maybe Patrick has some ideas. Right now even without this patch it looks like a truncated attribute will always appear to have changed since its text content will never match the full attribute.

::: devtools/client/markupview/test/browser_markupview_tag_edit_07.js
@@ +80,1 @@
>      "data-long": LONG_ATTRIBUTE

The 'desc' on this function should be updated to reflect the new pref name

@@ +80,4 @@
>      "data-long": LONG_ATTRIBUTE
>    },
>    setUp: function(inspector) {
> +    SpecialPowers.setBoolPref("devtools.markup.collapseAttributes", false);

Nice!  This now also covers that the props are being read properly from prefs

@@ +95,2 @@
>    }
>  }];

Can you please add a new test case here that runs with a modified collapseAttributeLength (to something small)

	Brian Grinstead [:bgrins]
	Comment 14 • 9 years ago

(In reply to Brian Grinstead [:bgrins] from comment #13)
> @@ +2660,5 @@
> >      // attributes have already been removed at this point.
> >      for (let attr of nodeAttributes) {
> >        let el = this.attrElements.get(attr.name);
> > +      let valueChanged = el &&
> > +        (this.markup.globalAttrChange || el.querySelector(".attr-value").textContent !== attr.value);
> 
> I don't think we need globalAttrChange in this case.  I think what we need
> is to move the 'collapse' function out of the function below and onto an
> object that can be called from here, then do something like
> `el.querySelector(".attr-value").textContent !==
> this.collapseAttribute(attr.value)`.
> 
> Either that or we need to somehow map the full attribute that belongs to the
> element.  So maybe a WeakMap <DOMElement, string> where the element is `el`
> and the string is the full attribute.  Then we could do a lookup on the map
> with el and see if it matches attr.value.
> 
> Sorry, I wish this was more straightforward..  Maybe Patrick has some ideas.
> Right now even without this patch it looks like a truncated attribute will
> always appear to have changed since its text content will never match the
> full attribute.

ni? Patrick for this part

	Patrick Brosset <:pbro> Reporter
	Comment 15 • 9 years ago

I agree, we shouldn't have to rely on globalAttrChange to avoid flashing attributes when the pref is changed.
As Brian said, the code in 'update' should know when an attribute's value truly changed, not just appears to have changed.
In particular, the 'valueChanged' variable shouldn't check the textContent. Or if it does, it should compare it to the truncated value.
How about we store the full attribute value in the DOM, as a data attribute?
In markup-view.xhtml, add 'data-value="${attrValue}"' to the #template-attribute element, and then in markup-view.js, initialize 'valueChanged' with:
'let valueChanged = el && el.dataset.value !== attr.value;'

	Brian Grinstead [:bgrins]
	Comment 17 • 8 years ago

Comment on attachment 8708852 [details] [diff] [review]
0001-Bug-1225063-add-option-devtools.markup.collapseAttri.patch

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

This looks great, thanks!  Just a few minor notes

I have a push to try ongoing: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e25c0fe43151

::: devtools/client/inspector/markup/test/browser_markup_tag_edit_07.js
@@ +79,5 @@
>    expectedAttributes: {
>      "data-long": LONG_ATTRIBUTE
>    },
>    setUp: function(inspector) {
> +    SpecialPowers.setBoolPref("devtools.markup.collapseAttributes", false);

Nit: I've seen mostly `Services.prefs.setBoolPref` used throughout our tests

@@ +91,4 @@
>      is(visibleAttrText, LONG_ATTRIBUTE);
>    },
>    tearDown: function(inspector) {
> +    SpecialPowers.setBoolPref("devtools.markup.collapseAttributes", true);

Services.prefs.clearUserPref("devtools.markup.collapseAttributes")

@@ +99,5 @@
> +  expectedAttributes: {
> +    "data-long": LONG_ATTRIBUTE
> +  },
> +  setUp: function(inspector) {
> +    SpecialPowers.setIntPref("devtools.markup.collapseAttributeLength", 2);

Services.prefs.setIntPref

@@ +113,5 @@
> +      .textContent;
> +    is(visibleAttrText, collapsed);
> +  },
> +  tearDown: function(inspector) {
> +    SpecialPowers.setIntPref("devtools.markup.collapseAttributeLength", 120);

Services.prefs.clearUserPref("devtools.markup.collapseAttributeLength")

::: devtools/client/locales/en-US/toolbox.dtd
@@ +62,5 @@
> +<!-- LOCALIZATION NOTE (options.collapseAttrs.label): This is the label
> +  -  for the checkbox option to enable collapse attributes in the Inspector
> +  -  panel. -->
> +<!ENTITY options.collapseAttrs.label "Truncate attributes">
> +<!ENTITY options.collapseAttrs.tooltip "Turning this on will enable attributes collapsing in the inspector">

Maybe the tooltip text should read something like "Truncate long attributes in the inspector"

	Brian Grinstead [:bgrins]
	Comment 18 • 8 years ago

(In reply to Brian Grinstead [:bgrins] from comment #17)
> ::: devtools/client/locales/en-US/toolbox.dtd
> @@ +62,5 @@
> > +<!-- LOCALIZATION NOTE (options.collapseAttrs.label): This is the label
> > +  -  for the checkbox option to enable collapse attributes in the Inspector
> > +  -  panel. -->
> > +<!ENTITY options.collapseAttrs.label "Truncate attributes">
> > +<!ENTITY options.collapseAttrs.tooltip "Turning this on will enable attributes collapsing in the inspector">
> 
> Maybe the tooltip text should read something like "Truncate long attributes
> in the inspector"

Helen, do you have an opinion about how this option is exposed / what it says?  Maybe if nothing else we could move the checkbox up next to the other checkbox so it lines up vertically more nicely.

	Helen V. Holmes (:helenvholmes) (:✨)
	Comment 19 • 8 years ago

I think "truncate attributes" is still a little vague since you're not sure where you're truncating them. I still stand by the label idea in comment #4:

[ ] Never truncate Inspector attributes

I like the tooltip text that's been chosen.

I'm not certain I understand what Brian's saying about moving the checkbox (I assume we're talking about the attached screenshot). Are you referring to the space between "Show browser styles", "Default color unit", and "Truncate attributes" seeming a little skewed?

	Brian Grinstead [:bgrins]
	Comment 20 • 8 years ago

(In reply to Helen V. Holmes (:helenvholmes) (:✨) from comment #19)
> I think "truncate attributes" is still a little vague since you're not sure
> where you're truncating them. I still stand by the label idea in comment #4:
> 
> [ ] Never truncate Inspector attributes
> 

Oops sorry, forgot you had a comment earlier in the bug with a suggestion.  It is already in the "Inspector" section of the options but even still it may not be clear..  What about "Truncate DOM attributes"?  My 2 cents, I prefer the positive variation since it's easier to figure out what checking the box will do because of less double negatives.  Nearly all of our options are in this form ('Enable *', 'Autoclose Brackets', 'Show *', etc).

> I'm not certain I understand what Brian's saying about moving the checkbox
> (I assume we're talking about the attached screenshot). Are you referring to
> the space between "Show browser styles", "Default color unit", and "Truncate
> attributes" seeming a little skewed?

I meant to change the order of the form elements so that it's:

[] Show Browser Styles
[] (truncate text)
Default Color Unit ----

	Helen V. Holmes (:helenvholmes) (:✨)
	Comment 21 • 8 years ago

(In reply to Brian Grinstead [:bgrins] from comment #20)
> Oops sorry, forgot you had a comment earlier in the bug with a suggestion. 
> It is already in the "Inspector" section of the options but even still it
> may not be clear..  What about "Truncate DOM attributes"?  My 2 cents, I
> prefer the positive variation since it's easier to figure out what checking
> the box will do because of less double negatives.  Nearly all of our options
> are in this form ('Enable *', 'Autoclose Brackets', 'Show *', etc).
This makes a lot of sense to me! +1

> > I'm not certain I understand what Brian's saying about moving the checkbox
> > (I assume we're talking about the attached screenshot). Are you referring to
> > the space between "Show browser styles", "Default color unit", and "Truncate
> > attributes" seeming a little skewed?
> 
> I meant to change the order of the form elements so that it's:
> 
> [] Show Browser Styles
> [] (truncate text)
> Default Color Unit ----
Ah, I understand now. I agree, this is a good idea imo.

	Brian Grinstead [:bgrins]
	Comment 23 • 8 years ago

Comment on attachment 8711374 [details] [diff] [review]
0001-Bug-1225063-add-option-devtools.markup.collapseAttri.patch

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

Thanks, this looks great!  If this try push looks good I'll check it in later: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d3419758e462

	Pulsebot
	Comment 24 • 8 years ago

https://hg.mozilla.org/integration/fx-team/rev/4f5aafee00a6

	Carsten Book [:Tomcat]
	Comment 25 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/4f5aafee00a6

	Will Bamberg [:wbamberg]
	Comment 26 • 8 years ago

-> https://developer.mozilla.org/en-US/docs/Tools/Tools_Toolbox#Inspector

...does this cover it?

	Patrick Brosset <:pbro> Reporter
	Comment 27 • 8 years ago

(In reply to Will Bamberg [:wbamberg] from comment #26)
> -> https://developer.mozilla.org/en-US/docs/Tools/Tools_Toolbox#Inspector
> 
> ...does this cover it?
Yes, perfect, thanks Will.

	Saheda Reza [:Antora]
	Comment 28 • 8 years ago

I have reproduced this according to (2015-11-16)

It's fixed on Latest Developer Edition-- Build ID (20160416004025) ,User Agent: Mozilla/5.0 (Windows NT 6.3; rv:47.0) Gecko/20100101 Firefox/47.0

Tested OS-- Windows8.1 32bit