Add Custom Element adorner to custom elements in Elements tree [devtools/devtools-frontend : main]
Kateryna Prokopenko (Gerrit)
Kateryna Prokopenko voted and added 1 comment![]( "Open in Gerrit")
Votes added by Kateryna Prokopenko
| Auto-Submit | +1 |
1 comment
Hi Nicholas, could you please take a look?:)
Related details
- Nicholas Roscino
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicholas Roscino (Gerrit)
Nicholas Roscino added 4 comments![]( "Open in Gerrit")
Hey Kateryna! I added a couple of comments!
if (this.nodeType() !== Node.ELEMENT_NODE) {I did a little bit of experimenting here while reviewing and in case of an svg tag like `<font-face-src>` this check will not be enough to prevent it being marked as a custom element. Let me know if I am missing something!
it('custom-element adorner click reveals definition in sources', () => {The definition of the test mentions a click action, but the test itself is not checking any click behaviors. Since adding the additional assertion here might complicate the test with stubs maybe we can check this as an e2e test. I don't have a strong opinion on this tho. WDYT?
DEFAULT_VIEW({NIT: Is the reformat intended here?
Related details
- Kateryna Prokopenko
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kateryna Prokopenko (Gerrit)
Kateryna Prokopenko voted and added 4 comments![]( "Open in Gerrit")
Votes added by Kateryna Prokopenko
| Auto-Submit | +1 |
| Commit-Queue | +1 |
4 comments
Hey Nicho, please take another look?
if (this.nodeType() !== Node.ELEMENT_NODE) {I did a little bit of experimenting here while reviewing and in case of an svg tag like `<font-face-src>` this check will not be enough to prevent it being marked as a custom element. Let me know if I am missing something!
I've looked into it some more, it seems we can check against the list of exceptions that is not supposed to change according to the spec
https://html.spec.whatwg.org/multipage/custom-elements.html#custom-elements-core-concepts
it('custom-element adorner click reveals definition in sources', () => {The definition of the test mentions a click action, but the test itself is not checking any click behaviors. Since adding the additional assertion here might complicate the test with stubs maybe we can check this as an e2e test. I don't have a strong opinion on this tho. WDYT?
Done
DEFAULT_VIEW({NIT: Is the reformat intended here?
That's presubmit's fault, has to upload with --bypass-hooks now to avoid the formatting, let's see if the dry run passes like this. If not, I guess we'll have to keep the formatting changes too
Related details
- Nicholas Roscino
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kateryna Prokopenko (Gerrit)
Kateryna Prokopenko voted and added 1 comment![]( "Open in Gerrit")
Votes added by Kateryna Prokopenko
| Auto-Submit | +1 |
1 comment
DEFAULT_VIEW({Kateryna ProkopenkoNIT: Is the reformat intended here?
That's presubmit's fault, has to upload with --bypass-hooks now to avoid the formatting, let's see if the dry run passes like this. If not, I guess we'll have to keep the formatting changes too
Yeah, the formatting is something presubmit want to necessarily have and the tests fail without it
Related details
- Nicholas Roscino
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicholas Roscino (Gerrit)
Nicholas Roscino added 4 comments![]( "Open in Gerrit")
Thank you Kateryna. I added another couple of minor comments, LMKWYT! :)
if (this.nodeType() !== Node.ELEMENT_NODE) {Kateryna ProkopenkoI did a little bit of experimenting here while reviewing and in case of an svg tag like `<font-face-src>` this check will not be enough to prevent it being marked as a custom element. Let me know if I am missing something!
I've looked into it some more, it seems we can check against the list of exceptions that is not supposed to change according to the spec
https://html.spec.whatwg.org/multipage/custom-elements.html#custom-elements-core-concepts
Sounds good! I think we should also exclude all the XML nodes completely so that we avoid any custom xml tag to be captured as custom element.
DEFAULT_VIEW({Kateryna ProkopenkoNIT: Is the reformat intended here?
That's presubmit's fault, has to upload with --bypass-hooks now to avoid the formatting, let's see if the dry run passes like this. If not, I guess we'll have to keep the formatting changes too
Ok! no problem, wanted just to make sure if it was a previous wrong formatting fixed or something automatic change
constructorObject.release();I might have overlook something but I think that in case of any error that doesn't allow us to reach this point we are creating a memory leak. We should release the object in any case. Since we are in an async function we could use `try {} finally {}` WDYT? If I am missing something let me know!
Related details
- Kateryna Prokopenko
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |