Fix toBeVisible visibility #428
Conversation
| * 1. One set of properties allow parent elements to fully controls its sub-tree visibility. This | ||
| * means that if higher up in the tree some element is not visible by this criteria, it makes the | ||
| * entire sub-tree not visible too, and there's nothing that child elements can do to revert it. | ||
| * This includes `display: none`, `opacity: 0`, the presence of the `hidden` attribute`, and the |
There was a problem hiding this comment.
Note that you can have <div hidden style="display: block" /> which will visible but may be excluded from the a11y tree. It should be documented if display or hidden has priority in your implementation.
| * 1. One set of properties allow parent elements to fully controls its sub-tree visibility. This | ||
| * means that if higher up in the tree some element is not visible by this criteria, it makes the | ||
| * entire sub-tree not visible too, and there's nothing that child elements can do to revert it. | ||
| * This includes `display: none`, `opacity: 0`, the presence of the `hidden` attribute`, and the |
There was a problem hiding this comment.
I wouldn't include opacity to be honest. 0.00001 will suddenly be considered visible? Opacity also has no relevance to a11y tree inclusion.
There was a problem hiding this comment.
Ok, but that's a separate discussion. Right now this is not new functionality, and I'm merely maintaining backward compatibility.
I agree that 0.0001 could be considered as such, but this was a compromise, because there's no arguing that opacity: 0 makes things not visible. toBeVisible was never about the presence of things in the a11y tree. It was more about actual visibility with your eyes.
I'll open an issue to discuss this. But I will not remove in this PR, as it will be a breaking change.
There was a problem hiding this comment.
Makes sense. I missed, that you already included opacity.
| * This includes `display: none`, `opacity: 0`, the presence of the `hidden` attribute`, and the | ||
| * open state of a details/summary elements pair. | ||
| * | ||
| * 2. The other aspect influencing if an element is visible is the CSS `visibility` style. This one |
There was a problem hiding this comment.
nit:
| * 2. The other aspect influencing if an element is visible is the CSS `visibility` style. This one | |
| * 2. The other aspect influencing if an element is visible is the CSS `visibility` property. This one |
That's how these are named officially i.e. in the spec.
| * open state of a details/summary elements pair. | ||
| * | ||
| * 2. The other aspect influencing if an element is visible is the CSS `visibility` style. This one | ||
| * is also inherited. But unlike the previous case, this one can be reverted by child elements. |
There was a problem hiding this comment.
What is "also inherited" in this context? Neither display nor opacity are inherited CSS properties. The use of "inherited" is ambiguous in this context.
There was a problem hiding this comment.
Maybe "inherited" is not the best word. What I mean is that setting display: none, opacity: 0, or visibility: hidden in a parent element always hides the child elements too. But, unlike with the former two, the last one is reversible: child elements can make themselves visible over its parent's wish.
I'll reword to not use the word inherit to avoid confusion.
| * Hence, the apprach taken by this function is two-fold: it first gets the first set of criteria | ||
| * out of the way, analyzing the target element and up its tree. If this branch yields that the | ||
| * element is not visible, there's nothing the element could be doing to revert that, so it returns | ||
| * false. Only if the first check is true, if proceeds to analyze the `visibility` CSS. |
There was a problem hiding this comment.
That's an interesting trade-off. You're basically assuming that checking visibility is more expensive than walking up the tree and checking display etc on every parent.
How was this trade-off motivated?
In the other Testing Library implementations we're checking visibility on the element first since that allows us to potentially bail out of walking up the tree i.e.
function isElementVisible(element) {
- return isElementTreeVisible(element) && isElementVisibilityVisible(element)
+ return isElementVisibilityVisible(element) && isElementTreeVisible(element)
}There was a problem hiding this comment.
I was honestly not thinking of performance, and the comment talks more about the logic we follow. Both branches walk up the tree, and either of these branches could bail out more quickly than the other. There's no way to tell.
Though the check for visibility is simpler, so maybe you're right that on average it could be faster. I'm ok with making this change.
Clarified below: #428 (comment)
| function isVisibleSummaryDetails(element, previousElement) { | ||
| return element.nodeName === 'DETAILS' && | ||
| previousElement.nodeName !== 'SUMMARY' | ||
| ? element.hasAttribute('open') |
There was a problem hiding this comment.
question not directly related to this PR: Is the attribute or property relevant i.e. when I set someDetailsElement.open = false will element.hasAttribute('open') return true?
There was a problem hiding this comment.
I'm honestly not sure. I did not even implement the support for summary/detail. It may be worth checking.
| const {getComputedStyle} = element.ownerDocument.defaultView | ||
| const {visibility} = getComputedStyle(element) | ||
| return visibility || getElementVisibilityStyle(element.parentElement) |
There was a problem hiding this comment.
Why do you walk up the tree here? visibility should never be falsy unless the user agent has an incorrect implementation of the CSS spec. visibility has an initial value of visible
| @@ -1,33 +1,63 @@ | |||
| import {checkHtmlElement} from './utils' | |||
|
|
|||
| function isStyleVisible(element) { | |||
| function getElementVisibilityStyle(element) { | |||
| if (!element) return 'visible' | |||
There was a problem hiding this comment.
This is a bit odd. If the element is falsy it should be considered "visible"?
There was a problem hiding this comment.
Hmmm, good point. This clarifies your point about bailing out quickly. I'll make the change.
There was a problem hiding this comment.
Yeah, for visibility you don't need to walk up since it the computed visbility of parents doesn't matter. only the computed visibility of the element asserted on matters.
This was relevant in earlier versions of JSOM where visibility wasn't inherited.
| expect(subject).not.toBeVisible() | ||
| expect(() => expect(subject).toBeVisible()).toThrowError() | ||
| describe('with the "hidden" attribute', () => { | ||
| it('considers an element to not be visible', () => { |
There was a problem hiding this comment.
| it('considers an element to not be visible', () => { | |
| it('considers an element as not visible', () => { |
Skipped the modal test as the matcher will be fixed in the future. testing-library/jest-dom#428
What:
Fixes #209 (comment)
Why:
Because
.toBeVisible(element)is not consistent with how CSSvisibilityworks.How:
By fixing the logic that calculates how visibility is determined. Enough details are present in the new code in the form of comments explaining the logic.
Checklist: