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.

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

Jump to bottom

Keyboard navigation doesn't work properly with an initially empty grid with items added via notifications #1259

Closed
kfranqueiro opened this issue Mar 11, 2016 · 2 comments
Closed

Keyboard navigation doesn't work properly with an initially empty grid with items added via notifications #1259

kfranqueiro opened this issue Mar 11, 2016 · 2 comments
Milestone
1.1.0

Comments

@kfranqueiro
Copy link
Member

This is sort of tangential to #1181, but has definite repercussions for usability. In the test case below, you cannot tab into the individual item that was added - it still focuses contentNode.

<!DOCTYPE html>
<html>
<head>
<link rel="stylesheet" href="../css/dgrid.css">
</head>
<body>
<div id="grid"></div>
<script src="../../dojo/dojo.js" data-dojo-config="async: true"></script>
<script>
require([
    'dojo/_base/declare',
    'dgrid/OnDemandGrid',
    'dgrid/Keyboard',
    'dstore/Memory',
    'dstore/Trackable'
], function (
    declare,
    OnDemandGrid,
    Keyboard,
    Memory,
    Trackable
) {
    var store = new (declare([ Memory, Trackable ]))({ data: [] });
    var grid = new (declare([ OnDemandGrid, Keyboard ]))({
        columns: {
            id: 'ID'
        },
        collection: store
    }, 'grid');

    store.addSync({});
});
</script>
</body>
</html>
@kfranqueiro
Copy link
Member Author

Pushed a branch/commit with a fix for this. I'll mull it over and merge next week or the week after (probably won't have much time for dgrid next week).

I ended up resolving this using an _onNotification aspect. I used aspect because it fit in better with the existing code, and aspected on _onNotification mainly to avoid trouble / edge cases with removeRow if I were to try aspecting that directly, given that the main use case here involves stores anyway.

@kfranqueiro
Copy link
Member Author

I'm merging my commit since I've had success testing this fix in an app for the past week (in addition to the unit tests passing).

kfranqueiro pushed a commit that referenced this issue Mar 24, 2016
Fix #1259: Keyboard: Fix and test focus in sometimes-empty grid
f9cf1c4 
(cherry picked from commit c578425)
kfranqueiro pushed a commit that referenced this issue Mar 24, 2016
Fix #1259: Keyboard: Fix and test focus in sometimes-empty grid
360ddbc 
(cherry picked from commit c578425)
@kfranqueiro kfranqueiro added this to the 1.0.1 milestone Apr 25, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.1.0
Development

No branches or pull requests
1 participant
@kfranqueiro