Skip to content

Ddlbrena/popup scroll bar fix #4528

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.

Sign up for GitHub

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

Merged
jspurlin merged 4 commits into from
Apr 13, 2018
Merged

Ddlbrena/popup scroll bar fix #4528

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ea2fa68
Fix scroll bar shows when it should not.
ddlbrena Apr 12, 2018
d08d1cb
rush change
ddlbrena Apr 12, 2018
a0fae40
fix using wrong element to calculate root height
ddlbrena Apr 12, 2018
5a447e1
Update the code to not use BoundingClientRect since it is too expensi…
ddlbrena Apr 12, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions common/changes/office-ui-fabric-react/ddlbrena-popupScrollBarFix_2018-04-12-05-44.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{
"changes": [
{
"packageName": "office-ui-fabric-react",
"comment": "Fix scroll bar shows when it should not.",
"type": "patch"
}
],
"packageName": "office-ui-fabric-react",
"email": "ddlbrena@microsoft.com"
}
5 changes: 3 additions & 2 deletions packages/office-ui-fabric-react/src/components/Popup/Popup.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,8 +115,9 @@ export class Popup extends BaseComponent<IPopupProps, IPopupState> {
private _getScrollBar() {
let needsVerticalScrollBar = false;
if (this._root && this._root.value && this._root.value.firstElementChild) {
needsVerticalScrollBar = this._root.value.clientHeight > 0
&& 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;

@jspurlin jspurlin Apr 12, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ddlbrena ddlbrena Apr 12, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
if (this.state.needsVerticalScrollBar !== needsVerticalScrollBar) {
this.setState({
Expand Down