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

focus container on page load #116

Merged
merged 4 commits into from Apr 17, 2018

Conversation

Projects
None yet
2 participants
@mlcdf
Contributor

mlcdf commented Apr 14, 2018

On page load, the focus is set the main content area instead of on the nav bar.

This fixes (#99).

@nolanlawson

This comment has been minimized.

Owner

nolanlawson commented Apr 14, 2018

Hmm, it looks like this is failing the tests because it will change the focus anytime Layout.html is created, which basically means anytime we navigate between different pages. In some cases we want the focus to be something else (e.g. you click a toot, you navigate from a timeline view to a thread view, you press the back button, and now you're still focused on the toot).

Is there a way to do this without JavaScript? If not, maybe the most practical thing is to put the focus() in main.js so that it only runs only when the very first page is loaded, and not during in-app navigations.

@mlcdf

This comment has been minimized.

Contributor

mlcdf commented Apr 15, 2018

Oh, I see. I'll look into that.

@nolanlawson

This comment has been minimized.

Owner

nolanlawson commented Apr 15, 2018

Another option: I just added a change to LazyPage.html (which loads for all routes) to detect first load versus every subsequent load. We could use the same trick in Layout.html.

I may be best to do this in Layout.html because we don't even need to call querySelectorAll('.container'): you can use ref:node and then this.refs.node in Svelte to get the node of a rendered element.

@mlcdf mlcdf force-pushed the mlcdf:focus-container-on-page-load branch from f2e02ed to 645f497 Apr 16, 2018

@mlcdf mlcdf force-pushed the mlcdf:focus-container-on-page-load branch 2 times, most recently from 610794d to 5a6b638 Apr 16, 2018

@mlcdf

This comment has been minimized.

Contributor

mlcdf commented Apr 16, 2018

Thanks for the lead on this. Now, I'll try to make this work!

@mlcdf mlcdf force-pushed the mlcdf:focus-container-on-page-load branch from 5a6b638 to fab4f76 Apr 16, 2018

@mlcdf mlcdf force-pushed the mlcdf:focus-container-on-page-load branch from fab4f76 to 58942df Apr 16, 2018

@nolanlawson

This comment has been minimized.

Owner

nolanlawson commented Apr 17, 2018

That's odd; the code looks correct to me but some tests are failing. I'll look into this when I get a chance!

@mlcdf

This comment has been minimized.

Contributor

mlcdf commented Apr 17, 2018

I tried to reproduce those failing tests locally, and it worked (it produced the expected result). So, I'm kinda lost on this one.

@mlcdf mlcdf force-pushed the mlcdf:focus-container-on-page-load branch from d733607 to f7e1a8e Apr 17, 2018

@nolanlawson

This comment has been minimized.

Owner

nolanlawson commented Apr 17, 2018

Hmmm, I can reproduce using Chrome on Mac with the command:

npx testcafe --hostname localhost chrome --skip-js-errors tests/spec/010-focus.js

Oddly though I can't reproduce manually when using Chrome. It seems there may be some timing issue in a11y-dialog that causes it to think the previous focused element was .container rather than the .play-video-button (when clicking that button).

@nolanlawson

This comment has been minimized.

Owner

nolanlawson commented Apr 17, 2018

^ I couldn't push to your branch for some reason, here's my attempted fix

@nolanlawson nolanlawson merged commit eef54e9 into nolanlawson:master Apr 17, 2018

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
@mlcdf

This comment has been minimized.

Contributor

mlcdf commented Apr 17, 2018

Thank you for the support @nolanlawson!

@mlcdf mlcdf deleted the mlcdf:focus-container-on-page-load branch Apr 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment