Skip to content
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

Prevent event responses to other targets #321

Merged
merged 5 commits into from
May 10, 2018

Conversation

quisido
Copy link
Contributor

@quisido quisido commented Apr 20, 2018

re: /issues/316

This is a pretty big shot in the dark, as I couldn't get build to work (.sh a Linux script?) to test, and I've never used the ES5 syntax for React before.

Logic is for ReactWheelHandler to have an optional HTMLElement and only fire events that belong to children of said element.
I couldn't conceptualize the Scrollbar component itself having React children that would alter the wheel event propagation, so I did not add the root HTMLElement parameter to its wheel listener.
Data Table has a ref listener on the parent-most div, which sets the wheelHandler's root element to that ref value.

Seems straight forward, let me know if I missed something obvious. I don't know how much time I have to dedicate to this as I have more pressing issues of my project to work on, so small fix requests like style would be appreciated on your end. 😁

@quisido quisido changed the title Prevent event responses to other tarets Prevent event responses to other targets Apr 20, 2018
@wcjordan
Copy link
Member

Thanks for adding. I don't have a good test case to see if this fixes the issue, but this seems reasonably fast, so I think we can add it behind a feature flag. I still want to verify it's sub 0.1 milliseconds in IE11, but it's well within that in chrome so should be alright. In the meantime could you add the feature flag?

@quisido
Copy link
Contributor Author

quisido commented Apr 26, 2018

By feature flag, do you mean a boolean prop that enables/disables it?

If you want a test case, the issue arose with material-ui's Popover. I haven't been able to test this either, without a way to build. QQ

import Table from 'fixed-data-table-2';
import Popover from 'material-ui/Popover';
import React from 'react';

const anchorOrigin = {
  horizontal: 'center',
  vertical: 'bottom'
};

const transformOrigin = {
  horizontal: 'right',
  vertical: 'top'
};

class MyComp extends React.PureComponent {

  constructor(props) {
    super(props);
    this.getRef = this.getRef.bind(this);
    this.onClick = this.onClick.bind(this);
    this.onClose = this.onClose.bind(this);
    this.onRef = this.onRef.bind(this);
    this.ref = null;
    this.state = { open: false };
  }

  getRef() {
    return this.ref;
  }

  onClick() {
    this.setState({ open: true });
  }

  onClose() {
    this.setState({ open: false });
  }

  onRef(ref) {
    this.ref = ref;
  }

  render() {
    return (
      <Table>
        <div onClick={this.onClick} ref={this.onRef}>test</div>
        <Popover
          anchorEl={this.getRef}
          anchorOrigin={anchorOrigin}
          children={
            <div style={{ height: '200px' }}>
              <div style={{ height: '400px' }}>Make me overflow.</div>
            </div>
          }
          onClose={this.onClose}
          open={this.state.open}
          transformOrigin={transformOrigin}
        />
      </Table>
    );
  }
}

You would need to add the props/Column components, as I don't know them offhand, but that should replicate the problem I was experiencing. Both Popover and div were in the header, so this should be valid even if there is no data to use to generate rows. If Popover has a scrollbar, this PR should scroll it with wheel, while master scrolls the table itself.

I'm freeballing this code as well, so sorry if anything is missing, but this should be like 99% of anything needed if it makes your life easier.

@wcjordan
Copy link
Member

wcjordan commented May 1, 2018

Yep by feature flag, I mean boolean prop. keyboardScrollEnabled or touchScrollEnabled are good examples.

Let me know when there's a working example I can test over. I'm happy to work off a JsFiddle or CodePen (see example from the issue template) or an example within the examples folder.

@quisido
Copy link
Contributor Author

quisido commented May 2, 2018

Do you have a mock for how I can link JsFiddle or CodePen to pull from a node package or GitHub repo (my branch)?

@wcjordan
Copy link
Member

wcjordan commented May 2, 2018

I recommend starting from this CodePen: https://codepen.io/alphalpha/pen/oqKJgG
You can point to your github repo by changing the rawgit links, but you'll need to update the built files for that to work.

@quisido
Copy link
Contributor Author

quisido commented May 3, 2018

https://codepen.io/charles-stover/pen/bMrbKz?editors=1010

This one points to this branch. Click any of the a1/b1/etc. texts, and a popup should appear. This is a sloppy-fast example, so the popups aren't particularly pretty. They only close if you press the Escape key while they are focused, as opposed to what you would probably expect of unfocusing them. Just for reference. But the scroll behavior does demonstrate the difference.

Open a popup, mouse wheel while you are over the popup, then mousewheel while you are over the table. The relevant scrollbar is the one that moves.

Change the package back to your original package, and do the same. The popup's scrollbar will not scroll until the table has first scrolled to the bottom, even when you have your mouse over the popup.

(You may need to adjust your screen size or add more arbitrary rows to make sure the table has a vertical scrollbar.)

Hope this is adequate!

@wcjordan
Copy link
Member

wcjordan commented May 4, 2018

Nice, this looks good to me! Thanks! Any luck with the flag? If you could add that, I think we're ready to go.

@quisido
Copy link
Contributor Author

quisido commented May 4, 2018

Pushed.

@quisido
Copy link
Contributor Author

quisido commented May 8, 2018

Is this still on track to merge?

@wcjordan wcjordan merged commit 333525d into schrodinger:master May 10, 2018
@wcjordan
Copy link
Member

Sorry for the delays, this is now released as part of v0.8.13

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 this pull request may close these issues.

None yet

2 participants