Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug] Source code snippets can be too large [0.1] #2211

Closed
antross opened this issue Apr 11, 2019 · 6 comments

Comments

Projects
None yet
3 participants
@antross
Copy link
Member

commented Apr 11, 2019

Source code snippets in reports from hints include the entire outerHTML of a tag. This works well for items like <link>, but when a hint reports something on <html> a serialization of the entire document gets included.

These should be fixed to at least be truncated, but perhaps to only include the start tag of the element instead.

@antross antross added the type:bug label Apr 11, 2019

@antross antross added this to the 1904-2 milestone Apr 12, 2019

@molant

This comment has been minimized.

Copy link
Member

commented Apr 15, 2019

I think this should be done at the report level. Otherwise we will have to update all the packages to repeat the code.

What about:

  • Include only the opening tag (regular expression?)
  • Check the size and if it's still to long truncate it
    • How can truncate this correctly? I don't think we are providing the position relative in the element so if the tag is too long we might have to do something like <div [...] failingAttribute="blah"> because otherwise we might crop it

Or maybe we just use the opening tag because it should still be a lot shorter than what we are providing right now.

@webhintio/contributors, thoughts?

@antross

This comment has been minimized.

Copy link
Member Author

commented Apr 15, 2019

I'm okay just including the opening tag regardless of length for now.

@antross

This comment has been minimized.

Copy link
Member Author

commented Apr 15, 2019

Thinking about this some more, I do think if the overall content is small enough we could still include the outerHTML. This may be useful for leaf items like <button>. Maybe we do a size check before switching to just the opening tag?

@antross

This comment has been minimized.

Copy link
Member Author

commented Apr 15, 2019

Also because report deals with different types of source code (e.g. CSS in addition to HTML) I'm not sure it's the right place to put this logic. We'd quickly end up with sniffing logic to determine what type of source code is being reported (though perhaps we should be able to specify the source code type anyway when calling report)...

Maybe a helper on our HTMLElement type to get the snippet would be better, and then updating hints to call this instead of outerHTML.

@molant

This comment has been minimized.

Copy link
Member

commented Apr 16, 2019

We'd quickly end up with sniffing logic to determine what type of source code is being reported (though perhaps we should be able to specify the source code type anyway when calling report)...

That will be a bit of a change. The printing of the HTML is done already in report, the interesting part looks like this:

if (element) {
     // When element is provided, position is an offset in the content.
    position = this.findProblemLocation(element, position);
    sourceCode = element.outerHTML.replace(/[\t]/g, '    ');
}

So if there's an element we do the outerHTML. Haven't checked what we are doing for JS or CSS but maybe element should be a bit more abstract and have a toString() that's smart enought to know what to do? And we will have an implementation for each type of resource we need.

The other option is to have other properties in the options the same way we have element (css, js, etc.) but that doesn't scale very well...

@sarvaje

This comment has been minimized.

Copy link
Member

commented Apr 16, 2019

We already have an option codeSnippet that overwrite any other value. That is the option I'm using in hint-compat-api to pass the code snippet.

@sarvaje sarvaje assigned sarvaje and unassigned sarvaje Apr 16, 2019

@molant molant modified the milestones: 1904-2, 1905-1 Apr 25, 2019

@antross antross self-assigned this May 8, 2019

@molant molant closed this in #2407 May 8, 2019

molant added a commit that referenced this issue May 8, 2019

@antross antross changed the title [Bug] Source code snippets can be too large [Bug] Source code snippets can be too large [0.1] May 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.