fix popper's behavior when it doesn't have any parents (#136) #137
Conversation
| will be 'false positive'. Because getOffsetParent(element) returns html node, and scrollParent would be the body node. | ||
| Hence the additional check for nodeNames. | ||
| */ | ||
| } else if (!(getOffsetParent(element).nodeName === 'HTML' && scrollParent.nodeName === 'BODY') && getOffsetParent(element).contains(scrollParent)) { |
There was a problem hiding this comment.
getOffsetParent should be cached here instead of get called twice.
|
May you add a test for this case? |
| rect.right += scrollLeft; | ||
| } else { | ||
| const offsetParent = getOffsetParent(element); | ||
| if (!(offsetParent.nodeName === 'HTML' && scrollParent.nodeName === 'BODY') && offsetParent.contains(scrollParent)) { |
There was a problem hiding this comment.
Sorry if I'm being picky, I hope you don't mind.
!(offsetParent.nodeName === 'HTML' && scrollParent.nodeName === 'BODY') is quite confusing to read, I was thinking that we could split it into multiple variables to make it easier to understand what's happening here.
Below you find my attempt, I'm not sure if it's correct though...
const offsetParent = getOffsetParent(element);
const offsetParentIsHTML = offsetParent.nodeName === 'HTML';
const scrollParentIsBody = scrollParent.nodeName === 'BODY';
const isScrollParentBodyChildOfHTML = offsetParentIsHTML && scrollParentIsBody;
if (!isScrollParentBodyChildOfHTML && offsetParent.contains(scrollParent)) {
There was a problem hiding this comment.
Don't mind of course. isBodyChildOfHTML doesn't make a lot of sense though, since body is always a child of html. But I see your point, I'll make it more readable.
|
About test: I was planning to work on unit tests for the rest of the functions on the next weekend (can't do that sooner), so it will be covered in there. Writing functional tests is a nightmare, maybe need to do something about them as well... This fix doesn't break anything, all the existing tests are green locally. |
|
I was thinking about a simple functional test like the others we already have, unit tests make sense to me to check stuff that doesn't interact with DOM (IMHO) so I wouldn't bother trying to write an unit test for this case. |
|
By the way, SauceLabs is in maintenance until Monday so we have time before we are able to merge this PR. |
|
I would strongly disagree with the 'simple, like others we have' :D There has to be a better way to do this... Done, renamed and tested. |
| // When a popper doesn't have any positioned or scrollable parents, `offsetParent.contains(scrollParent)` | ||
| // will return a "false positive". This is happening because `getOffsetParent` returns `html` node, | ||
| // and `scrollParent` is the `body` node. Hence the additional check. | ||
| const isParentScrolledOrPositioned = !(offsetParentIsHTML && scrollParentIsBody); |
There was a problem hiding this comment.
I don't get why this is called isParentScrolledOrPositioned? We don't check if the parent is scrolled nor if it's positioned...
There was a problem hiding this comment.
Because:
getOffsetParent(element) - returns a positioned parent or HTML if there is none.
scrollParent = getScrollParent(parent) - returns a scrollable parent or BODY if there is none.
Which means that if we want to know if our popper isn't inside some container that is positioned or scrollable, we need offsetParent to be HTML and scrollParent to be BODY.
isScrolledParentBodyAndOffsetParentHtml doesn't seem like a good idea.
There was a problem hiding this comment.
I see, but isParentScrolled means that we are checking that the parent element has a scrollTop or scrollLeft value different from 0.
There was a problem hiding this comment.
Yeah, you're right... Can you change it to something that you think is better?
|
@nadiam84 any idea to make the tests simpler is more than welcome 🙂 |
| // When a popper doesn't have any positioned or scrollable parents, `offsetParent.contains(scrollParent)` | ||
| // will return a "false positive". This is happening because `getOffsetParent` returns `html` node, | ||
| // and `scrollParent` is the `body` node. Hence the additional check. | ||
| else if (getOffsetParent(element).contains(scrollParent) && scrollParent.nodeName !== 'BODY') { |
There was a problem hiding this comment.
@nadiam84 does it make sense? I avoided to check that offsetParent is html because we already know that scrollParent is child of offsetParent.
So, if body is bothoffsetParent and scrollParent , it will return false because body doesn't contains itself.
If offsetParent contains scrollParent AND scrollParent is body, then offsetParent must be html
Also, doing so we don't have to invent a name for the variable! 😜
There was a problem hiding this comment.
body doesn't contains itself Actually it does ;) But all's good I think, it's the same situation. And tests are green :)
Solves this: #136