Add missing HTMLStyleElement.innerHTML getter #1323
Conversation
Remove sinon usage from HTMLAnchorElement test
Bugfix iframe navigation handling
Centralize window.location in WindowBase
Bugfix worker baseUrl args
Remove Node insertAfter method
Make Node insertBefore work with null reference node
Custom protocol handlers for fetch
WebGL getContextAttributes behavior
WebGL instanceof inheritance
Bump window-xhr version
Bugfix fetch blob support
Add iterable support for DOM attributes
WebSocket origin header setting support
Add some HTMLIFrameElement stub methods
Add MediaStream stub methods
Bugfix Element append() method
| get innerHTML() { | ||
| return super.innerHTML; | ||
| } | ||
| set innerHTML(innerHTML) { |
There was a problem hiding this comment.
Would it be more correct to also construct and attach e.g. a Text node in the setter?
There was a problem hiding this comment.
I'm not sure, but the intent of this PR is to fix the bug introduced by defining set innerHTML without defining get innerHTML. Forwarding to super.innerHTML takes care of the issue.
There was a problem hiding this comment.
Does this correctly return the set .innerHTML, or just default to the empty string?
If the superclass handles this, is it more appropriate to simply remove the interception on this subclass and let the parent class handle the innerHTML logic?
More generally, we should take the opportunity to "do the right thing" with regard to innerHTML setting on HTMLStyleElement. I'm confident many libraries in the wild depend on getting this correct.
There was a problem hiding this comment.
I think this is proof that you will go to any length not to merge pull requests from outside contributors (or at least from me). It speaks to the effectiveness of this project.
Even simple bug fixes are endlessly debated, analyzed, and random "what if?" scenarios are brought up for no apparent reason.
There is zero – and I do mean zero – reason not merge this PR. Your code for HTMLStyleElement is broken. This PR fixes it.
Merge it and make it better, if you want. Or do your usual thing of let it sit for 3 months and then do the same exact thing yourself in a slightly different way.
There was a problem hiding this comment.
(Yes, libraries do rely on HTMLStyleElement .innerHTML actually returning the innerHTML. This is exactly the problem I ran into with exokit DOM, and why I spent time submitting this PR. It actually breaks for actual websites right now.)
4 participants
Without this PR,
document.createElement('style').innerHTMLincorrectly returnsundefined:With this PR,
.innerHTMLis correctly returned: