[fix] Setting 0 for height/width on components.html and components.iframe . #12479
[fix] Setting 0 for height/width on components.html and components.iframe . #12479sfc-gh-lwilby merged 6 commits intodevelopfrom
Conversation
…ml backwards compatibility
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) ✅ license/snyk check is complete. No issues have been found. (View Details) |
✅ PR preview is ready!
|
sfc-gh-lwilby
added
type:bug
| [ | ||
| { width: 0, height: 100 }, | ||
| getDefaultStyles({ | ||
| width: "auto", |
There was a problem hiding this comment.
This is the default, so no change.
| minStretchBehavior?: MinFlexElementWidth | ||
| } | ||
|
|
||
| const isPositiveNumber = (value: unknown): value is number => |
There was a problem hiding this comment.
This is used for validating pixelHeight and pixelWidth which are on the proto and validated before being set, so we shouldn't expect 0 measurements from something like a resizeObserver. We should only expect it when the python function allows it and sets the proto that way.
| expect(zero_html_container).to_have_attribute("width", "0px") | ||
| expect(zero_html_container).to_have_attribute("height", "0px") | ||
| expect(zero_html_container).to_have_css("width", "0px") | ||
| expect(zero_html_container).to_have_css("height", "0px") |
There was a problem hiding this comment.
suggestion: Consider removing the to_have_attribute calls since that asserts implementation details, whereas the to_have_css asserts what is actually rendered in the browsers. The to_have_css assertion should be both more powerful and less brittle, since the underlying implementation details may change over time.
There was a problem hiding this comment.
For my own understanding, if there's multiple classes with width and one of them has 0, would the to_have_css check come out to be true? And then if there's a width: 12 !important such that the resulting width is 12, would to_have_attribute("width", "0px") fail?
There was a problem hiding this comment.
if there's multiple classes with width and one of them has 0, would the to_have_css check come out to be true?
It depends on which class wins based on CSS Precedence.
The big thing to note here is that to_have_attribute is asserting against what is literally in the DOM element, basically whatever is here: <div width="0">.
Note that CSS classes do not write this attribute to the DOM, therefore to_have_attribute is a brittle check. The other nuance here is that it looks like Emotion does write the width attribute, but that is an implementation detail, and one that we shouldn't rely on.
And then if there's a width: 12 !important such that the resulting width is 12, would to_have_attribute("width", "0px") fail?
Similar to above, to_have_attribute will fail. to_have_css("width", 0) would also fail.
I hope all of this info illustrates why to_have_css is a better assertion overall.
There was a problem hiding this comment.
Yup, that helps clarify a bunch. Thanks for the great explanation. :D
There was a problem hiding this comment.
Yeah, good point. The to_have_attribute assertion was added by the agent during an iteration to fix a failure, it was the wrong approach and the root cause was addressed, but I decided to leave both test assertions. I can't remember now why I thought I should leave both, but your argument makes a lot of sense.
3 participants
Describe your changes
GitHub Issue Link (if applicable)
Fixes #12454
Testing Plan
Contribution License Agreement
By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.