Fix for Repetitive Resizing issue when scrollbar appears between two breakpoints #1891 #2141

ShettyAkarsh · 2019-01-03T13:43:28Z

Summary

Resolves the Repetitive resizing issue when scrollbar appears between two breakpoints while the browser window is minimised and user tries to change the browser window width.

The fix is to pass the width of the parent container along with its scrollbar width to the handle resize method.

StephenEsser · 2019-01-04T18:25:08Z

packages/terra-responsive-element/src/ResponsiveElement.jsx

@@ -61,7 +61,7 @@ class ResponsiveElement extends React.Component {
      this.resizeObserver = new ResizeObserver((entries) => {
        this.animationFrameID = window.requestAnimationFrame(() => {
          this.animationFrameID = null;
-          this.handleResize(entries[0].contentRect.width);
+          this.handleResize(entries[0].target.getBoundingClientRect().width);


I was able to reproduce the infinite resizing with this new change using the code linked on the original issue:

Code

import React from 'react'; import List from 'terra-list'; import ResponsiveElement from 'terra-responsive-element/lib/ResponsiveElement'; import ItemView from 'terra-clinical-item-view'; import ItemDisplay from 'terra-clinical-item-display'; const ResponsiveTableExample = () => ( <div className="kaiju-Untitled"> <ResponsiveElement responsiveTo="parent" defaultElement={( <List isDivided> <List.Item isSelectable hasChevron content={<ItemView displays={[<ItemDisplay text="Item 1" />]} />} /> <List.Item isSelectable hasChevron content={<ItemView displays={[<ItemDisplay text="Item 2" />]} />} /> <List.Item isSelectable hasChevron content={<ItemView displays={[<ItemDisplay text="Item 3" />]} />} /> </List> )} tiny={( <List isDivided> <List.Item isSelectable hasChevron content={<ItemView displays={[<ItemDisplay text="Item 1" />]} />} /> <List.Item isSelectable hasChevron content={<ItemView displays={[<ItemDisplay text="Item 1" />]} />} /> <List.Item isSelectable hasChevron content={<ItemView displays={[<ItemDisplay text="Item 1" />]} />} /> <List.Item isSelectable hasChevron content={<ItemView displays={[<ItemDisplay text="Item 1" />]} />} /> <List.Item isSelectable hasChevron content={<ItemView displays={[<ItemDisplay text="Item 1" />]} />} /> <List.Item isSelectable hasChevron content={<ItemView displays={[<ItemDisplay text="Item 1" />]} />} /> <List.Item isSelectable hasChevron content={<ItemView displays={[<ItemDisplay text="Item 1" />]} />} /> <List.Item isSelectable hasChevron content={<ItemView displays={[<ItemDisplay text="Item 1" />]} />} /> <List.Item isSelectable hasChevron content={<ItemView displays={[<ItemDisplay text="Item 1" />]} />} /> <List.Item isSelectable hasChevron content={<ItemView displays={[<ItemDisplay text="Item 1" />]} />} /> <List.Item isSelectable hasChevron content={<ItemView displays={[<ItemDisplay text="Item 1" />]} />} /> <List.Item isSelectable hasChevron content={<ItemView displays={[<ItemDisplay text="Item 1" />]} />} /> <List.Item isSelectable hasChevron content={<ItemView displays={[<ItemDisplay text="Item 1" />]} />} /> <List.Item isSelectable hasChevron content={<ItemView displays={[<ItemDisplay text="Item 1" />]} />} /> <List.Item isSelectable hasChevron content={<ItemView displays={[<ItemDisplay text="Item 1" />]} />} /> <List.Item isSelectable hasChevron content={<ItemView displays={[<ItemDisplay text="Item 1" />]} />} /> <List.Item isSelectable hasChevron content={<ItemView displays={[<ItemDisplay text="Item 1" />]} />} /> <List.Item isSelectable hasChevron content={<ItemView displays={[<ItemDisplay text="Item 1" />]} />} /> <List.Item isSelectable hasChevron content={<ItemView displays={[<ItemDisplay text="Item 1" />]} />} /> <List.Item isSelectable hasChevron content={<ItemView displays={[<ItemDisplay text="Item 1" />]} />} /> <List.Item isSelectable hasChevron content={<ItemView displays={[<ItemDisplay text="Item 1" />]} />} /> <List.Item isSelectable hasChevron content={<ItemView displays={[<ItemDisplay text="Item 1" />]} />} /> <List.Item isSelectable hasChevron content={<ItemView displays={[<ItemDisplay text="Item 1" />]} />} /> <List.Item isSelectable hasChevron content={<ItemView displays={[<ItemDisplay text="Item 1" />]} />} /> <List.Item isSelectable hasChevron content={<ItemView displays={[<ItemDisplay text="Item 1" />]} />} /> <List.Item isSelectable hasChevron content={<ItemView displays={[<ItemDisplay text="Item 1" />]} />} /> <List.Item isSelectable hasChevron content={<ItemView displays={[<ItemDisplay text="Item 1" />]} />} /> <List.Item isSelectable hasChevron content={<ItemView displays={[<ItemDisplay text="Item 1" />]} />} /> <List.Item isSelectable hasChevron content={<ItemView displays={[<ItemDisplay text="Item 1" />]} />} /> <List.Item isSelectable hasChevron content={<ItemView displays={[<ItemDisplay text="Item 1" />]} />} /> <List.Item isSelectable hasChevron content={<ItemView displays={[<ItemDisplay text="Item 1" />]} />} /> <List.Item isSelectable hasChevron content={<ItemView displays={[<ItemDisplay text="Item 1" />]} />} /> <List.Item isSelectable hasChevron content={<ItemView displays={[<ItemDisplay text="Item 1" />]} />} /> <List.Item isSelectable hasChevron content={<ItemView displays={[<ItemDisplay text="Item 1" />]} />} /> <List.Item isSelectable hasChevron content={<ItemView displays={[<ItemDisplay text="Item 1" />]} />} /> <List.Item isSelectable hasChevron content={<ItemView displays={[<ItemDisplay text="Item 1" />]} />} /> <List.Item isSelectable hasChevron content={<ItemView displays={[<ItemDisplay text="Item 1" />]} />} /> <List.Item isSelectable hasChevron content={<ItemView displays={[<ItemDisplay text="Item 1" />]} />} /> <List.Item isSelectable hasChevron content={<ItemView displays={[<ItemDisplay text="Item 1" />]} />} /> <List.Item isSelectable hasChevron content={<ItemView displays={[<ItemDisplay text="Item 2" />]} />} /> <List.Item isSelectable hasChevron content={<ItemView displays={[<ItemDisplay text="Item 3" />]} />} /> </List> )} /> </div> ); export default ResponsiveTableExample;

Using Tyler's suggestion does seem to fix the issue. I would double check with @tbiethman to make sure this is what he had in mind with his suggestion.

Suggested change

this.handleResize(entries[0].target.getBoundingClientRect().width);

this.handleResize(entries[0].target.parentNode.offsetWidth);

I would recommend testing these changes with the code sample above.

I'm not sure if getting the parentNode of the target is what we want. The target should already be the container for the responsive elements, and that container is what is what is (conditionally) rendering the scrollbar. If we go a level higher, we're in uncharted waters.

I wouldn't have expected the width from getBoundingClientRect to differ from the offsetWidth of the same element.

@StephenEsser In above code example there is outer div for responsive element hence the target value is set as div and the target.parentnode is set as parentcontainer. so we are getting the issue of repetitive resizing using my initial code. if we add one more outerdiv to the current div the issue still exists with newly suggested code as the target.parentnode will be the outermost div and target will be innerdiv.

Observerresize.target will always hold the immediate parent control of responsiveelement. if the div elements are removed entries[0].target.getBoundingClientRect().width works fine as the target is always set to immediate parent of the responsive element. without any outer div elements the target.parentnode value will be set to parent container of responsive element parent container.

i even tried creating sample react app consuming the responsive element without any blank outer div elements. in this case the target value is set to root div element where as target.parentnode is html body.

so my question here is will there be a real time scenario where blank div be there around responsive element ?

@bjankord , @StephenEsser request you guys to review this PR. please let me know if i need to test anything else.

Is there a scenario where the code change above prevents an infinite resize?

@tbiethman @bjankord and I spoke offline and there is not currently a known solution that fixes the infinite resizing in all cases and it's possible there isn't one. The infinite resizing is caused by the available space fluctuating by a set amount of pixels that pushes each resize into the other breakpoint threshold.

Unless the solution above fixes any of the infinite resizing use-cases I would vote to close this PR and replace it with a PR that documents some of the caveats and recommended use cases of the ResponsiveElement. Specifically the caveat that if any ancestor's overflow can be affected by the contents within the ResponsiveElement it can result in an infinite resize.

@tbiethman @bjankord Do you guys have any thoughts on this?

@StephenEsser Regarding your question "Is there a scenario where the code change above prevents an infinite resize?"

Yes, the above code change prevents the infinite resize during the scrollbar display while shifting to next immediate breakpoint of responsive element. the scrollbar is due to parent control which has the overflow property set to 'Auto'.

the previous code 'entries[0].contentRect.width' used to not take scrollbar width into account which would make the breakpoint of responsive element shift to one level below. while it should actually not shift the breakpoints. hence using parent control width which includes scrollbar width also.

I believe the width returned from entries[0].target.getBoundingClientRect().width also includes the padding and borders. The ResponsiveElement is currently implemented to shift between breakpoints based on the available space for the content. There are a few notes about this on the ReadMe:

The ResponsiveElement observes the available width. Meaning padding, borders, and margins set on the parent are not included in the calculation. Modifying box-sizing will impact how these properties are used in an elements width calculation. For more details see: https://css-tricks.com/almanac/properties/b/box-sizing/

I think the ResponsiveElement should continue to honor the available space for calculations and not the raw width of the parent including paddings / borders.

Here's a short example.

<div id="parent" style="padding: 20px; border: 5px solid black; width: 100px;"> <div>ResponsiveElement</div> </div>

In this example the bounding rect width would report 150px of space, but there is actually only 100px of available space for the ResponsiveElement.

Had a chance to look over this PR and the related issue in more detail. @StephenEsser created a branch with a reduced test case which was helpful to reason about the issue. I'm okay with updating the responsive element behavior to report the raw width of the parent including paddings / borders instead of the available space as it was prior, but I would consider this to be a non-passive change.

I'd like to see some documentation updates noting the behavior change as the docs currently mention reporting the available space.

e.g. Before this PR, if a div has width: 120px and padding: 10px, it would report 100px of available space. With the change in this PR, the responsive element would now report 120px of available space even though there is 10px padding on the sides. I think this concern can be mitigated with some documentation.

In regards to this being a non-passive change, I'd like to hold off on merging this PR until we are ready to incorporate the other non-passive changes we have planned in this issue. That work will likely happen sometime in Q1.

Updated the documentation.

bjankord

Let's test with the provided code sample as @StephenEsser mentioned

bjankord · 2019-01-11T17:28:19Z

Talked some more with @StephenEsser and @tbiethman on this today. @StephenEsser is working on creating some reduced test cases so we can talk through the issues in more detail.

bjankord · 2019-02-12T19:18:37Z

In addition to this PR, we'll also want to complete the work for #2048 in a separate PR. The goal will be to release the breaking changes in this PR and the breaking changes for #2048 in 1 major release version.

packages/terra-responsive-element/docs/README.md

emilyrohrbough · 2019-05-01T14:51:00Z

@ShettyAkarsh We need to update the upgrade guide for these breaking changes for this MVB

ShettyAkarsh · 2019-05-09T07:10:57Z

@ShettyAkarsh We need to update the upgrade guide for these breaking changes for this MVB

Changes updated for upgrade guide.

packages/terra-responsive-element/CHANGELOG.md

ryanthemanuel · 2019-06-14T19:43:59Z

packages/terra-responsive-element/CHANGELOG.md

@@ -24,6 +24,9 @@ Unreleased
 ### Changed
 * Made required updates to consume terra-toolkit v5 and terra-dev-site v5

+### Fixed
+* Repititive Resizing when scrollbars appear between two breakpoints


We'll also want to move this changelog entry into the unreleased section

bjankord · 2019-06-17T13:54:21Z

package.json

@@ -79,6 +79,9 @@
    "enzyme-adapter-react-16": "^1.1.1",
    "enzyme-to-json": "^3.2.2",
    "eslint": "^5.0.0",
+    "eslint-config-terra": "^2.2.0",
+    "gh-pages": "^1.1.0",


This line can be removed, we have gh-pages defined on line 86.

bjankord · 2019-06-17T13:54:46Z

package.json

@@ -79,6 +79,9 @@
    "enzyme-adapter-react-16": "^1.1.1",
    "enzyme-to-json": "^3.2.2",
    "eslint": "^5.0.0",
+    "eslint-config-terra": "^2.2.0",
+    "gh-pages": "^1.1.0",
+    "glob": "^7.1.1",


This line can be removed, we have glob defined on line 87

bjankord · 2019-06-17T13:55:20Z

package.json

@@ -79,6 +79,9 @@
    "enzyme-adapter-react-16": "^1.1.1",
    "enzyme-to-json": "^3.2.2",
    "eslint": "^5.0.0",
+    "eslint-config-terra": "^2.2.0",


We can keep this line and remove the eslint-config-terra defined on line 85

bjankord

+1 from me, I'd like to have @StephenEsser take a longer over the PR one more time before we merge this down.

StephenEsser · 2019-06-17T15:07:21Z

packages/terra-responsive-element/docs/README.md

@@ -6,6 +6,8 @@ The Responsive Element can be set to be responsive to the parent of the componen

 ## Additional Notes

+The ResponsiveElement observes the total width. Meaning padding, borders except the margins set on the parent are included in the calculation. Modifying box-sizing will impact how these properties are used in an elements width calculation. For more details see: https://css-tricks.com/almanac/properties/b/box-sizing/
+
 For consistency, breakpoint ranges are inherited from [terra-breakpoints](https://engineering.cerner.com/terra-ui/#/components/terra-breakpoints/breakpoints/about).

 The Responsive Element observes the available width. Meaning padding, borders, and margins set on the parent are not included in the calculation. Modifying box-sizing will impact how these properties are used in an elements width calculation. For more details see: <https://css-tricks.com/almanac/properties/b/box-sizing/>


We'll need to delete this section, it's the opposite of the newly added section

StephenEsser · 2019-06-17T15:13:47Z

packages/terra-responsive-element/docs/README.md

@@ -6,6 +6,8 @@ The Responsive Element can be set to be responsive to the parent of the componen

 ## Additional Notes

+The ResponsiveElement observes the total width. Meaning padding, borders except the margins set on the parent are included in the calculation. Modifying box-sizing will impact how these properties are used in an elements width calculation. For more details see: https://css-tricks.com/almanac/properties/b/box-sizing/


We might clean this sentence up, it is a little confusing as it is. Maybe something along the lines of:

The ResponsiveElement observes the total width of the bound container. Padding and borders are included in the calculation to determine the available width. Margins are not included. For breakpoint accuracy it is not recommended to set padding on the parent container when responsiveTo is set to parent.

bjankord · 2019-06-18T15:42:03Z

Merging down as is to get theme repos unblocked. @StephenEsser will work on the code review comments he has in a new PR.

* master: (48 commits) Update search-field-spec.js to increase the tolerance for the default search field (#2468) [terra-responsive-element] - Update examples to use React Hooks (#2469) Move Wiki Upgrade Guide to Deployed Upgrade Guide (#2467) Update UpgradeGuide.3.doc.jsx Update terra-mixin import file path (#2464) Resolve WCAG 2.1 AA issues (#2456) Add upgrade guide for terra-base (#2459) Publish Update changelogs [terra-icon] Updated Devdependencies (#2451) [terra-responsive-element] - Update documentation (#2463) Fix for Repetitive Resizing issue when scrollbar appears between two breakpoints #1891 (#2141) Fixed pageSet logic for total pages less than max allowed (#2458) Update UPGRADEGUIDE.md Additional null check for to handle empty query selectors in Form-Select. (#2455) [terra-core] Removed inline style for dev-site components Part 1 (#2427) [terra-responsive-element] - Update breakpoint ranges (#2452) update wdio tests to capture all interactions (#2443) Enable form-field required prop to pass down to children form fields (#2398) Cleanup WDIO Snapshots (#2454) ... # Conflicts: # packages/terra-form-select/src/_Frame.jsx # packages/terra-form-select/tests/wdio/_frame-spec.js # packages/terra-form-select/tests/wdio/select-field-spec.js # packages/terra-form-select/tests/wdio/select-spec.js

Fixes Repititive Resizing issue cerner#1891

d37c497

ShettyAkarsh added 📦 terra-responsive-element tech-only labels Jan 3, 2019

ShettyAkarsh requested review from bjankord, tbiethman, satyajeetdesale, ryanthemanuel and dkasper-was-taken January 3, 2019 13:43

ShettyAkarsh requested review from JakeLaCombe and mjhenkes as code owners January 3, 2019 13:43

dkasper-was-taken approved these changes Jan 3, 2019

View reviewed changes

bjankord approved these changes Jan 3, 2019

View reviewed changes

bjankord temporarily deployed to terra-core-deployed-pr-2141 January 3, 2019 15:45 Inactive

tbiethman approved these changes Jan 3, 2019

View reviewed changes

StephenEsser reviewed Jan 4, 2019

View reviewed changes

bjankord suggested changes Jan 4, 2019

View reviewed changes

bjankord added the Major Version Bump label Jan 18, 2019

emilyrohrbough assigned ShettyAkarsh Jan 25, 2019

Added doc chenges

af5b6e5

bjankord had a problem deploying to terra-core-deployed-pr-2141 February 4, 2019 07:00 Failure

Merge branch 'master' into ResponsiveElementResizing

7194fa7

bjankord temporarily deployed to terra-core-deployed-pr-2141 February 4, 2019 17:31 Inactive

bjankord reviewed Feb 12, 2019

View reviewed changes

packages/terra-responsive-element/docs/README.md Show resolved Hide resolved

bjankord temporarily deployed to terra-core-deployed-pr-2141 May 9, 2019 07:08 Inactive

Changes to upgrade.md file

7fc6bcc

emilyrohrbough reviewed May 9, 2019

View reviewed changes

packages/terra-responsive-element/CHANGELOG.md Outdated Show resolved Hide resolved

Merge branch 'master' into ResponsiveElementResizing

484d430

bjankord temporarily deployed to terra-core-deployed-pr-2141 May 10, 2019 04:48 Inactive

Merge branch 'master' into ResponsiveElementResizing

b578e29

bjankord temporarily deployed to terra-core-deployed-pr-2141 June 11, 2019 16:16 Inactive

Merge branch 'master' into ResponsiveElementResizing

f8a33fd

bjankord temporarily deployed to terra-core-deployed-pr-2141 June 14, 2019 19:35 Inactive

ryanthemanuel reviewed Jun 14, 2019

View reviewed changes

bjankord reviewed Jun 17, 2019

View reviewed changes

bjankord approved these changes Jun 17, 2019

View reviewed changes

StephenEsser reviewed Jun 17, 2019

View reviewed changes

Merge branch 'master' into ResponsiveElementResizing

7a016cc

bjankord deployed to terra-core-deployed-pr-2141 June 18, 2019 14:51 View deployment

Merge branch 'master' into ResponsiveElementResizing

5eab881

bjankord requested a deployment to terra-core-deployed-pr-2141 June 18, 2019 15:41 Abandoned

bjankord merged commit f981334 into cerner:master Jun 18, 2019

StephenEsser mentioned this pull request Jun 18, 2019

[terra-responsive-element] - Update documentation #2463

Merged

bjankord mentioned this pull request Jun 18, 2019

[terra-responsive-element] Issue with repetitive resizing #1891

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for Repetitive Resizing issue when scrollbar appears between two breakpoints #1891 #2141

Fix for Repetitive Resizing issue when scrollbar appears between two breakpoints #1891 #2141

ShettyAkarsh commented Jan 3, 2019

StephenEsser Jan 4, 2019 •

edited

Loading

tbiethman Jan 4, 2019

ShettyAkarsh Jan 7, 2019 •

edited

Loading

ShettyAkarsh Jan 9, 2019

StephenEsser Jan 9, 2019 •

edited

Loading

ShettyAkarsh Jan 10, 2019

StephenEsser Jan 10, 2019

bjankord Jan 18, 2019

ShettyAkarsh Feb 4, 2019

bjankord left a comment

bjankord commented Jan 11, 2019

bjankord commented Feb 12, 2019

emilyrohrbough commented May 1, 2019 •

edited

Loading

ShettyAkarsh commented May 9, 2019

ryanthemanuel Jun 14, 2019

StephenEsser Jun 17, 2019

bjankord Jun 17, 2019

bjankord Jun 17, 2019

bjankord Jun 17, 2019

bjankord left a comment

StephenEsser Jun 17, 2019

StephenEsser Jun 17, 2019

bjankord commented Jun 18, 2019

	this.handleResize(entries[0].target.getBoundingClientRect().width);
	this.handleResize(entries[0].target.parentNode.offsetWidth);

		@@ -6,6 +6,8 @@ The Responsive Element can be set to be responsive to the parent of the componen

		## Additional Notes

		The ResponsiveElement observes the total width. Meaning padding, borders except the margins set on the parent are included in the calculation. Modifying box-sizing will impact how these properties are used in an elements width calculation. For more details see: https://css-tricks.com/almanac/properties/b/box-sizing/

Fix for Repetitive Resizing issue when scrollbar appears between two breakpoints #1891 #2141

Fix for Repetitive Resizing issue when scrollbar appears between two breakpoints #1891 #2141

Conversation

ShettyAkarsh commented Jan 3, 2019

Summary

StephenEsser Jan 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShettyAkarsh Jan 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StephenEsser Jan 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjankord left a comment

Choose a reason for hiding this comment

bjankord commented Jan 11, 2019

bjankord commented Feb 12, 2019

emilyrohrbough commented May 1, 2019 • edited Loading

ShettyAkarsh commented May 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjankord left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjankord commented Jun 18, 2019

StephenEsser Jan 4, 2019 •

edited

Loading

ShettyAkarsh Jan 7, 2019 •

edited

Loading

StephenEsser Jan 9, 2019 •

edited

Loading

emilyrohrbough commented May 1, 2019 •

edited

Loading