Skip to content

Incorrect percentage in scroll events when scroll bar enabled but not shown #6474

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
cwalther opened this issue Nov 21, 2017 · 6 comments · Fixed by #6534
Closed

Incorrect percentage in scroll events when scroll bar enabled but not shown #6474

cwalther opened this issue Nov 21, 2017 · 6 comments · Fixed by #6534

Comments

@cwalther
Copy link

When a grid (v4.0.11) has enableHorizontalScrollbar and enableVerticalScrollbar set to uiGridConstants.scrollbars.ALWAYS (default) and its content is high enough to require scrolling but not wide enough, so that no horizontal scroll bar is actually shown, then the scrolling percentage reported in vertical scroll events is incorrectly scaled.

http://plnkr.co/edit/ovNv8v1ibKX1KwKeIait?p=preview
Steps to reproduce: open console, scroll the grid all the way to the bottom
Expected result: output "percentage 1"
Actual result: output "percentage 0.9954257565095004"

The reason seems to be that GridRenderContainer.getVerticalScrollLength() unconditionally takes into account the height of the horizontal scroll bar, whether or not it is actually there.

I am not sure how this would best be fixed. Comparing clientHeight and offsetHeight of the ui-grid-viewport seems like it would work to detect a space-occupying horizontal scroll bar, however the involved controller objects don’t seem to have direct access to the DOM elements to do that. (Note that the criterion is whether the scroll bar occupies space, not whether it is shown – it could also be overlaid on the content (system setting on macOS).)

The reduced scroll percentage output breaks detection of whether the grid is scrolled all the way to the bottom in the ui-grid-auto-scroll plugin.

By the way, I am a bit confused about the meaning of uiGridConstants.scrollbars.ALWAYS. From the name and from its documentation I would expect that it causes the scrollbar to be present all the time, whether or not the content requires scrolling (like overflow: scroll). What it actually does however is to make the scrollbar appear and disappear based on whether it is needed (overflow: auto), which is actually the more useful behavior. Is this the intention?

@tarzasai
Copy link

I believe this is a duplicate of #6292: GridRenderContainer.needsHScrollbarPlaceholder() returns true regardless of the horizontal scrollbar being visible or not.

@cwalther
Copy link
Author

Definitely related, but not a duplicate (in the sense that it would automatically be fixed if the other were fixed) IMO – there is no needsHScrollbarPlaceholder() involved here. More like a symptom of the same underlying issue – I’m starting to believe that scrollbars.ALWAYS isn’t actually meant to behave as it does but as it says, and changing from overflow: scroll to overflow:auto in 21819c5 (which seems like a curious mixture of unrelated changes) wasn’t fully thought through.

mportuga pushed a commit that referenced this issue Jan 16, 2018
After reading over the comments by @cwalther on issue #6474, I decided to revert some of the changes

done in commit 21819c5.

fix #6292, fix #6474, fix #6484
@mportuga
Copy link
Member

@cwalther was right and the change of making overflow: auto was certainly a mistake. Reverting that fixes 3 issues.

mportuga pushed a commit that referenced this issue Jan 16, 2018
After reading over the comments by @cwalther on issue #6474, I decided to revert some of the changes

done in commit 21819c5.

fix #6292, fix #6474, fix #6484
@jeffantosh
Copy link

After upgrading to 4.2.2, this issue is still not fixed.

@andrewkittredge
Copy link
Contributor

I concur with @jeffantosh. I upgraded but still have the problem.

@httpete
Copy link

httpete commented Mar 7, 2018

still have the problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants