Add custom element option to WindowScroller #481

andrewbranch · 2016-11-22T19:17:31Z

In reference to #474

Adds an optional prop scrollElement to WindowScroller which, if provided, attaches scroll handlers to the given element instead of to window.

@bvaughn I “tested” this via adding an option to the demo (although maybe you’d like to change the way I did it—added a couple things to Application’s context—I’m not familiar with HashRouter, but maybe it can be done through props instead?) and it seems to work nicely. I’m happy to help add tests and docs, but wanted to let you take a look at it as soon as possible, and also get your feedback on the best way to add some tests for this. It almost seems like a big group of the tests could just be run twice, once for window and once for some custom element.

There are a couple things I wasn’t sure about, so I’ll start adding a few inline comments to ask about specific things.

…tainers

andrewbranch · 2016-11-22T19:21:48Z

source/WindowScroller/WindowScroller.css

@@ -0,0 +1,3 @@
+.disabled > * {
+  pointer-events: none !important;


I’m using this instead of inline styles because a custom scroll container may have many children that need pointer-events: none, whereas scrolling the window only needs to apply that style to body. So, I wasn’t sure whether to add !important or not. It ought to work pretty similarly to the way it used to (replacing inline styles), and it is still overrideable (higher-specificity selectors with !important, or inline styles with !important). On the other hand, I hate it when libraries use !important.

Do you actually mean .disabled *?

Hm... either way, > * is not a very performant selector so I'm pretty reluctant to use it, and definitely not with such a generic class name as disabled. RV classes are named so as to (hopefully) not conflict with any other global CSS names.

Also related- RV has also been shifting away from any CSS-based styles (with the exception of Table). Basically anything that's functional is defined via an inline style; anything that's presentational (only Table) is defined via CSS to make it easier to override in a variety of ways (without requiring use of !important).

If I understand correctly, the intention is to disable pointer events on the contents of whatever element is scrolling, without disabling them on the scrolling element itself. So if the contents can now be N number of nodes, either we use CSS to select all children, or we iterate over all N nodes to add/remove inline styles.

To the initial question, .disabled > * and .disabled * should have the same effect, and I didn't expect the latter to be more efficient, but that’s fine.

And as far as the name goes, we can definitely change it—since the demo was using CSS Modules, and I'm using CSS Modules in my own stuff, I totally didn't think about it. Another option would be to dynamically generate/insert a stylesheet, but that would be a lot more magic; harder for developers to track down if they have the need to customize behavior.

To the initial question, .disabled > * and .disabled * should have the same effect, and I didn't expect the latter to be more efficient, but that’s fine.

No. The first disables pointer events only for immediate children. The latter disables for all descendants. I wasn't sure which you were going for. (The second is more expensive, so I wasn't recommending it- just asking for clarification.) First would only work if the Grid (or List or whatever) was an immediate child of the new scrollElement.

And as far as the name goes, we can definitely change it—since the demo was using CSS Modules, and I'm using CSS Modules in my own stuff, I totally didn't think about it.

Yup, the demo uses CSS modules (love 'em) but the lib itself doesn't in order to be as flexible and compatible with the various approaches to styling as possible.

Another option would be to dynamically generate/insert a stylesheet, but that would be a lot more magic; harder for developers to track down if they have the need to customize behavior.

Agreed. I don't think this is a good path to go down.

Hmm...it's been a long day and I'm tired, but...I wonder if it would be more appropriate to approach this by passing a prop (eg disablePointerEvents) to Grid telling it to disable pointer events. That would be more performant than this CSS approach. Basically this line would change to something like:

pointerEvents: this.state.isScrolling || this.props.disablePointerEvents ? 'none' : '',

The first disables pointer events only for immediate children. The latter disables for all descendants.

Well, yeah, but pointer-events is inherited, which I guess is what I was counting on there—analogous to WindowScroller in master counting on elements to inherit it from document.body.

I wonder if it would be more appropriate to approach this by passing a prop (eg disablePointerEvents) to Grid telling it to disable pointer events.

That works, but it is different than the existing behavior. WindowScroller in current master disables pointer-events on document.body, e.g. all the stuff inside the scroll container. My CSS (while obviously not being feasible) accomplishes exactly that for arbitrary scroll containers. What you’re suggesting would narrow the scope of what gets pointer-events: none down to just the grid/list, not the whole scroll container. That seems like a reasonable trade-off to me, but I don’t have the same prior knowledge as you about how pointer-events effects performance of this whole system. If you’re ok with that trade-off, I can move forward with implementing it.

pointer-evetns is an Inherited style, yes, but it can be overridden by children. And Grid would override, since it sets the property on the ReactVirtualized__Grid__innerScrollContainer, which is not the direct child of WindowScroller but- at best- a child of its child.

Also using !important is kind of a last resort, so I like to avoid it.

andrewbranch · 2016-11-22T19:25:07Z

source/WindowScroller/WindowScroller.example.js

  static contextTypes = {
-    list: PropTypes.instanceOf(Immutable.List).isRequired
+    list: PropTypes.instanceOf(Immutable.List).isRequired,
+    customElement: PropTypes.object,


In an earlier commit, I had PropTypes.instanceOf(Element), but the linter complained about that, and I realized that it’s probably for good reason: Element won’t be defined on servers, and this is a static property. So, not sure if there’s something better than object here? I guess we could make a custom validator function which, when invoked, looks at window.Element but gracefully does nothing on server-side render?

any type is probably okay here. It's not great, but even the official Flow types use any for ref type.

andrewbranch · 2016-11-22T19:29:39Z

source/WindowScroller/utils/dimensions.js

+ * Handles edge-case where a user is navigating back (history) from an already-scrolled page.
+ * In this case the body’s top position will be a negative number and this element’s top will be increased (by that amount).
+ */
+export function getPositionFromTop (element, container) {


I was having trouble wrapping my mind around this (previously here), so extra care in making sure this satisfies the original intention is appreciated.

Okay. When I do a real review, I'll read this one carefully.

andrewbranch · 2016-11-22T19:31:51Z

source/demo/ContentBox.css

@@ -1,5 +1,5 @@
 .ContentBox {
-  flex: 1;
+  flex: 1 0 auto;


This is obviously not directly related to the source changes, but something to this effect was required to make my addition to the demo work (a ContentBox was shrinking and scrolling, rather than .Body). I spot checked the other demo changes and didn't see anything obviously broken. Removing overflow: auto has the same effect, if that is preferable to you.

No strong feelings here. That's fine. 😄

bvaughn

Don't have time to do a full code review now but thought I'd share some quick thoughts.

By the way, thanks a ton for contributing. Very exciting. 😁

bvaughn · 2016-11-22T19:46:56Z

source/WindowScroller/WindowScroller.css

@@ -0,0 +1,3 @@
+.disabled > * {
+  pointer-events: none !important;


Do you actually mean .disabled *?

Hm... either way, > * is not a very performant selector so I'm pretty reluctant to use it, and definitely not with such a generic class name as disabled. RV classes are named so as to (hopefully) not conflict with any other global CSS names.

Also related- RV has also been shifting away from any CSS-based styles (with the exception of Table). Basically anything that's functional is defined via an inline style; anything that's presentational (only Table) is defined via CSS to make it easier to override in a variety of ways (without requiring use of !important).

bvaughn · 2016-11-22T19:48:53Z

source/WindowScroller/WindowScroller.example.js

  static contextTypes = {
-    list: PropTypes.instanceOf(Immutable.List).isRequired
+    list: PropTypes.instanceOf(Immutable.List).isRequired,
+    customElement: PropTypes.object,


any type is probably okay here. It's not great, but even the official Flow types use any for ref type.

bvaughn · 2016-11-22T19:50:23Z

source/WindowScroller/utils/dimensions.js

+ * Handles edge-case where a user is navigating back (history) from an already-scrolled page.
+ * In this case the body’s top position will be a negative number and this element’s top will be increased (by that amount).
+ */
+export function getPositionFromTop (element, container) {


Okay. When I do a real review, I'll read this one carefully.

bvaughn · 2016-11-22T19:51:01Z

source/demo/ContentBox.css

@@ -1,5 +1,5 @@
 .ContentBox {
-  flex: 1;
+  flex: 1 0 auto;


No strong feelings here. That's fine. 😄

andrewbranch · 2016-11-22T20:51:20Z

Tests were failing due to missing autoprefixer-loader, which is referenced in webpack.config.umd.js, but is deprecated, and you have the correct postcss-loader with autoprefixer config in other webpack config files. I'm guessing you intend to update the UMD config soon, but I went ahead and added the simplest possible fix so we can see test results.

andrewbranch · 2016-11-22T21:54:50Z

Back to the CSS issue, I see we definitely have to do something different. I totally wasn't thinking and did import styles from '../WindowScroller.css' in that util, which doesn’t work in the built output, because the CSS file isn't copied, and not everyone is using Webpack / something that can import CSS. So, not sure what the best way forward is.

bvaughn · 2016-11-23T00:41:15Z

source/WindowScroller/utils/onScroll.js

@@ -1,5 +1,5 @@
+import styles from '../WindowScroller.css'


Not using CSS modules in RV proper

bvaughn · 2016-11-23T00:44:47Z

Tests were failing due to missing autoprefixer-loader, which is referenced in webpack.config.umd.js, but is deprecated, and you have the correct postcss-loader with autoprefixer config in other webpack config files. I'm guessing you intend to update the UMD config soon, but I went ahead and added the simplest possible fix so we can see test results.

Back to the CSS issue, I see we definitely have to do something different. I totally wasn't thinking and did import styles from '../WindowScroller.css' in that util, which doesn’t work in the built output, because the CSS file isn't copied, and not everyone is using Webpack / something that can import CSS. So, not sure what the best way forward is.

I'm intentionally not using CSS modules within react-virtualized because I don't want to impose that requirement on lib users. Things started failing b'c you are importing a stylesheet to use it this way and the build isn't setup to. (Static style names are used.)

* When the scroll position is updated internally (i.e. the inner container is scrolled), it previously updated the internal scroll state for both axes, which overwrote the scroll position that was passed in from props * If one axis was controlled externally (e.g. by WindowScroller), that axis's scroll position was being overwritten * This fix stops the internal state of an axis from being modified if an external scroll position is supplied for that axis * Additionally, when the external scroll position is modified (props are updated), the scroll direction is now updated, so that overscanning works for an axis that is controlled, e.g. by a WindowScroller

…it's supported by default

andrewbranch · 2016-11-30T22:22:19Z

@bvaughn don't want to be a bother, but any thoughts on my last comments?

bvaughn · 2016-12-02T04:58:04Z

Do you think other changes to the way pointer events works (e.g. passing down as a prop to Grid) is outside the scope of this PR, or does it need to be addressed here?

I don't think it needs to be addressed here.

Unfortunately I noticed a bug while testing this change locally. Steps to reproduce it:

Open the WindowScroller demo
Click the "Use custom element for scrolling" checkbox
Scroll down a bit
Open another demo (any one)
Return to the WindowScroller demo

At this point the list will be cut off in the middle, like:

Unfortunately it's been a long, rough day here and I don't have the mental energy to dig into this. I don't feel comfortable moving forward with the PR though without it being addressed.

andrewbranch · 2016-12-02T04:59:38Z

No worries, I'll look into it tomorrow! Definitely need to get that fixed. Thanks for reviewing!

bvaughn · 2016-12-02T05:00:12Z

❤️ thank you for understanding.

…t rendered

andrewbranch · 2016-12-02T18:42:14Z

@bvaughn fixed the bug you were seeing—good catch. When WindowScroller’s componentDidMount runs, the list has not yet been rendered, so the scroll container, at the time its height was being calculated and cached, had shrunk down to fit its contents. A moment later, the list finished rendering and the scroll container grew to take up all of its available space (the bottom of the window).

Above, I fixed the issue by making sure that scroll container always grows to fill the window, regardless of its contents. However, do you think this is a good case for using detectElementResize on the custom scroll containers?

…er handles window resize events.

…the behavior regarding controlled/uncontrolled and props and in a way that was not backwards compatible. (See issue #490 for more.)

bvaughn · 2016-12-03T16:52:33Z

Nice work on fixing that bug.

I resumed the code review and left some more thoughts. I also merged in master and there were a few conflicts which I've fixed.

To save you a little time, I've pushed the merged result here: https://github.com/bvaughn/react-virtualized/tree/andrewbranch-window-scroller-custom-element

bvaughn · 2016-12-03T16:32:46Z

source/WindowScroller/WindowScroller.js

+    } else if (!nextProps.scrollElement && this.scrollElement !== window) {
+      this._updateDimensions(window)
+      unregisterScrollListener(this, this.scrollElement)
+      registerScrollListener(this, window)


Am I reading this wrong, or could we reduce the complexity of this a bit?

if (nextProps.scrollElement !== this.scrollElement) { this._updateDimensions(nextProps.scrollElement || window) unregisterScrollListener(this, this.scrollElement) registerScrollListener(this, nextProps.scrollElement || window) }

bvaughn · 2016-12-03T16:35:48Z

source/WindowScroller/WindowScroller.js

@@ -92,7 +113,7 @@ export default class WindowScroller extends Component {
  _onResizeWindow (event) {
    const { onResize } = this.props

-    const height = window.innerHeight || 0
+    const height = getHeight(this.scrollElement)


We should probably call this._updateDimensions() here instead? That will handle cases where a resize affects top position (due to things like text wrapping) and it will also only call setState if the height has actually changed (eg not just a horizontal resize).

The onResize call should also be moved into this helper method for similar reasons (and to avoid duplication).

_onResizeWindow (event) { this._updateDimensions() }

bvaughn · 2016-12-03T16:38:46Z

source/WindowScroller/WindowScroller.example.js

  }

  render () {
-    const { list } = this.context
+    const { list, isScrollingCustomElement, customElement } = this.context


I know this is probably necessary to test the change within the demo site, but I'm not a fan of the complexity this adds to the WindowScroller.example or Application components. I wonder if it wouldn't be better to leave this particular functionality out of the demo and just cover it via unit tests instead?

Speaking of which 😁 this change set should be accompanied by a couple of new unit tests so others (including myself) don't break it in the future.

Totally. I was hoping to get your thoughts on what/how to test. Was wondering if some of the existing WindowScroller tests could be extracted into a function and be called once for each "mode" (scrolling window and scrolling scrollElement). Does that make sense or should each new test be written out distinctly? Do you want to duplicate some of the basic assertions to make sure WindowScroller as a whole doesn’t mess up with scrollElement, or should the new tests only address the specific added functionality?

One way I test things like this is by iterating over a set of options and creating my tests (the describe block) inside of a loop. eg

for (let i = 0; i < 2; i++) { describe('WindowScroller', () => { const useCustomElement = !!i; // Tests here ... }); }

Rather than extracting tests into helper functions. I'm not convinced that's the appropriate way to handle this case though. But new functionality like this absolutely needs new test coverage. 😄

bvaughn · 2016-12-03T16:50:34Z

source/WindowScroller/WindowScroller.js

+    if (height !== newHeight) {
+      this.setState({
+        height: newHeight
+      })


This is probably the place to call the onResize prop too.

codecov-io · 2016-12-15T21:51:33Z

Current coverage is 98.20% (diff: 94.54%)

Merging #481 into master will decrease coverage by 0.06%

@@             master       #481   diff @@
==========================================
  Files            61         62     +1   
  Lines          4338       4404    +66   
  Methods         916        928    +12   
  Messages          0          0          
  Branches        331        339     +8   
==========================================
+ Hits           4263       4325    +62   
- Misses           75         79     +4   
  Partials          0          0

Powered by Codecov. Last update 0182729...36a54f9

andrewbranch · 2016-12-15T21:58:17Z

source/WindowScroller/utils/dimensions.js

@@ -26,5 +26,5 @@ export function getHeight (element) {
 */
 export function getPositionFromTop (element, container) {
  const containerElement = container === window ? document.documentElement : container
-  return element.getBoundingClientRect().top - containerElement.getBoundingClientRect().top
+  return element.getBoundingClientRect().top + getVerticalScroll(container) - containerElement.getBoundingClientRect().top


This fixes a bug which can be reproduced in the demo, before this commit, by

Check the "use custom element for scrolling" box

Press Shift + Tab twice so that the "Hide header text" button is focused

Scroll down a ways

Press the space bar so as to press "Hide header text"

Scroll again

I also made sure that the back-button navigation edge case still works.

@bvaughn Can you check this out, because it ostensibly fixes bugs, but breaks some tests 😭

@bvaughn Any chance you could look at this sometime soonish? Too long in PRgatory makes me anxious 😅

Previously, given two mounted WindowScrollers with different scrollElements, when either scrollElement was scrolled, both WindowScrollers were updated with { isScrolling: true }.

bvaughn · 2017-01-11T19:07:06Z

I'm so sorry. Promise I'll try to make time to look this weekend. Please ping me again if you haven't heard back from my by Saturday.

bvaughn · 2017-01-11T19:07:16Z

PS Thanks for keeping on top of this!

andrewbranch · 2017-01-11T19:08:58Z

No worries! I didn't want to bother you until after the holidays. Maintaining popular open source things sounds hard 😁

bvaughn · 2017-01-11T19:17:47Z

That's very understanding of you 😄 Let's get this done this weekend. We'll talk more soon!

bvaughn · 2017-01-15T16:31:43Z

Looks like this branch has diverged from master a bit, unfortunately. There are changes showing up in the diffs that aren't related, and some things committed to master haven't made it into this branch either. My fault for taking so long to review it but it is what it is.

If I checkout this branch as-is and merge in master, 4 tests fail:

WindowScroller onScroll should trigger callback when window scrolls
WindowScroller onScroll should update :scrollTop when window is scrolled
WindowScroller updatePosition should recalculate the offset from the top when the window resizes
WindowScroller updatePosition should recalculate the offset from the top if called externally

These tests were added in 2e91ec7 which was made after your branch and did not merged in. I'll take a stab at reconciling them this morning because I know we'd both really like this PR to get merged. 😄

bvaughn · 2017-01-15T19:04:14Z

Merged in 5cc2ccb, after conflicts resolved.

Thanks!

bvaughn · 2017-01-15T19:39:34Z

Okay @andrewbranch. This was just released in 8.10.0. Thanks again for your work!

If you'd be willing, I'd still like to get some test coverage for this new feature maybe as a follow-up PR. If you can't get to it I'll find some time to add them.

Cheers!

andrewbranch · 2017-01-18T17:20:40Z

🎉 Thanks for all the help! I can probably add some tests in the near-ish future. Cheers!

bvaughn · 2017-01-18T17:27:20Z

That would be great. Thanks! :)

andrewbranch added 13 commits November 22, 2016 10:03

Add util for abstracting API differences between window and other con…

b7ac151

…tainers

Modify onScroll utils to accept arbitrary scroll element

754cb2a

Allow WindowScroller to receive custom scrollElement prop

1981162

Fix copy/paste error

b9399dd

Look at what element scroll listener was attached to, not initiator

a0df5f8

Update demo to include custom scroll element option

57efa5f

Fix case where prop changes from custom element to nothing

27f2b69

Make sure my custom element is scrolling, not ContentBox

e06f19e

Auto fix linter errors

ca60f00

Fix linter errors / things that will break server-side

8da79fb

Update pointerEvents assertions

6fd41a0

Fix failing "restore pointer events on unmount" test

18612a6

Add missing _updateDimensions call

8ff9cfc

andrewbranch commented Nov 22, 2016

View reviewed changes

bvaughn reviewed Nov 22, 2016

View reviewed changes

andrewbranch added 2 commits November 22, 2016 12:41

Replace PropTypes.object with any for HTMLElement props

dc7a7ac

Add autoprefixer-loader (for now?)

2a90fbf

bvaughn reviewed Nov 23, 2016

View reviewed changes

source/WindowScroller/utils/onScroll.js

@@ -1,5 +1,5 @@

import styles from '../WindowScroller.css'

Copy link

Owner

bvaughn Nov 23, 2016

Choose a reason for hiding this comment

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

Not using CSS modules in RV proper

bvaughn assigned andrewbranch Nov 24, 2016

romulof and others added 6 commits November 29, 2016 21:27

Updated cellRenderer callback parameters to also match List rowRenderer

a8f039f

Update CellMeasurer documentation

42493ce

Fixed code-style issue

2ddaed2

Updated tests for new CellMeasurer render :index param

919e342

Removed outdated warning about using CellMeasurer with List now that …

197a5c3

…it's supported by default

Updated CHANGELOG in advance of upcoming release

c7bb0b7

Brian Vaughn added 2 commits December 1, 2016 20:44

Merge branch 'arusakov-486_cell_size_cache_optimisation'

5c2b141

Prepping for 8.6.1 release

d44dde1

andrewbranch added 3 commits December 2, 2016 10:31

Fix checkbox bug/typo

188d691

Fix demo bug: make sure container does not shrink when Grid is not ye…

43691da

…t rendered

Fix bug where new height was calculated with old element (never changed)

c86015e

Brian Vaughn added 3 commits December 2, 2016 19:36

Added to to handle case when header items change or resize. also bett…

2e91ec7

…er handles window resize events.

Reverted part of the change introduced in version 8.6.0 that changed …

32454e0

…the behavior regarding controlled/uncontrolled and props and in a way that was not backwards compatible. (See issue #490 for more.)

Resolved conflicts

36a54f9

bvaughn reviewed Dec 3, 2016

View reviewed changes

Fix positionFromTop calculation

87fe0f1

andrewbranch commented Dec 15, 2016

View reviewed changes

andrewbranch added 2 commits December 22, 2016 10:25

Add scroll handlers to distinct scrollElements

f2199ac

Avoid some unnecessary updates to some WindowScrollers

7c78188

Previously, given two mounted WindowScrollers with different scrollElements, when either scrollElement was scrolled, both WindowScrollers were updated with { isScrolling: true }.

bvaughn closed this Jan 15, 2017

andrewbranch mentioned this pull request Jan 19, 2017

WindowScroller setting incorrect scrollTop after resizing window height #544

Closed

		@@ -0,0 +1,3 @@
		.disabled > * {
		pointer-events: none !important;

Add custom element option to WindowScroller #481

Add custom element option to WindowScroller #481

Conversation

andrewbranch commented Nov 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bvaughn Nov 23, 2016 • edited Loading

Choose a reason for hiding this comment

andrewbranch Nov 28, 2016 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bvaughn left a comment • 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

andrewbranch commented Nov 22, 2016 • edited Loading

andrewbranch commented Nov 22, 2016

Choose a reason for hiding this comment

bvaughn commented Nov 23, 2016

andrewbranch commented Nov 30, 2016

bvaughn commented Dec 2, 2016

andrewbranch commented Dec 2, 2016

bvaughn commented Dec 2, 2016

andrewbranch commented Dec 2, 2016

bvaughn commented Dec 3, 2016

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

Choose a reason for hiding this comment

codecov-io commented Dec 15, 2016 • edited Loading

Current coverage is 98.20% (diff: 94.54%)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bvaughn commented Jan 11, 2017

bvaughn commented Jan 11, 2017

andrewbranch commented Jan 11, 2017

bvaughn commented Jan 11, 2017

bvaughn commented Jan 15, 2017 • edited Loading

bvaughn commented Jan 15, 2017

bvaughn commented Jan 15, 2017

andrewbranch commented Jan 18, 2017

bvaughn commented Jan 18, 2017

bvaughn Nov 23, 2016 •

edited

Loading

andrewbranch Nov 28, 2016 •

edited

Loading

bvaughn left a comment •

edited

Loading

andrewbranch commented Nov 22, 2016 •

edited

Loading

codecov-io commented Dec 15, 2016 •

edited

Loading

bvaughn commented Jan 15, 2017 •

edited

Loading