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

fix(gatsby): fix not focusing router wrapper after navigation #13197

Merged
merged 26 commits into from Jul 3, 2019

Conversation

@pieh
Copy link
Contributor

pieh commented Apr 7, 2019

This is work in progress to assert that focus management is working as expected on navigation.

Right now we are misusing @reach/router and don't take full advantage of its focus management feature because we have with single catch-all route - which means that this route never change and focus is never triggered.

Initially I'll try to push just failing test (to verify correctness), with actual changes following that

@pieh pieh requested a review from gatsbyjs/core as a code owner Apr 7, 2019
@pieh pieh changed the title [wip] tests(gatsby): test focus management on navigation [wip] fix(gatsby): fix not focusing router wrapper after navigation Apr 7, 2019
@DSchau DSchau added the status: WIP label Apr 8, 2019
@@ -33,3 +33,7 @@ Cypress.Commands.add(`shouldNotMatchScrollPosition`, id =>
expect(scrollPosition).not.to.be.closeTo(storedScrollPositions[id], 100)
})
)

Cypress.Commands.add(`assertRouterWrapperFocus`, () =>

This comment has been minimized.

Copy link
@marcysutton

marcysutton Apr 8, 2019

Member

This is dope! 👍

const target =
stage === `build-html` || stage === `develop-html` ? `node` : `web`
if (target === `web`) {
// force to use es modules when importing internals of @reach.router

This comment has been minimized.

Copy link
@marcysutton

marcysutton Apr 8, 2019

Member

Should we open an issue on @reach/router to make any changes?

This comment has been minimized.

Copy link
@pieh

pieh Apr 8, 2019

Author Contributor

We do import internal match function (to match those parameterized / wildcarderded paths like /app/:someParam or /app/*). If @reach/router exported that from entry point, we wouldn't need this part, but I'm not sure if Ryan would really want that - as this is lower level function. Worth asking at least ;)

This comment has been minimized.

Copy link
@wardpeet

wardpeet May 16, 2019

Member

@pieh are you sure we need this? https://github.com/reach/router/blob/master/package.json#L6 should take care of it.

Nvm I see what you did there, it's because we require @reach/router/lib/utils

@pieh

This comment has been minimized.

Copy link
Contributor Author

pieh commented May 16, 2019

finally got unblocked on this, next step here is to adjust gatsby develop to use similar technique (right now only production builds are handled)

dangerouslySetInnerHTML={{ __html: `` }}
suppressHydrationWarning
/>
)

This comment has been minimized.

Copy link
@pieh

pieh May 16, 2019

Author Contributor

@davidbailey00 I would like sanity check here:
this seems to do similar thing (preventing react from cleaning up ___gatsby div) and it works in more scenarios than throwing (in case we throw before any parent component "renders" any DOM element, react will wipe ___gatsby div). It seems like we were "lucky" with this, because

was always there coming from above . My change moves below so no DOM element is above, which was causing issues.

This comment has been minimized.

Copy link
@wardpeet

wardpeet May 16, 2019

Member

this means we will do a full client rehydration if something goes wrong, right? maybe add a comment about it?

This comment has been minimized.

Copy link
@pieh

pieh May 16, 2019

Author Contributor

It's partial hydration - things above the actual page will be hydrated - so if users use wrapRootElement/wrapPageElement or gatsby-plugin-layout those will be actually hydrated, but the content of the page itself (in case template chunk or query results can't be loaded) won't be trully hydrated, but content from static html will be there

This is a neat trick that you can do when using:

<div
          dangerouslySetInnerHTML={{ __html: `` }}
          suppressHydrationWarning
        />

during hydration, as it won't try to actually change DOM nodes under it.

But yeah, I should add more context there for sure

@wardpeet

This comment has been minimized.

Copy link
Member

wardpeet commented May 16, 2019

so the fix is actually bringing all routes under the as separate pages? This way Reach router knows about all paths and can do its magic tricks?

@pieh

This comment has been minimized.

Copy link
Contributor Author

pieh commented May 16, 2019

so the fix is actually bringing all routes under the as separate pages? This way Reach router knows about all paths and can do its magic tricks?

We still have one route, but it's dynamic - see <PageRenderer> path prop under <Router>. This should make sure that @reach/router detect that route has actually changed and will properly trigger focusing router's wrapper div (which should tell assistive tech that something happened on navigation)

edit:
See e2e tests I added and result for those without any code changes in https://circleci.com/gh/gatsbyjs/gatsby/102743?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link (so right now in master, router wrapper is not being focused on navigation)

pieh added 3 commits May 19, 2019
navigating from and to 404 doesn't focus router wrapper correctly. This is because 404 in development is special case (see notes before skipped e2e-tests/development-runtime accessibility tests). This can be fixed but require a lot of changes which would be out of scope of this PR and would potentially make it harder to merge page manifest changes, so for now lets ship fixes that are already here.
@pieh pieh changed the title [wip] fix(gatsby): fix not focusing router wrapper after navigation fix(gatsby): fix not focusing router wrapper after navigation May 19, 2019
@pieh

This comment has been minimized.

Copy link
Contributor Author

pieh commented May 19, 2019

This should be ready to review. Here's demo with voiceover for production build https://www.dropbox.com/s/tbrt2uczih2jolk/Kapture%202019-05-18%20at%2017.21.45.mp4?dl=0 - page content is being announced properly (I think). To live check it, you can use for example https://deploy-preview-13197--using-remark.netlify.com/ which is build with changes from this PR

Notes:

  • There is problematic scenario that I didn't fix in development (see skipped tests in development-runtime). Problematic scenario is navigating in development from or to 404 page - this is because 404 in development is pretty special case and we have 2 code paths that it's pretty difficult to properly use router there. This code may also significantly change for page manifest changes so I chose to not spend too much time on it and focus on shipping this as this fixes majority of issues with focus management
  • I duplicated some code in production and runtime e2e tests - not sure if extra effort to keep it DRY is worth the time investment? It might be more worth if we have more duplicated tests across those two?
@pieh

This comment has been minimized.

Copy link
Contributor Author

pieh commented Jun 24, 2019

We chatted with Marcy about this and plan is to merge this PR (after resolving conflicts) and then do research on how could we implement further improvements that would focus just content area.

@pieh pieh force-pushed the pieh:reach-focus branch from 99357f1 to 5996d80 Jun 25, 2019
pieh added 2 commits Jun 25, 2019
@pieh

This comment has been minimized.

Copy link
Contributor Author

pieh commented Jun 25, 2019

Merge conflicts after per-page-manifest merge are resolved and this is ready for review

@@ -7,7 +7,7 @@ const NotFound = () => <h1>Not Found in App</h1>

const AppPage = () => (
<Router>
<App path="/paths/app" />

This comment has been minimized.

Copy link
@sidharthachatterjee
pieh added 2 commits Jul 3, 2019
@sidharthachatterjee sidharthachatterjee self-requested a review Jul 3, 2019
Copy link
Member

sidharthachatterjee left a comment

🌮🌮🌮

@gatsbybot gatsbybot merged commit b8e2adc into gatsbyjs:master Jul 3, 2019
18 checks passed
18 checks passed
Danger All good
Details
Peril All green. Yay.
Details
ci/circleci: bootstrap Your tests passed on CircleCI!
Details
ci/circleci: e2e_tests_development_runtime Your tests passed on CircleCI!
Details
ci/circleci: e2e_tests_gatsby-image Your tests passed on CircleCI!
Details
ci/circleci: e2e_tests_gatsbygram Your tests passed on CircleCI!
Details
ci/circleci: e2e_tests_path-prefix Your tests passed on CircleCI!
Details
ci/circleci: e2e_tests_production_runtime Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_gatsby_pipeline Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_long_term_caching Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: starters_validate Your tests passed on CircleCI!
Details
ci/circleci: theme_starters_validate Your tests passed on CircleCI!
Details
ci/circleci: unit_tests_node10 Your tests passed on CircleCI!
Details
ci/circleci: unit_tests_node12 Your tests passed on CircleCI!
Details
ci/circleci: unit_tests_node8 Your tests passed on CircleCI!
Details
ci/circleci: unit_tests_www Your tests passed on CircleCI!
Details
unit_tests_windows Build #20190703.123 succeeded
Details
@sidharthachatterjee

This comment has been minimized.

Copy link
Member

sidharthachatterjee commented Jul 3, 2019

Published in gatsby@2.13.2

@pieh pieh deleted the pieh:reach-focus branch Jul 4, 2019
johno added a commit to johno/gatsby that referenced this pull request Jul 17, 2019
…js#13197)

* add probably failing test

* rename custom command to make it clear it's assertion

* first find page, then use router + import from @reach/router/es

* revert grabbing @reach/router/es directly

* use webpack alias to use @reach/router es build for browser bundles

* update snapshots to cover added id to focus wrapper

* testing

* mind blown

* use BASE_PATH and PATH_PREFIX

* fix focusing on navigation from 404 to different 404, improve focus managment tests

* fix most of focus navigation focus managment in development

navigating from and to 404 doesn't focus router wrapper correctly. This is because 404 in development is special case (see notes before skipped e2e-tests/development-runtime accessibility tests). This can be fixed but require a lot of changes which would be out of scope of this PR and would potentially make it harder to merge page manifest changes, so for now lets ship fixes that are already here.

* remove stray console.log

* add note about partial hydration when missing resources and hydrating

* update dev runtime

* update snapshot

* use nested route

* add matchPath to pageResources object

* update snapshot

* remove stray console.log
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.