fix(select): Position incorrect if selection scrolled. #6354
Conversation
topherfangio
force-pushed
the
team/topher/fix-select-position-6190
branch
from
d488e4b
to
c69e486
Compare
|
@rschmukler This is ready for review. @ThomasBurleson If he happens to not be around, this is ready for review and Naomi would like it in the next release. |
| var oldDisplay = element[0].style.display; | ||
|
|
||
| // Set the element's display to block so that this calculation is correct | ||
| element[0].style.display = 'block'; |
There was a problem hiding this comment.
this should be in a try { } finally { }.
|
LGTM |
| @@ -1276,7 +1276,7 @@ function SelectProvider($$interimElementProvider) { | |||
| * Calculate the | |||
| */ | |||
| function calculateMenuPositions(scope, element, opts) { | |||
| var | |||
| var | |||
There was a problem hiding this comment.
Can you make all those with var? or without it cause there are var deceleration right after..
There was a problem hiding this comment.
Note how all of the following variable declarations have a comma , at the end except the last one, which has a semi-colon. The semi-colon is the stop of the var statement basically, so yes, you can do it :-)
There was a problem hiding this comment.
It wasn't a question.. i know you can aggregate var declarations into one.
I was asking you to separate it or make the following var declarations (after the semi-colon) aggregated too.
There was a problem hiding this comment.
Ah, sorry, I see now! I originally had the style code above/below this line which is why I separated it, but I can indeed move those back together now.
topherfangio
force-pushed
the
team/topher/fix-select-position-6190
branch
from
c69e486
to
b8e92d7
Compare
|
@ThomasBurleson Updated with your suggestions. Ready for review/merging. |
If the currently selected object scrolled into view because it was further down in the list, the position of the menu was incorrect upon opening because the `scrollTop` of the `contentNode` was always `0` due to the ARIA changes which set it to `display: none` upon closing. Fix by modifying the display to `block` when we run the `isScrollable` check and back afterward (usually `display: none` but we account for other options). Also fixes a slight `2px` positioning issue which made the menu not perfectly line up with the underlying selection. Fixes #6190.
topherfangio
force-pushed
the
team/topher/fix-select-position-6190
branch
from
b8e92d7
to
81acf67
Compare
| // Nothing to do | ||
| } | ||
| return isScrollable; | ||
| } |
There was a problem hiding this comment.
I unit test would be nice to have, but I want to get this in 1.0.1, so it can be added later.
ThomasBurleson
removed
the
needs: review
This is a simple unit test to ensure the fix from PR #6354 works as expected.
This is a simple unit test to ensure the fix from PR #6354 works as expected.
If the currently selected object scrolled into view because it was further down in the list, the position of the menu was incorrect upon opening because the
scrollTopof thecontentNodewas always0due to the ARIA changes which set it todisplay: noneupon closing.Fix by modifying the display to
blockwhen we run theisScrollablecheck and back afterward (usuallydisplay: nonebut we account for other options).Also fixes a slight
2pxpositioning issue which made the menu not perfectly line up with the underlying selection.Fixes #6190.