Traverse #1943
Traverse #1943
Conversation
|
Wasn't evaluate going to be done at the connector level?
|
|
AFAIK, no, the parser will have the evalute. |
|
Aghhh email answers don't put the comment in the right converation. I thought we agreed that connectors should do the evaluate and Maybe we should all sync about this? |
|
@molant maybe there is a little confusion here. The connectors will use their own evalute, but The traverse was remove from the Now the traverse is just an |
| private setFetchElement(event: FetchEnd) { | ||
| const url = event.request.url; | ||
| const elements = Array.from(document.querySelectorAll('[href],[src]')).filter((element: any) => { | ||
| const elements = Array.from(this._document.querySelectorAll('[href],[src]')).filter((element: any) => { |
There was a problem hiding this comment.
You also needed to resolve the original element for FetchEnd in other connectors too, right? Should this be a shared helper? If so, we may also need to include poster, data, and srcset for completeness.
There was a problem hiding this comment.
Are you thinking in a new util in hint?
There was a problem hiding this comment.
I'm taking a look to this and they are not compatible (at least not now). In jsdom, we have the element that is fetching the content, but not in the browser extension.
Also, I found another problem here, in case of a relative url, event.request.url will contain the full url (domain + relative url), but the element will contain the relative url, so this code won't work.
I'm thinking in something like url.endsWith(element.getAttribute('src')) || .....
Maybe there is another way, but I don't know this code too much.
What do you think @antross?
There was a problem hiding this comment.
If so, we may also need to include
poster,data, andsrcsetfor completeness.
@antross what elements use the attribute 'data'
There was a problem hiding this comment.
/*
* Possible URL sources:
*
* * <img>, <audio>, <video>, <source> and <track> can have a `src` attribute.
* E.g.: <img src="http://example.com/image.jpg">
*
* * <img> and <source> can have a `srcset` attribute.
* https://developer.mozilla.org/en-US/docs/Web/HTML/Element/source
* E.g.: <img src="image-src.png" srcset="image-1x.png 1x, image-2x.png 2x">
*
* * <video> can have a `poster` attribute.
* E.g.: <video width="480" controls poster="https://archive.org/download/WebmVp8Vorbis/webmvp8.gif" />
*
* * <object> has a `data` attribute.
* E.g.: <object data="movie.swf" type="application/x-shockwave-flash"></object>
* https://developer.mozilla.org/en-US/docs/Web/HTML/Element/object
*/There's also srcset that uses another format that we should probably take into consideration.
There was a problem hiding this comment.
I'm taking into consideration this format image-1x.png 1x, image-2x.png 2x. There is any other format?
sarvaje
force-pushed
the
traverse
branch
2 times, most recently
from
f8f9340 to
d5f8193
Compare
packages/hint/src/lib/types/html.ts
Outdated
| [key: string]: string; | ||
| }; | ||
|
|
||
| export type ParsedHTMLElement = Element & { |
There was a problem hiding this comment.
Does a ParsedHTMLElement actually include support for everything Element defines (since this seems to refer to the built-in Element type)?
I suspect not since we're using an attribs collection instead of attributes. I think it's better to explicitly define all the properties we expect here if we don't have a suitable type from parse5 to use instead of deriving from Element. I'm concerned we could accidentally reference things that don't exist here in the future otherwise.
There was a problem hiding this comment.
Also, this type should be able to be private to this file.
| export type ParsedHTMLElement = Element & { | |
| type ParsedHTMLElement = Element & { |
There was a problem hiding this comment.
Does a ParsedHTMLElement actually include support for everything Element defines (since this seems to refer to the built-in Element type)?
Nop. It support just some element properties.
packages/hint/src/lib/types/html.ts
Outdated
| return Object.entries(x).map((y) => { | ||
| return { | ||
| name: y[0], | ||
| value: y[1] as string |
There was a problem hiding this comment.
Why as string, is null possible here? If so, we should probably update our type instead of casting.
There was a problem hiding this comment.
It looks like that casting is outdated. We don't need it anymore.
|
@antross done! |
There was a problem hiding this comment.
Looks like we're not quite getting correct position information in extension-vscode (underlines appear on the line following the actual problem). Try installing the extension, then using it on the hint repo, editing packages/extension-browser/src/devtools/panel/panel.html to look something like:
<!doctype html>
<html lang="en">
<head>
<meta charset="utf8">
<title>Hints</title>
</head>
<body>
<script src="panel.js"></script>
<img src="test.png">
<button>Test</button>
</body>
</html>This will trigger hints for meta-charset-utf8 and button-type (among others). I also didn't see any reports coming from axe in this context which may suggest our eval code isn't working as expected with connector-local.
|
Also, I took this for a spin with |
|
@antross does the extension needs a zero based location? If I can reproduce your error, but If I use the local connector for the same file, I get this result: As you can see, the locations are good. P.S.: In this capture, the issue with |
sarvaje
force-pushed
the
traverse
branch
3 times, most recently
from
523a11b to
0c14004
Compare
sarvaje
force-pushed
the
traverse
branch
2 times, most recently
from
e79317f to
4f71371
Compare
|
Yeah!
hehehe, no problem!! |
There was a problem hiding this comment.
Awesome work. Just a few more changes and we can get this merge! We might want to do a release before merging everything though.
Another thing that worries me is the commit history, how do we want to tackle it? hint is a Breaking but what about the other ones? The output and public API doesn't change so they should probably be patch or minor in most cases?
|
|
||
| export default (dom: HTMLDocument, url: string): HTMLElement | null => { | ||
| const elements = dom.querySelectorAll('[href],[src],[poster],[srcset]').filter((element: any) => { | ||
| const elementUrl = element.getAttribute('href') || element.getAttribute('src') || element.getAttribute('poster'); |
There was a problem hiding this comment.
If the dom doesn't change (and IIRC it shouldn't) we should probably cache a few things like the list of all the elements with a URL and absoluteUrls. Probably something that doesn't have to be done in this PR but we should open an issue. Thoughts @webhintio/core ?
There was a problem hiding this comment.
I don't think this is going to be called more than once with the same url (if so, is because the user has something duplicated is the code). But we can cache the result of the querySelectorAll.
There was a problem hiding this comment.
Probably a premature optimization at this point. I'd wait.
There was a problem hiding this comment.
We might end up creating the same new Url quite a few times depending on how many links the page has.
Can I have a //TODO here and hopefully one day we will do #210 ?
There was a problem hiding this comment.
We might end up creating the same new Url quite a few times depending on how many links the page has.
We shouldn't have the same url repeated many often, the only scenario I can think about is when we have the same image in few places.
I will add the //TODO
| return false; | ||
| }); | ||
|
|
||
| if (elements.length > 0) { |
There was a problem hiding this comment.
Add a comment here to explain saying something like:
Even if there are multiple elements with the same URL, it's the first one that triggers the download in the browser and thus the one we should be reporting.
| const attribute = element.attributes.item(i); | ||
|
|
||
| if (attribute) { | ||
| query += `[${attribute.name}="${attribute.value}"]`; |
There was a problem hiding this comment.
Don't we have to do any escaping here? We are doing something similar in location-helpers.ts right now
There was a problem hiding this comment.
I think we can use get-element-by-url.ts to do this.
| @@ -169,10 +149,10 @@ export default class WebExtensionConnector implements IConnector { | |||
| } | |||
|
|
|||
| public evaluate(source: string): Promise<any> { | |||
| return Promise.resolve(this._window.evaluate(source)); | |||
| return Promise.resolve(eval(source)); // eslint-disable-line no-eval | |||
There was a problem hiding this comment.
Can you add a comment explaining that this script is injected into the page and that's why we are using eval (because that's what's happening, right?)
|
@molant I think everything should be done, except the commits stuff |
Pull request checklist
Make sure you:
For non-trivial changes, please make sure you also:
Short description of the change(s)
The bundle size using this solution increase less than 500kb.
I still have to test it and see that everything is working as expected.
Also I need to polish the code, right now I just wanted to have something working to see the bundle size.
@molant @antross any feedback would be great.