report: truncate long attribute values in HTML snippets#10984
Conversation
| }); | ||
| for (const attributeName of clone.getAttributeNames()) { | ||
| let attributeValue = clone.getAttribute(attributeName); | ||
| if (attributeValue.length > 100) { |
There was a problem hiding this comment.
i'd pull this 100 up to a const towards the top of the method
| for (const attributeName of clone.getAttributeNames()) { | ||
| let attributeValue = clone.getAttribute(attributeName); | ||
| if (attributeValue.length > 100) { | ||
| attributeValue = attributeValue.slice(0, 97) + '...'; |
There was a problem hiding this comment.
protip: option-; on mac is … which is a single character ellipsis.
|
before/after screenshots? :) |
oh, I see, not limiting which attributes, just making sure none of them are ridiculously long. I guess that's also not that hard to imagine :) |
|
This is technically a breaking change. Any way we could truncate in just the report but leave |
I could look into making this change in the snippet renderer instead. I think eventually we'd want to do it in page-functions for the next major release. |
|
Spoke offline: nah not worth it.
…On Mon, Jun 22, 2020, 1:09 PM Michael Blasingame ***@***.***> wrote:
This is technically a breaking change.
Any way we could truncate in just the report but leave snippet untouched?
I could look into making this change in the snippet renderer instead. I
think eventually we'd want to do it in page-functions for the next major
release.
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
<#10984 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA7CAMWMH2SLDLQGVJW4MRTRX6277ANCNFSM4OBADRPQ>
.
|
connorjclark
left a comment
There was a problem hiding this comment.
Looks good to me. pending approval on seeing a before/after (cnn.com has nice big ones).
|
Maybe we should also have a |
That would be nice, but then we run the risk of more important identifiers being cut off because they fall behind the other super long data attributes. I think the reason we opted for truncating per attribute was so that wouldn't happen. An alternative is that we could omit data-* attributes. They tend to be long and I don't know if they're actually useful in regards to element identification. |
|
spoke offline:
|
do we need to? since this adds so much more complexity i'd rather defer until we across a real example that necessitates it. |
Can you elaborate? the example in the OP is exactly why this is necessary. Just having a cap on the size of individual attributes does not prevent a lengthy node snippet when there are many attributes. |
|
So the approach I ended up taking here is to establish an "attribute character budget" of sorts. We simply keep count of the number of characters used for attribute names/values and once we've exceeded that budget we remove the remaining attributes. I'm seeing 50-60% reductions in snippet length for some of the longer snippets (1200+ characters). |
| const reOpeningTag = /^[\s\S]*?>/; | ||
| const match = clone.outerHTML.match(reOpeningTag); | ||
| if (match && match[0] && charCount > SNIPPET_CHAR_LIMIT) { | ||
| return match[0].slice(0, match[0].length - 1) + '…>'; |
There was a problem hiding this comment.
can you add some tests for this part?
There was a problem hiding this comment.
yep, currently working on it
| } | ||
|
|
||
| const reOpeningTag = /^[\s\S]*?>/; | ||
| const match = clone.outerHTML.match(reOpeningTag); |
There was a problem hiding this comment.
maybe replace with
const [match] = clone.outerHTML.match(reOpeningTag) || [];
and then drop all the match && match[0] for just match ?
connorjclark
left a comment
There was a problem hiding this comment.
some nits, but marking my approval anyway
Co-authored-by: Connor Clark <cjamcl@google.com>
Addresses #10717.
Before:
After:
