Don't autofocus if the entire react-data-grid is offscreen #704
Conversation
|
@theunixbeard thanks for opening such a detailed PR! |
|
Changes Unknown when pulling 716060c on theunixbeard:dont-autofocus-from-offscreen into ** on adazzle:master**. |
|
Changes Unknown when pulling 716060c on theunixbeard:dont-autofocus-from-offscreen into ** on adazzle:master**. |
|
Good idea. I think it would make more sense to call the method 'isTableOnScreen` or something like that, rather than tying it in with the isFocusedOnBody method Also see issue #714 - i think it should not autofocus when it's focused on the body because chrome & firefox set focus to body during blur which means it may steal focus from other parts of the page, even though it has no business doing so. |
theunixbeard
force-pushed
the
dont-autofocus-from-offscreen
branch
2 times, most recently
from
4772db0 to
643ace7
Compare
|
Changes Unknown when pulling 643ace7 on theunixbeard:dont-autofocus-from-offscreen into ** on adazzle:master**. |
theunixbeard
force-pushed
the
dont-autofocus-from-offscreen
branch
4 times, most recently
from
9c0d2f4 to
e3538cb
Compare
theunixbeard
force-pushed
the
dont-autofocus-from-offscreen
branch
from
e3538cb to
28bbe86
Compare
|
Changes Unknown when pulling 28bbe86 on theunixbeard:dont-autofocus-from-offscreen into ** on adazzle:master**. |
| return false; | ||
| }, | ||
|
|
||
| isFocusedOnBody(dataGridDOMNode) { |
There was a problem hiding this comment.
hi @theunixbeard , it would be better if you changed this back to the original isFocusedOnBody function, and then...
| @@ -363,7 +379,7 @@ const Cell = React.createClass({ | |||
| // Only focus to the current cell if the currently active node in the document is within the data grid. | |||
| // Meaning focus should not be stolen from elements that the grid doesnt control. | |||
| let dataGridDOMNode = this.props.cellMetaData && this.props.cellMetaData.getDataGridDOMNode ? this.props.cellMetaData.getDataGridDOMNode() : null; | |||
| if (this.isFocusedOnCell() || this.isFocusedOnBody() || (dataGridDOMNode && dataGridDOMNode.contains(document.activeElement))) { | |||
| if (this.isFocusedOnCell() || this.isFocusedOnBody(dataGridDOMNode) || (dataGridDOMNode && dataGridDOMNode.contains(document.activeElement))) { | |||
There was a problem hiding this comment.
... change this if statement to include || this.isFocusedOnBody() || this.isTableOnScreen(dataGridDOMNode)
(Or better still remove this.isFocusedOnBody() altogether.)
|
Closing this issue, as auto focus will be removed in v 4.0 of the grid. The plan is to move the responsibility of cell section out of the Cell component and into a dedicated SelectionMask that sits at canvas level. This will provide the following benefits
|
Description
Due to the current autofocus behavior, when the react-data-grid is rerendered the scroll bar 'jumps' up to put the focused cell in view, even if it was a completely different section of the UI that caused the underlying data to change. This would more often occur in applications with complex UIs such as the one I am working on.
I created a short 1-minute video demonstrating the issue & explaining my fix:
https://www.youtube.com/watch?v=xejQdVGJX6w
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (check one with "x")
What is the current behavior? (You can also link to an open issue here)
When the react-data-grid is rerendered the scroll bar 'jumps' up to put the focused cell in view, even if it was a completely different section of the UI that caused the underlying data to change.
What is the new behavior?
In the isFocusedOnBody() method in Cell.js, we simply check if the dataGridDOMNode is currently on screen or off screen. If it's off screen, then don't trigger the autofocus.
Does this PR introduce a breaking change? (check one with "x")
If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...
Other information:
Tests have been added. I also ran the linter and that gave a clean output too.
Thanks for taking a look at this PR!