Skip to content

Implement DOMElement::insertAdjacent{Element,Text} #11700

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jul 17, 2023

Conversation

Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't really have a look at the test cases. @theseer do you want to double check?

Comment on lines +1300 to +1400
php_dom_throw_error(SYNTAX_ERR, dom_get_strict_error(this_intern->document));
return INSERT_ADJACENT_RES_FAILED;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The spec seem to indicate that a DOMException must be thrown:

Suggested change
php_dom_throw_error(SYNTAX_ERR, dom_get_strict_error(this_intern->document));
return INSERT_ADJACENT_RES_FAILED;
php_dom_throw_error(SYNTAX_ERR, true);
return INSERT_ADJACENT_RES_FAILED;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but strangely PHP offers an "stricterror" option (default true) which allows you to get warnings instead of exceptions.
And that maybe doesn't even violate spec, because from https://webidl.spec.whatwg.org/#dfn-throw

The resulting behavior from creating and throwing an exception is language binding specific.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah PHP extension used to have the trifecto of no warning/exception, warnings, exceptions. I think mainly because prior to the introduction of exception warnings were the only way.

We probably should get rid of those and only allow silent or exception behaviour (similarly to the SQLite3 RFC) but that's future scope

@theseer
Copy link
Contributor

theseer commented Jul 17, 2023

The existing tests look good to me.

Two questions:

@nielsdos
Copy link
Member Author

The existing tests look good to me.

Thanks for having a look.

Two questions:

* Mozlla (according to https://developer.mozilla.org/en-US/docs/Web/API/Element/insertAdjacentElement) additionally throws a `TypeError` exception. Not sure if that is relevant for us.

Thrown if the element specified is not a valid element.

Not sure what that means to be honest. It seems Firefox specific. Spec doesn't define it so I'd say we should ignore this.

* The spec mentions that the `where` string should be case insensitive. All examples in the test are fully lowercase. Do we need or want to explicitly cover alternative cases?

Good point, I'll add a small test for this.

@nielsdos nielsdos force-pushed the dom-element-adjacent branch from 0eb2350 to 8019499 Compare July 17, 2023 14:40
@nielsdos nielsdos merged commit a73f38f into php:master Jul 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants