DocumentFragment->innerHTML missing PHPDoc
g105b opened this issue · 8 comments
A DocumentFragment object exposes its innerHTML property, just like other Element objects do. This isn't a feature of the native DOMDocument, so it has to be exposed to IDEs using PHPDoc. Currently, accessing the innerHTML property works fine, but IDEs do not recognise the property.
Did you mean adding @property string $innerHTML Gets or sets the HTML syntax describing the element's descendants to the class phpdoc comment ? If so, I guess the other properties of Element class such as outerHTML, innerText etc. should be documented too ?
To test the innerHTML property and to get to know the PhpGt/Dom library, I created the following:
use Gt\Dom\HTMLDocument;
require("../vendor/autoload.php");
$document = new HTMLDocument('<html lang="en"><head><title>Fragment Test</title></head><body></body></html>');
$frag = $document->createDocumentFragment();
$div = $document->createElement('div');
$div->classList->add('toggler');
$p = $document->createElement('p');
$p->appendChild($document->createTextNode('bla bla bla'));
$div->appendChild($p);
$frag->appendChild($div);
// bla bla bla
var_dump($frag->innerText);
// null
var_dump($frag->innerHTML);
$document->body->appendChild($frag);
// <div class="toggler"><p>bla bla bla</p></div>
var_dump($div->outerHTML);
It seems the innerHTML is null ?
That's interesting, @speich as you've found an extra bug caused by the reliance on DOMDocument's existing functionality.
The original issue was tracking the fact that IDEs didn't "know" about the innerHTML property. The Element describes its property, but DocumentFragment does not. See here:
You can see that my IDE understands that there are "children" and "childElementCount" properties, even though these are added using PHPDoc, but in this next screenshot there is no mention of innerHTML - also note that I'm getting wiggly underlined lines under the use of the property, indicating that it's being accessed by a magic method, rather than a documented or concrete function.
What you've inadvertantly found out with your test is that the innerHTML property doesn't actually provide the fragment's content until after it's been attached to the DOM.
To circumvent the strange hierarchy of PHP's native DOMDocument, this repo uses what I've called "live properties" for properties that represent live/changing data. If you look in the Element's implementation, you see an innerHTML getter method, but the same can't be said for the DocumentFragment class.
I think you should be able to copy the innerHTML getter/setter over from Element, although be aware that the DocumentFragment is a strange one, as it doesn't have any parent node until it's attached to the document.
If you'd like to help fix this, the unit test will be really easy to write so that it proves failure - on the DocumentFragmentTest class, you could create a new document fragment (shown in existing tests), then importHTML (also shown in existing tests) and check its innerHTML is not null. This will fail, which is a good starting point to get to before fixing.
If you need any extra assistance with the way the project is set up, I'd love to help you out to get another contributor on board :)
What you've inadvertantly found out with your test is that the innerHTML property doesn't actually provide the fragment's content until after it's been attached to the DOM.
Actually, if I call innerHTML after appending the fragment to the body, it is also null:
$document->body->appendChild($frag);
// null
var_dump($frag->innerHTML);
which according to MDN is the correct behavior:
Doing this moves the fragment's nodes into the DOM, leaving behind an empty DocumentFragment.
I'll give it a try. Do you agree, that the test should check that the property innerHTML is not null before attaching it to the DOM and that it is null after and should that be one or two tests?
I was going to say yes to your first question, but then I thought it would be best to check what the behaviour is in JavaScript and try to match that. Weirdly, a new document fragment does have a null innerHTML before it is set (rather than an empty string).
It's always a good idea to check JavaScript behaviour when implementing any functionality in this repo.
Personally, I don't get caught up about putting two similar assertions within the same test. If you're testing for the same thing, I think it's fine to bundle them up into the same test. Rule of thumb: if your test's name has to use the word "and" or "or" or "if", then it probably should be another test (thanks to Uncle Bob for that one).
Closing after checking MDN (https://developer.mozilla.org/en-US/docs/Web/API/DocumentFragment) to confirm there should be no innerHTML property of a fragment.