Skip to content
This repository was archived by the owner on Aug 16, 2021. It is now read-only.

Conversation

@dmarchuk
Copy link
Contributor

Improves 6d48805 with comment from @Jekedorff. Sending PR so the fix could get to master branch ASAP.

@arendjr
Copy link
Owner

arendjr commented May 31, 2017

Can you tell me what this improvement does, exactly? The original comment said that fromElement might not be properly supported across browsers, but so far I've noticed no issue. I found some documentation suggesting Firefox might be the only browser not supporting fromElement, but I've tested it with Firefox and scrolling seems to work fine (Linux).

Also, this fix is quite hard to understand and seems rather brittle, so it would likely break again in the future. I wouldn't merge this without further evidence of browser support and an improvement to make this fix more robust and understandable.

@dmarchuk
Copy link
Contributor Author

dmarchuk commented May 31, 2017

The original comment mentions that because FF doesn't have property fromElement, suggested fix (which is already in the master branch) causes selectivity dropdown being still open when bluring the input or dropdown itself. So now the only way to close the dropdown in FF is to click again on the input itself. Recordings proving the bug in FF: Mac and Windows.

Also, this fix is quite hard to understand and seems rather brittle, so it would likely break again in the future.

You are definitely right, I've made an improvement to the if statement so it's more transparent, understandable and cleaner in 7216241. I also added a comment there. I used contains() method and relatedTarget property - both are supported in all major browsers.

Recordings prooving fix of the bug in FF: Mac and Windows.

Tested on:
Windows: Edge 15, IE11, FF53, Chrome 58, Opera 45
Mac: Safari 10, FF53, Chrome 58, Opera 45

@arendjr
Copy link
Owner

arendjr commented May 31, 2017

I see, I indeed missed the fact the dropdown had become hard to close on Firefox.

This fix indeed also seems much more robust this way, thanks!

@arendjr arendjr merged commit 0610983 into arendjr:master May 31, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants