Closed Bug 893677 Opened 11 years ago Closed 11 years ago

[markup view] we should limit the size of an attribute

Categories

(DevTools :: Inspector, defect)

Product:
DevTools
Component:
Inspector
Platform:
x86
All
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Firefox 26

People

(Reporter: paul, Assigned: bgrins)

References

Details

Attachments

(1 file, 2 obsolete files)

collapse_attributes3.patch
10.25 KB, patch
bgrins
: review+
Details | Diff | Splinter Review
It's not an attribute that would be edited by the user, and it's very annoying. Let's do what Chrome does: limit the size to ~60 characters. Maybe we should do that for data urls only.
Blocks: gaia-devtools
Depends on: 899375
Assignee: nobody → bgrinstead
The implementation of this seemed straightforward - use a xul:label with a 'crop' attribute, and set the 'value' property instead of the textContent. I have this working, however it causes failures with selecting and pasting content out of the markup view (see Bug 134555). When setting it with textContent, the crop attribute does not work. So a tree that looks like this: <body dir="ltr"> <div id="container"> … </div> </body> Pastes out like this: <body dir=""> <div id=""> … </div> </body> Unless if there is a workaround for this (I'm not finding any), then the text cropping will have to be implemented using textContent rather than the label feature.
I would much prefer to avoid xul:labels in the markup view. We had many problems because of them in the past (copy/pasting and resize issues).
(In reply to Paul Rouget [:paul] from comment #2) > I would much prefer to avoid xul:labels in the markup view. We had many > problems because of them in the past (copy/pasting and resize issues). OK, I've removed the xul:label, and am just truncating the string / setting the textContent now. I have a WIP patch at: https://hg.mozilla.org/try/rev/e283f7ed4d98. I'd like opinions on whether this should only collapse data URLs, or if it should collapse anything over X characters, or collapse anything over X characters with no breaks. Given these four inputs: 1) style="background(data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAABAAAACSCAYAAABMi0qyAAAOHElEQVRoge2Ya1CTeZbG36quomZ3reVT19jtjjWNzJQ7PdmlB0dam21Xu3tH1wbBAO==);" 2) title="some content that is really very long, but has spaces so that it will wrap inside of the markup view. some content that is really very long, but has spaces so that it will wrap inside of the markup view." 3) title="somecontentthatisreallyverylongwithnobreakssomecontentthatisreallyverylongwithnobreakssomecontentthatisreallyverylongwithnobreaks" 4) href="https://mozilla.com?one=33&two=8309823098312409834233830982309831240983423383098230983124098342338309823098312409834233830982309831240983423383098230983124098342". Currently in devtools, #1 and #3 end up causing a big scrollbar to the right as it doesn't wrap at all in #3 and only moves to a new line after the ';' in #1. Cases #2 fits horizontally within the container, but can take up a lot of vertical space (especially if the markup view is not very wide). Case #4 depends a little bit on the URL and what kind of breaking characters it has as far as whether it splits or not. Though the overflow problem can be fixed easily by setting: 'word-break: break-all;' on the .attreditor as far is I can tell, so the distinction between breaking and non breaking strings can be worked around. So, which of these four should be collapsed with an … in the middle of it? I think the data URL definitely should be. The trade off with collapsing is that the data cannot be seen in full, and if you copy/paste out of the editor it will have the collapsed string only, so we probably want to be judicious with what we collapse.
> 1) > style="background(data:image/png;base64, > iVBORw0KGgoAAAANSUhEUgAAABAAAACSCAYAAABMi0qyAAAOHElEQVRoge2Ya1CTeZbG36quomZ3r > eVT19jtjjWNzJQ7PdmlB0dam21Xu3tH1wbBAO==);" Thinking about this some more, would we want to collapse the whole style attribute in this case, or would we only want to collapse if the attribute was *just* a data URL? I'm trying to think of a situation where an attribute would be set directly as a data URL. Seeing as how this is blocking Bug 893669, maybe this is something done with Gaia?
Flags: needinfo?(paul)
> or if it should collapse anything over X character Yes. Long strings are annoying. Wherever they came from. I don't think we need to be too smart here. Cutting any attribute value that is larger than a fixed size would work. But then, what do we do for editing?
Flags: needinfo?(paul)
(In reply to Paul Rouget [:paul] from comment #5) > > or if it should collapse anything over X character > > Yes. > > Long strings are annoying. Wherever they came from. I don't think we need to > be too smart here. Cutting any attribute value that is larger than a fixed > size would work. But then, what do we do for editing? The collapsed strings clearly need to be expandable just in case a user needs to view / edit the contents.
If I am not correct, the only issue here is visual clutter , and not excess of data transfer, right ? In that case, why can we not simply add a max-width + text-overflow: ellipsis to the attribute spans ?
(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #6) > (In reply to Paul Rouget [:paul] from comment #5) > > > or if it should collapse anything over X character > > > > Yes. > > > > Long strings are annoying. Wherever they came from. I don't think we need to > > be too smart here. Cutting any attribute value that is larger than a fixed > > size would work. But then, what do we do for editing? > > The collapsed strings clearly need to be expandable just in case a user > needs to view / edit the contents. I don't agree with the "clearly". Being expandable would be better, but I don't think we should really care. A long string is usually a non-human-readable string, so no need to worry to much (in Chrome, it doesn't expand).
(In reply to Girish Sharma [:Optimizer] from comment #7) > If I am not correct, the only issue here is visual clutter , and not excess > of data transfer, right ? In that case, why can we not simply add a > max-width + text-overflow: ellipsis to the attribute spans ? So ellipsis only at the end (not in the middle). I'm ok with that too, but it might be a bit hard to implement because of the word-wrapping thing. We word-wrap the attributes, which make text-overflow impossible. I would not try to be too smart here. If the string is too long, we take the start, the end, [start,"...",end].join(""), and that's it. Editing would show the ellipsis too.
(In reply to Paul Rouget [:paul] from comment #9) > I would not try to be too smart here. If the string is too long, we take the > start, the end, [start,"...",end].join(""), and that's it. Editing would > show the ellipsis too. I think it is easy enough to expand the string once editing (the editableField already supports this via the `initial` attribute). Plus it would be very annoying if you clicked into an attribute, then just pressed enter and it changed the value into the collapsed version with the "…". If we are limiting only on length, I would suggest we increase the max number of characters to somewhere between 160-200. Empirically, that is around how many characters it takes to cause two full lines of wrapping on a laptop screen, and should be safely above most user generated / edited values. One exception is a large inline style attribute, which has another representation over on the rule view. Another exception is the popular library [Modernizr](http://modernizr.com/), which adds a "class" attribute to the HTML element with ~500 characters. I *could* see someone wanting to scan that attribute when trying to debug something with their app, but most of the time it is probably just noise. Based on the feedback from everyone, I will post an updated patch with the restriction *just* on length, that also expands on editing.
I think we should simply make the attribute span inline-block with max height of 1 row and max width so that it covers one line, and simply add text-overflow:ellipses thereafter. And while editing , we can make it normal if we want to.
I'm afraid of collapsing *too much* content - this could become inconvenient for users that really want to see the whole attribute value. The concern I have with tying it to the width of the inspector panel is that we could collapse valid user-generated attributes. Even if we were able to expand it to a number of lines (say, 2) and allow word-wrapping up to that number of lines this could still be a problem. Setting the .attrvalue span to inline-block as required for this technique seems to have some side effects such as the value jumping down to the next line when it needs to be overflowed: http://content.screencast.com/users/bgrins/folders/Snagit/media/cfe7533e-1060-40b2-ac22-809398ddb3f8/2013-08-12_08-55-33.png. These type of issues are probably able to be worked around, but I'm not sure how simple the solution will end up in the end. The thing that obviously needs to be fixed is the data URL value on this page: http://davidwalsh.name/demo/data-uri-php.php (screenshot: http://i.snag.gy/YY69p.jpg). This would be fixed either by collapsing in JS, or limiting the length using CSS. One option would be to collapse a data URL to a much smaller value (say 60 characters), and collapse any other type of attribute to a higher number (say 200 characters).
If we show the whole string when you start editing, do you think that's enough for people that need to see more than we show by default? Seems like we could just limit all attributes to some small number of characters if double-clicking can get the whole value in case it's needed.
(In reply to Dave Camp (:dcamp) from comment #13) > If we show the whole string when you start editing, do you think that's > enough for people that need to see more than we show by default? > > Seems like we could just limit all attributes to some small number of > characters if double-clicking can get the whole value in case it's needed. The issue with showing it when editing only is that we do not word wrap while editing, so it is hard to find your place inside of the editor component on a large string on a single line. Is word wrapping possible in the editableField component? That said, they could always copy/paste out from the inplace editor into a different editor, or we maybe could add a context menu item to copy the attribute value to their clipboard if they right clicked on the name or value of the attribute.
Why not show the full value in a tooltip on hover? That way you almost have the best of all worlds.
Depends on: 904049
Depends on: 855523
> Is word wrapping possible in the editableField component? There are multiline editableFields (see the TextEditor in markup-view.js). I don't know if those word wrap large lines, but we could manually wordwrap maybe?
We automatically size the editor to fit the content, but maybe we could max-width that somehow.
Attached patch collapse_attributes.patch (obsolete) — — Splinter Review
Doing the simple thing here and collapsing the attributes with an ellipses when they are too long, then expanding them fully once editing begins. I do have a special case for data URLs, so they get shrunk to a smaller size than a normal attribute.
Attachment #803438 - Flags: review?(mratcliffe)
Comment on attachment 803438 [details] [diff] [review] collapse_attributes.patch Review of attachment 803438 [details] [diff] [review]: ----------------------------------------------------------------- r+ with one issue addressed: ::: browser/devtools/markupview/markup-view.js @@ +11,5 @@ > > const PREVIEW_AREA = 700; > const DEFAULT_MAX_CHILDREN = 100; > +const COLLAPSE_ATTRIBUTE_LENGTH = 120; > +const COLLAPSE_DATA_URL_REGEX = /^data.*base64/; We should try to avoid false positives here: /^data:.+;base64/
Attachment #803438 - Flags: review?(mratcliffe) → review+
Attached patch collapse_attributes2.patch (obsolete) — — Splinter Review
Updated data url regex
Attachment #803438 - Attachment is obsolete: true
Attachment #803830 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/e62454ab7c04
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
Fixes test failure with class name changing in Bug 855523.
Attachment #803830 - Attachment is obsolete: true
Attachment #804056 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/467edd1d0e1e
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/467edd1d0e1e
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 26
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: