[1.16] Allow choosing the box model in Node::getPosition() - #737
Merged
Conversation
Contributor
|
Thanks! 🎉 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Node::getPosition() reads only the content box from DOM.getBoxModel, so there is no way to measure an element including its padding, border or margin, which is useful when rendering single page documents among other things. This picks up @aprat84's change from #726, retargeted at 1.16 since it adds new behaviour: getPosition() accepts an optional box model argument defaulting to content, so existing calls are unaffected, and the original commit is included as authored.
A follow-up commit applies some corrections. An invalid box model now throws an InvalidArgumentException, following the precedent of the screenshot format validation, rather than silently returning null, since a null return otherwise means the element could not be measured and a typo should not masquerade as that. The test is rewritten to use a data provider instead of PHP 8 attributes, because #[TestWith] requires PHPUnit 10 and the trailing comma in the parameter list is a parse error on PHP 7.4, so the original test file would have broken the PHP 7.4 and 8.0 builds, which also makes the phpstan-ignore workarounds unnecessary. The PHPStan platform constraint is left untouched as PHPStan must run on PHP 7.4 only, and a changelog entry is included.
Closes #726.