In-page hash navigation does not set focus

[AlmeroSteyn]

I have initially logged this as an issue on the gatsby-mdx repo here;

Describe the bug When I click an anchor that refers to a location inside the page, the destination element does not receive proper focus. Although the browser scroll position is reset properly, the destination element is not read out by screen readers as it does not receive focus correctly.

To Reproduce

1. Clone repository https://github.com/AlmeroSteyn/gatsby-mdx-minimal-repro
2. Go to homepage
3. Click on the link right at the top named Skip to content
4. Note how the container below with the text This is some content does not get a focus outline.
5. Now go to route /focus which is a straight React component without MDX.
6. Click on link right at the top named Skip to content
7. Note how the container below with text This is some content DOES get a focus outline on this page.

Both of these pages uses te same Layout component that contains the link and in-page navigation destination. When used in an MDX file it doesn't work, while using it as a straight page component does work

When I was looking into why this was happening I noticed that on the MDX pages the entire content is remounted even when the hash portion of the URL changes. This breaks the standard browser behaviour of setting focus to the target element.

Because focus is not set correctly, screen readers will not announce to the users that any action has been performed.

During triage of the issue on the gatsby-mdx repo it was found that setting a shouldComponentUpdate flag based on the location.hash value on the MDXTag component will fix the issue in some cases. However the following cases should work and properly set focus to the element in question:

1. Clicking a hash anchor when the browser URL is not set to the hash location.
2. Clicking a hash anchor when the browser URL was set before by a previous click.
3. Refreshing a page when the browser URL already contains the hash.

Expected behavior Navigating to an element with an id corresponding to the hash in the href of the anchor should set focus on the element to allow screen reader users to also use applications built with MDX.

[johno]

Thank you for the thorough bug report @AlmeroSteyn. I'll have a deeper look into it this weekend and report back next week.

Initial findings

At an initial glance I think it has to do with the layoutProps being passed to MDXRenderer/MDXTag within MDX and gatsby-mdx internals. Currently they're all forwarded along which will cause a rerender when the window location changes. When the rerender happens the DOM node to match the hash is then removed causing the browser to lose focus.

MDX forwards along any props to MDXTag by design since it has no knowledge of context or its consumer. Components that are leveraged might want to access them as well. I wonder if in this scenario we should be destructuring only the props we care about and passing along to MDXTag more explicitly from MDXRenderer. Perhaps you have some ideas @ChristopherBiscardi?

[ChristopherBiscardi]

I still have some investigating to do on this issue. In this case the MDXRenderer isn't being used, it's just an mdx file loaded through the webpack loader "as usual" so should act just like any regular MDX.

I reproduced the issue by first confirming that MDX in a create-react-app works as desired, then I added reach-router and ironically, adding reach-router causes the accessibility issue because location change is getting pushed through by reach-router. I'm having a hard time finding a location to control the props passed to MDX since this is a page whose props controlled by "everyone else" (gatsby's page renderer, reach-router, MDXTag, etc).

The core of the problem seems to be that wrapper will always be re-mounted by MDXTag and so you can't use sCU on an MDXProvider components {{wrapper: Wrapper}} to fix that. It has to be done in MDXTag because the wrapper itself is already re-mounted by the time we're rendering layout, etc. (I think this section has errors but leaving it since I wrote it while investigating)

---

After some further investigation where I scattered key around a bunch in MDXTag and @mdx-js/mdx/mdx-hast-to-jsx I'm wondering if the problem is that the Layout prop is an inline function and thus, not stable.

[ChristopherBiscardi]

yes. I have to clean it up a bit, but the ultimate change apart from strategically placing key on MDX-controlled elements like wrapper and layout was changing the Layout export from

export default ({ children, ...props }) => <Layout {...props}>{children}</Layout>

to

export default Layout

The first results in an unstable component definition by placing the function inline in render.

[ChristopherBiscardi]

that is, this is where I think the inline function difference matters -- https://github.com/mdx-js/mdx/blob/e6a7b03fc7df49de4e05b6ff1bf4d7b693ff39b7/packages/mdx/mdx-hast-to-jsx.js#L98

[ChristopherBiscardi]

@AlmeroSteyn I've confirmed that the above layout change works in your repro repo. Please try it and let me know.

@johno I don't think we need keys (or any other changes I was making) anywhere but this does seem like an awkward footgun in mdx that should be documented at least.

[johno]

Thanks for the investigation @ChristopherBiscardi. This is great

@johno I don't think we need keys (or any other changes I was making) anywhere but this does seem like an awkward footgun in mdx that should be documented at least.

Yeah, we should definitely document. We should also do some thinking on how we can modify the API to make it less of a footgun, especially since it implicitly effects accessibility when router props are passed.

[johno]

I wonder if we could create some type of API to not forward all props by default and force some type of opt-in? When we were initially designing this we never thought about the implications of random props being passed in (like a router). I'll bet this has some performance implications on large MDX documents as well. Rerendering an entire doc for an unused prop is quite wasteful.

[ChristopherBiscardi]

I don't think we need to create a new API to fix the current remounting issue (and personally, I'd avoid creating the perf-related feature unless someone had a need for it., I think with this change that high-cost components can be sCU'd themselves rather than building it into mdx).

I've PR'd a fix to MDX that should fix the remounting issue https://github.com/mdx-js/mdx/pull/309

[AlmeroSteyn]

@ChristopherBiscardi I can confirm that the change above does fix the focus issue for the wrapper.

However other internal page hash links still does not work. So if, for instance, I have anchors pointing to headings inside the docs.

[AlmeroSteyn]

@ChristopherBiscardi This is great news! Thanks for jumping on this. Looking forward to the release.

[silvenon]

@AlmeroSteyn released! 😉

[AlmeroSteyn]

@silvenon Thank you! Will check this out first thing tomorrow. This fix will really help screen reader users a lot.

[silvenon]

Thanks for reporting this bug, we don't want to get in the way of a11y!

[AlmeroSteyn]

@silvenon @ChristopherBiscardi I attempted the upgrade to the new version today but it makes my application crash miserably.

In my app it crashes with Uncaught ReferenceError: props is not defined.

So I tried it on the minimal repo above : https://github.com/AlmeroSteyn/gatsby-mdx-minimal-repro

If I bump that to v0.16.0 it still works. The moment I go to v0.16.1 or above it crashes with the following error:

Uncaught TypeError: Cannot set property 'layout' of undefined

Layout is exported as:

import layout from '../components/layout'

export default ({ children, ...props }) => <layout {...props}>{children}</layout>

What am I missing? And what is the upgrade path to this release. I cannot find it in the docs or the issues.

Thanks!

[silvenon]

I recommend locking to v0.16.0 for now. We need to figure out a way to reliably fix this everywhere and add a more comprehensive test suite.

[AlmeroSteyn]

Thanks, will do. Was pretty excited about the focus issue being solved though ;-)

[silvenon]

Yeah, sorry about that. 😕 MDX is used in all kinds of contexts, so we need to make sure we get to a stable place before moving forward. I don't think it's going to take long.

[AlmeroSteyn]

No worries! I understand how it goes and it is an awesome tool!

I got a workaround in place for now by manually setting focus inside the Layout component so not dead in the water.

[silvenon]

Should be fixed now in v0.16.4.

[AlmeroSteyn]

@silvenon Thanks for the update. And thanks for the effort!

I can confirm that in my minimal test it works!

That's the good news. But there is also bad news :-(

Where I am using it in our project it still does not work.

The difference between the two is that in our project I am using the MDXProvider component and overriding a few of the standard components.

It seems that everything inside this component is still rerendering when I click on the <anchor> tag that points to an internal id.

Does that make sense? And is it expected? If so that is still breaking normal browser focus functionality.

What are you thoughts?

[silvenon]

I'll investigate.

[AlmeroSteyn]

Thank you!

[AlmeroSteyn]

@silvenon Ok I found the issue.

Diving into the source code for MDXProvider I saw that the components object was passed directly to the React Context provider. And I declared my custom configuration object inline in the JSX.

Which means that the Consumers updated with each render as a new object reference was created.

When I pulled it out of the render function it works as expected.

So I went back to the docs: https://mdxjs.com/getting-started/

The example there is, of course, good. AND it states that it uses Context.

However it may be a good idea to clearly explain that this particular caveat of Context also exists here. I really did not think about that when I initially used it as I thought of it as a normal prop.

BUT that said would it not be a better idea to manage this in the component itself so the users don't have to worry about it? Can probably be fixed with a memoized function deep checking the components prop.

If you want to go the code change way I can try my hand at a PR.

[ChristopherBiscardi]

Can probably be fixed with a memoized function deep checking the components prop.

Can it account for potentially new function references too if a given function is defined inline? ala

<MDXProvider components={{
  h1: ({children, ...props}) => <h1>{children}</h1>
}}>

---

nicely done on the digging btw, I think that may be the last piece to get some MDX react-pose staggering I was working on.

[AlmeroSteyn]

Hmm, spent some time on it this morning.

If this is done immutably, with the components object outside the render function it just works.

I was unable to find a way yet to use memoize and deep equals to solidly prevent this from happening.

Because it is declared inline, the object and function references ALWAYS changes so the deep equal will always flag it as different. My idea there was not correct. It suffers from exactly the same effect.

I fear that the only way may be to highlight this in the docs as React did (https://reactjs.org/docs/context.html#caveats).

So, for the case where the component prop is declared once it can live on a class variable or this.state. If it needs to be mutated it should be on this.state.

And in future with hooks that would be either on a ref or in a useState hook.

It should actually never be declared inline.

[silvenon]

If it’s a one-off, it can also be declared in an outside variable (I keep them in a separate file).

Would you like to send a PR outlining this caveat in the docs, also adding that link to React docs?

[AlmeroSteyn]

Yeah that also works, of course. I like having these sort of things collocated in one file but that's just personal taste.

Yes will do! First moment I have 👍

[AlmeroSteyn]

@silvenon I tried to put the PR together but getting it to commit is taking more time than I have right now. So sorry. I'm working on a Windows environment and things like the format script is failing for me. Also having line ending hell as all LF gets changed to CRLF when Prettier runs and then it wants to commit all files.

I'd get there eventually but gotta turn attention to other things here right now so would it be OK if I give you the text I was going to add to the Getting Started doc so you can add it?

Here it is:

How it works

MDXProvider uses [React Context][] to provide the component mapping to MDXTag. This way MDXTag knows that it should override the standard components with those provided in this mapping.

Important usage intructions

Because MDXProvider uses React Context directly, it is affected by the same caveats.

It is therefore important that you DON'T declare your components mapping inline in the JSX. Doing so will trigger a rerender of your entire MDX page with every render cycle. Not only is this bad for performance, but it can cause unwanted side affects, like breaking in-page browser navigation.

Avoid this by following the pattern shown above and declare your mapping as a constant.

Updating the mapping object during application runtime

If you need to change the mapping during runtime, declare it on the component's state object:

import React from 'react'
import { MDXProvider } from '@mdx-js/tag'

import { Heading, Text, Pre, Code, Table } from './components'

export default class Layout extends React.Component {
  state = {
    h1: Heading.H1,
    h2: Heading.H2,
    // ...
    p: Text,
    code: Pre,
    inlineCode: Code
  };

  render(){
    const {children, ...props} = this.props;
    return <MDXProvider components={this.state}>
        <main {...props} >
          {children}
          </main>
    </MDXProvider>
  }
}

You can now use the standard setState function to update the mapping object and be assured that it won't trigger unnecessary renders.

[silvenon]

I tried to put the PR together but getting it to commit is taking more time than I have right now. So sorry. I'm working on a Windows environment and things like the format script is failing for me. Also having line ending hell as all LF gets changed to CRLF when Prettier runs and then it wants to commit all files.

I'm terribly sorry to hear that. When you have time I'd like to pair with you to solve the development environment, because the point of that setup was to minimize the friction of enforcing a code style.

Thanks for writing the docs here, your great! I'll set you as the author when I'm committing the changes, so it should still show up as your contribution.

[AlmeroSteyn]

@silvenon I'd like that yes! Next week sometime?

[silvenon]

Yeah! You can send me a DM on Twitter.

[AlmeroSteyn]

Will do!