-
-
Notifications
You must be signed in to change notification settings - Fork 85
Conversation
per HTTPArchive/httparchive.org#141 - this is slightly complicated by the fact that I think we don't want to include native elements that have a shadow root like inputs and video in this, so... someone should probably review this to see if it makes sense.
Do you know of a page that has a shadow root? We can plug the custom metric into WPT and see if the results are expected. FWIW I tested it on facebook.com and got To reproduce, on the WPT home page go to the "Custom" tab of Advanced Settings and enter this under Custom Metrics:
|
There's something funny going on there I guess with returning a boolean? If I run it on facebook.com it is empty in the details table, if I run it on https://uncovered-mask.glitch.me/ then it is 1. IF I prefix a So, at least for these two tests I think it is working 'correctly' it's just a matter of what we want there I guess? I guess if it had to be returning a string, giving back "true" or "false" explicitly would make more sense? I'd like like someone to review the actual logic there too though. Maybe @zcorpan can? |
This has two consecutive |
This code would include SVG elements, but I don't know if any SVG elements have a |
Fix double || in regexp and stringify response for consistency
Hmm yeah... There are a very few svg elements with a dash, right? It looks like most of them are deprecated and looking through a few I can't imagine how they could have a shadow.... Also, now that you mention it, I'm not entirely sure what I was thinking looking at the 'shortcut' I was trying to take to that using ! Kind of makes me wish we had a I think this is very close, it may still need a tweak I'm just not sure what yet... I guess the challenge here is that it just says "does" or "doesn't" and if we match something we shouldn't that would make the data very misleading. Maybe let's leave it open and think on it some for a few days? |
Ah... now I think I remember what I was thinking, though, this seems backward -- I guess realistically we could check |
HTMLUnknownElement inherits from HTMLElement, so you still would need to check both if you want to limit to "known" HTML elements. |
Yeah, so... something like this expanded out with comments form? function isValidCustomElementName(el) {
// it's got to have a dash
if ((el.tagName.indexOf('-') !== -1) {
// it has to be both an HTMLElement this prevents us from getting
// any dasherized elements that could exist in 'other embdedded'
if (el instanceof HTMLElement) {
// finally, it actually has to be defined as a custom element
return !(el instanceof HTMLUnknownElement)
}
}
return false
} |
Yes |
Update the way we check for valid custom element name after some discussion in order to avoid potential false positives.
@bkardell have you had a chance to test this on WPT? |
Fixed broken paren. Tested on Web Page Test via custom metrics - returns explicit true or false. Assuming links hold up, these should illustrate successful runs.. https://www.webpagetest.org/custom_metrics.php?test=190514_AV_0cdf6cffade2c8b54dc205d6b3ac596a&run=1&cached=0 https://www.webpagetest.org/custom_metrics.php?test=190514_X3_773a477c083e5705cd680858be91f31e&run=1&cached=0
Sorry, no I hadn't. Have now, updated the pull including links to (I hope, assuming the links hold) the result runs. |
Ok great, thanks. Where can I find those links? FWIW I tested this on facebook and the glitch URLs from earlier and I'm getting false/true respectively. Let me know if you're happy with the results and I'll merge the PR. We should get initial data in the June crawl, available in early July. |
@bkardell closing the loop, is this PR ready to merge? |
Yes, I'm happy with it. The links are in the pull request comments, sorry I was traveling all last week |
per HTTPArchive/httparchive.org#141 - this is slightly complicated by the fact that I think we don't want to include native elements that have a shadow root like inputs and video in this, so... someone should probably review this to see if it makes sense.