Conversation
|
|
| && this._root.value.firstElementChild.clientHeight > this._root.value.clientHeight; | ||
| const rootHeight = this._root.value.getBoundingClientRect().height; | ||
| needsVerticalScrollBar = rootHeight > 0 | ||
| && this._root.value.firstElementChild.getBoundingClientRect().height > rootHeight; |
There was a problem hiding this comment.
Yeah, I'm concerned that this is overkill and this might result in too much of a perf hit if it's run too much... but if it doesn't get run too often it should be much of an issue. Do you try with a buffer and was it reliable for making sure the scrollbar did not appear (vs calling getBoundingClientRect)? I created a test on jsperf to compare clientHight vs getBoundClientRect.height https://jsperf.com/clienthightvsgetboundingclientrect so we can get an idea for what the perf implications are. I'd like to get @dzearing 's thoughts on this
There was a problem hiding this comment.
Thank you for the link, that is really neat. I looked more into this and scrollbar recalculation can happen quite often in the case of comboboxes when we are hovering over items and updating selection. From jsperf it does look like BoundingClientRect is 90% slower than ClientHeight. Updating the code to not use that and instead allow for a 1px diff between the comparison seems to do the trick and I dont think it could cause other problems. It does feel a bit too hacky to me, but I guess better than the perf hit. I will update the code in the next iteration.
jspurlin
left a comment
jspurlin
left a comment
There was a problem hiding this comment.
getBoundingClientRect may be overkill here, making the check a little more loose (+- 1px, for example) may be sufficient. See: https://jsperf.com/clienthightvsgetboundingclientrect to play with the perf differences between the two ways of getting the height
|
@manishgarg1 or @joschect care to weigh in here? The change is simple but want to get any options from other fabric engineers before going one way vs the other |
…ve and instead allow for a 1px difference in the height difference calculation.
jspurlin
left a comment
jspurlin
left a comment
There was a problem hiding this comment.
The description should be updated to reflect the current state of the PR
| const rootHeight = this._root.value.clientHeight; | ||
| const firstChildHeight = this._root.value.firstElementChild.clientHeight; | ||
| if (rootHeight > 0 && firstChildHeight > rootHeight) { | ||
| needsVerticalScrollBar = (firstChildHeight - rootHeight) > 1; |
There was a problem hiding this comment.
what if rootHeight is <= 0?
There was a problem hiding this comment.
It looks like you could get into a situation where the logic is different now than it used to be
There was a problem hiding this comment.
so I am leaving that as it was, which it just does not show the scrollbar, not sure when that happens
In reply to: 181263841 [](ancestors = 181263841)
There was a problem hiding this comment.
... I missed the if statement on line 129 when I made that comment...
3 participants
On some browsers (I am able to repro this issue with Chrome but so far not with IE or FireFox) at a zoom level greater than 100% popups shown from controls like split buttons show a scrollbar when they should not. The problem is that the logic to determine if a scrollbar must be shown or not compares the ClientHeight of the root container with the ClientHeight of its first element child. The problem is that ClientHeight is a rounded value to an integer. Sometimes the rounding generates different results for the root container and child even though they are the same height. This causes the scrollbar to show for no reason. The fix is to use BoundingClientRect().height for the calculation instead since that returns the height without the rounding. There were some concerns around the perf of calling BoundingClientRect, I verified that this does not get called multiple times in several basic scenario so I think it should be ok. A different approach could be to keep using ClientHeight but compare the values with a plus\minus difference of one pixel.