1198073 - Class names replaced with ellipsis in inspector

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[imarkthomson]

Attachment: screenshot-201508236_1708.jpg

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:42.0) Gecko/20100101 Firefox/42.0
Build ID: 20150824004008

Steps to reproduce:

Open devtools and select an element with lots of (long-ish) class names.


Actual results:

Not all class names are shown. Firefox replaces some with elipsis character "...". See screenshot.


Expected results:

I should be able to disable this "feature" completely so I can visually scan for a given class name.

Comment 1

[Patrick Brosset <:pbro>]

From SO [1]:

"

Truncating long attribute strings is, so far, a feature of the Firefox inspector, as in some cases, attributes may be really long and make it hard to use the tool. Think of base-64 image data-URLs for example.

I understand that this feature might not always be wanted, but for this to change, the inspector's code needs to be changed, there isn't a setting you can use. 2 options:

- Increase the limit after which strings are truncated to make sure only really long attributes are,
- Or add a setting (off by default to preserve today's behavior) to turn this off entirely.

But to answer the original question, no you can't disable this (other than by changing the code, or writing an addon that would monkey-patch this).

"

Would adding a pref in about:config be enough for this? We could also expose this pref in the devtools settings panel, but before we do this, I'd like us to do bug 1192429 first. It's about adding a toolbar to the inspector, which would be a handy place to place a settings button (just like the debugger's), and clicking on it would show all sorts of inspector-related settings, like auto-collapsing of long attributes.

[1] http://stackoverflow.com/questions/32127238/how-do-i-stop-firefox-developer-tools-inspector-from-collapsing-html-elements-be/32134940

Comment 2

[imarkthomson]

I'd love to see a pref in about:config for this. There are lots of CMS plugins (I'm looking at you WordPress) that use a ridiculous number of classes to achieve their styling.

Alternately, maybe this shortening feature should only apply to data URI's and not things like class names?

Thanks!

Comment 3

[Patrick Brosset <:pbro>]

I don't have time to work on this now, but I'm available for mentoring people who'd want to get a crack at it.
First things first, start at: https://wiki.mozilla.org/DevTools/GetInvolved#Hacking
Then NeedInfo? me on the bug to ask for more information about the code.

Comment 4

[Patrick Brosset <:pbro>]

(In reply to imarkthomson from comment #2)
> Alternately, maybe this shortening feature should only apply to data URI's
> and not things like class names?
That's an interesting idea. Maybe hard to implement correctly, indeed you could have very long custom attributes too (that aren't data URIs). Maybe let's start with a pref first, and later (in another bug) see if we can make this take smart decisions about when to use an ellipsis and when not.

Comment 6

[Hugo]

Attachment: patch

Hi, this is my first mozilla bug, so I hope this is actually the right way to submit patches. As discussed, I simply added the "devtools.markup.classNameMaxLength" pref, to change the length or disable the feature (with value=-1)

Comment 7

[Patrick Brosset <:pbro>]

Comment on attachment 8682899 [details] [diff] [review]
patch

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

This is looking pretty good, thanks for the patch.
A few comments/questions though:

- I like the approach where, if you don't define the pref, then attributes are truncated to a default size, if you provide a custom size, attributes are truncated to that size, and if provide -1, then attributes aren't truncated. I think this gives a lot of power.
- But why only use this for class attributes? I guess you could argue it's the most common case, but if we do this, I'm pretty sure other use cases will come up later where someone wants to do the same with ID attributes or some other attributes. If we do it for all attributes though, does that mean we also do it for data-uri attributes too (those are handled differently in the code already, see COLLAPSE_DATA_URL_LENGTH)?
- I think we should define the new pref "devtools.markup.classNameMaxLength" with a default value of 120 in https://dxr.mozilla.org/mozilla-central/source/devtools/client/preferences/devtools.js and remove the try/catch in the MarkupView's constructor.

Asking feedback from Brian who has been working on truncating attributes in the past, I think we should hear his point of view.

Comment 8

[Brian Grinstead [:bgrins]]

Comment on attachment 8682899 [details] [diff] [review]
patch

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

Agreed on the points from Comment 7.  Specifically, I see this as a user-definable replacement for the const COLLAPSE_ATTRIBUTE_LENGTH, applying to all attributes.  So yeah, get rid of the const and add a default value in the prefs file.

I'm fine with having a different (non-configurable) default for data URIs.  If we want to make this a pref later we can, but those should be shorter than 120 char by default.

----

And also see the browser_markupview_tag_edit_07 test which could use an additional test case to make sure the full DATA_URL_INLINE_STYLE is shown in the view if the pref is set to -1.  As an example of what might be added there:

let TEST_DATA = [ ... ];

let LONG_VISIBLE_TEST_DATA = [
{
  desc: "Try to add long attribute to make sure it's not collapsed in attribute editor.",
  text: 'data-long="'+LONG_ATTRIBUTE+'"',
  expectedAttributes: {
    'data-long':LONG_ATTRIBUTE
  },
  validate: (element, container, inspector) => {
    let editor = container.editor;
    let visibleAttrText = editor.attrElements.get("data-long").querySelector(".attr-value").textContent;
    is (visibleAttrText, LONG_ATTRIBUTE)
  }
},
]

add_task(function*() {
  let {inspector} = yield addTab(TEST_URL).then(openInspector);
  yield runAddAttributesTests(TEST_DATA, "div", inspector)

  Service.prefs.setIntPref(..., -1)

  ({inspector}) = yield addTab(TEST_URL).then(openInspector);
  yield runAddAttributesTests(LONG_VISIBLE_TEST_DATA, "div", inspector)

  Service.prefs.clearUserPref(...)
});

Comment 9

[Hugo]

Attachment: collapseAttributeLength-pref.diff

Hi Patrick, Brian, thank you for the feedback, here is a new patch in which I hope I addressed all your comments.

Comment 10

[Patrick Brosset <:pbro>]

Comment on attachment 8685470 [details] [diff] [review]
collapseAttributeLength-pref.diff

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

This looks mostly good, I just have a few minor remarks (especially about making the test slightly easier to read).
Please address these and re-upload a patch, setting R+ yourself, no need to ask again for review.
If you have commit access level 1, you can push to TRY yourself, if not, please NeedInfo? me on the bug once you have uploaded the new patch so I do it.

I filed bug 1225063 to add UI in the settings panel to change this pref more easily, if you're interested in fixing this too, that'd be great. Brian would be a great mentor for this.

::: devtools/client/markupview/markup-view.js
@@ +78,5 @@
>    } catch (ex) {
>      this.maxChildren = DEFAULT_MAX_CHILDREN;
>    }
>  
> +  this.collapseAttributeLength = Services.prefs.getIntPref("devtools.markup.collapseAttributeLength");

nit: please wrap lines longer than 80 chars. See our style guide (and how to use ESLint to check this automatically): https://wiki.mozilla.org/DevTools/CodingStandards

  this.collapseAttributeLength =
    Services.prefs.getIntPref("devtools.markup.collapseAttributeLength");

@@ +2498,5 @@
>    this.node = aNode;
>    this.markup = this.container.markup;
>    this.template = this.markup.template.bind(this.markup);
>    this.doc = this.markup.doc;
> +  this.collapseAttributeLength = this.markup.collapseAttributeLength;

Not sure adding an alias here helps. I would just access this.markup.collapseAttributeLength below.

@@ +2803,5 @@
>      let collapse = value => {
>        if (value && value.match(COLLAPSE_DATA_URL_REGEX)) {
>          return truncateString(value, COLLAPSE_DATA_URL_LENGTH);
>        }
> +      return this.collapseAttributeLength == -1 ? value : truncateString(value, this.collapseAttributeLength);

Same nit comment about wrapping. This is a pattern we often use:

      return this.collapseAttributeLength == -1
             ? value
             : truncateString(value, this.collapseAttributeLength);

Also, maybe just check if the value is less than 0 to account for cases where someone sets -2 as the pref.

::: devtools/client/markupview/test/browser_markupview_tag_edit_07.js
@@ +73,5 @@
>      is (visibleAttrText, DATA_URL_ATTRIBUTE_COLLAPSED);
>    }
>  }];
>  
> +var LONG_VISIBLE_TEST_DATA = [{

Adding a new TEST_DATA array makes this test a little bit more complex to follow. Here's a suggestion:
Instead of creating this new array, just add the test item inside it at the end of the TEST_DATA array, and add 2 function properties: setup and teardown that will be used to set and clear the pref, like this:

{
  desc: "Try to add long attribute with collapseAttributeLength == -1, to make sure it isn't collapsed in attribute editor.",
  text: 'data-long="'+LONG_ATTRIBUTE+'"',
  expectedAttributes: {
    'data-long': LONG_ATTRIBUTE
  },
  setUp: function() {
    SpecialPowers.setIntPref("devtools.markup.collapseAttributeLength", -1);
  },
  validate: (element, container, inspector) => {
    let editor = container.editor;
    let visibleAttrText = editor.attrElements.get("data-long").querySelector(".attr-value").textContent;
    is (visibleAttrText, LONG_ATTRIBUTE);
  },
  tearDown: function() {
    SpecialPowers.clearUserPref("devtools.markup.collapseAttributeLength");
  }
}

Then remove everything you added inside the add_task function.

And instead, go and edit helper_attributes_test_runner.js, which is the file that contains the runAddAttributesTests code.
In this file, inside runAddAttributesTest, call the setUp and tearDown functions at the very beginning and very end, if they exist:

function* runAddAttributesTest(test, selector, inspector) {
  if (test.setUp) {
    test.setUp();
  }

  ... unchanged code ...

  if (test.tearDown) {
    test.tearDown();
  }
}

@@ +82,5 @@
> +  },
> +  validate: (element, container, inspector) => {
> +    let editor = container.editor;
> +    let visibleAttrText = editor.attrElements.get("data-long").querySelector(".attr-value").textContent;
> +    is (visibleAttrText, LONG_ATTRIBUTE)

nit: missing semi-colon at the end of this line.

::: devtools/client/preferences/devtools.js
@@ +70,5 @@
>  pref("devtools.inspector.showAllAnonymousContent", false);
>  // Enable the MDN docs tooltip
>  pref("devtools.inspector.mdnDocsTooltip.enabled", true);
>  
> +pref("devtools.markup.collapseAttributeLength", 120);

Please add a comment before this new pref, something like:
// Collapse attributes that are too long.
// Use -1 to not collapse attributes at all.

Comment 11

[Hugo]

Attachment: 0001-replace-COLLAPSE_ATTRIBUTE_LENGTH-constant-with-devt.patch

Thanks Patrick, that was an excellent review, I really appreciate it. 

I don't have commit access.

Comment 12

[Hugo]

Sorry, It looks like I had my build broken when testing the patch (I moved to git but tested against the hg one).

It looks like the pref is not set after the setUp, I didn't take a depth look yet, I just wanted to let you know the patch is broken.

Comment 13

[Hugo]

I think the inspector has to be reopened to reload the preferences, so I think I have to kept the test as Brian suggested, or find another way. What do you think Patrick?

Comment 14

[Patrick Brosset <:pbro>]

(In reply to Hugo from comment #12)
> Sorry, It looks like I had my build broken when testing the patch (I moved
> to git but tested against the hg one).
> 
> It looks like the pref is not set after the setUp, I didn't take a depth
> look yet, I just wanted to let you know the patch is broken.
(In reply to Hugo from comment #13)
> I think the inspector has to be reopened to reload the preferences, so I
> think I have to kept the test as Brian suggested, or find another way. What
> do you think Patrick?

Right, good catch. The thing with your patch is that we read the pref in the MarkupView's constructor. And the MarkupView is only instantiated once during the toolbox's lifetime.
We can do 2 things:
- Either we make the pref hot-reloadable. By that I mean that the markup-view should react to pref changes. Like we do in the rule-view:
  this._prefObserver = new PrefObserver("devtools.");
  this._prefObserver.on(PREF_ORIG_SOURCES, this._onSourcePrefChanged);
But I find this a little bit complex, probably not worth it for this feature. Especially that it would mean refreshing the whole markup-view when the pref changes
- Or, we simply change the test so that it fakes a new pref but doesn't actually change it.
One way to do this would be to change the way you call setUp and tearDown in runAddAttributesTest: make sure you pass the inspector along: test.setUp(inspector) and test.tearDown(inspector).
And then, inside the test, instead of changing the pref, do this:
setUp: function(inspector) {
  inspector.markup.collapseAttributeLength = -1;
},
tearDown: function(inspector) {
  inspector.markup.collapseAttributeLength = 120;
},

Comment 15

[Hugo]

Attachment: 0001-replace-COLLAPSE_ATTRIBUTE_LENGTH-constant-with-devt.patch

Perfect! Thanks Patrick, here is the patch with the change in the test you suggested.

Comment 16

[Patrick Brosset <:pbro>]

Comment on attachment 8690343 [details] [diff] [review]
0001-replace-COLLAPSE_ATTRIBUTE_LENGTH-constant-with-devt.patch

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

Looks good, thanks.
I've pushed this patch to TRY so we catch any potential regressions before landing it: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6690563b8bbb
I anything turns red of orange, let's take a close look at it.

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/5c614c8507b7

Comment 18

[Patrick Brosset <:pbro>]

I landed this on fx-team.
Note that commit messages are expected to have a special format, so I changed your patch to reflect the format:

Bug <bugnumber> - <description of the changes>; r=<reviewer>
So, in this case:
Bug 1198073 - Introduce a pref to set the max length of attributes or not truncate them; r=pbro

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5c614c8507b7