Improve screen reader support #533
Comments
|
Hopefully can be done in way that would work with SimpleDOM's |
|
that would be awesome, yeah |
|
Looks like |
|
Related: ember-fastboot/fastboot#180 |
|
oh, nice! I'll see if I can get that running here locally :) |
// components/set-doc-lang.js
import Component from '@ember/component';
import { getOwner } from '@ember/application';
export default Component.extend({
tagName: '',
didReceiveAttrs() {
this._super(...arguments);
let dom = getDOM(this);
if (dom) {
let html = dom.documentElement;
html.setAttribute('lang', this.lang);
}
},
});
// from https://github.com/yapplabs/ember-wormhole/blob/0.5.4/addon/utils/dom.js#L45-L63
//
// Private Ember API usage. Get the dom implementation used by the current
// renderer, be it native browser DOM or Fastboot SimpleDOM
export function getDOM(context) {
let { renderer } = context;
if (!renderer._dom) {
// pre glimmer2
let container = getOwner ? getOwner(context) : context.container;
let documentService = container.lookup('service:-document');
if (documentService) {
return documentService;
}
renderer = container.lookup('renderer:-dom');
}
if (renderer._dom && renderer._dom.document) {
// pre Ember 2.6
return renderer._dom.document;
}
return null;
}this seems to work quite well, but it has the disadvantage of needing a component 🤔 |
|
I managed to make it work with the // myapp/utils/getDOM.js
import { getOwner } from '@ember/application';
// adjusted from https://github.com/yapplabs/ember-wormhole/blob/0.5.4/addon/utils/dom.js#L45-L63
//
// Private Ember API usage. Get the dom implementation used by the current
// renderer, be it native browser DOM or Fastboot SimpleDOM
export default function getDOM(context) {
let { renderer } = context;
if (!renderer || !renderer._dom) {
// pre glimmer2
let container = getOwner ? getOwner(context) : context.container;
let documentService = container.lookup('service:-document');
if (documentService) {
return documentService;
}
renderer = container.lookup('renderer:-dom');
}
if (renderer._dom && renderer._dom.document) {
// pre Ember 2.6
return renderer._dom.document;
}
return null;
}// myapp/services/intl.js
import Service from 'ember-intl/services/intl';
import getDOM from 'myapp/utils/get-dom';
export default Service.extend({
setLocale(locale) {
this._super(...arguments);
let dom = getDOM(this);
if (dom) {
let html = dom.documentElement;
html.setAttribute('lang', locale);
}
},
});This code works fine for me on Ember 3.1, but I have not tested it on any other Ember versions. In our case it goes through the |
I'm open to accepting extra features that may not work in older versions of Ember - just so long as it doesn't break older versions of Ember. What are your thoughts on the language attr updating logic being contained within a Component (your original approach)? I think it may catch people by surprise that the Service is updating the document directly. The downside is the user would need to be cognizant of dropping a component into their app template. The downside is also an upside in that if we're unable to support older versions of Ember the user can decide if they want to opted into it. The other upside is it will be easier for users to reason about what changed their html attribute. |
it would certainly have the advantage of being more explicit, but we're using Ember where everything should work right out of the box 😉 I'm not sure about this, but I think I prefer to do it right in the service if possible and possibly in the future provide an option to turn that behavior off if people complain about it. The only thing that is a little worrying is that the implementation uses private APIs and we should check with the core team if that is a good idea... |
Fair. Lets move forward with it on the service.
👍 agreed |
|
Most of the time I favor doing anything with DOM in components, even if that means an addon needs to tell people to put a singleton component in their templates. Trying to manipulate DOM from a service or route tends to make things fragile, especially in tests. This particular case is simple enough that if you do it carefully it’s probably ok. Bigger picture, there’s not really a strong reason we couldn’t make the |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
@Turbo87 Do you have time to / want to submit the PR for your code? Otherwise I'll submit the PR in your name. 🙏 |
|
@buschtoens feel free, happy to review 🙏 |
Closes #533. Based on the code provided by @Turbo87 in #533 (comment)
Closes #533. Based on the code provided by @Turbo87 in #533 (comment)
Closes #533. Based on the code provided by @Turbo87 in #533 (comment)
Closes #533. Based on the code provided by @Turbo87 in #533 (comment)
* feat: set document `lang` attribute Closes #533. Based on the code provided by @Turbo87 in #533 (comment) * feat: set document `lang` attribute Closes #533. Based on the code provided by @Turbo87 in #533 (comment) * refactor: call updateDocumentLanguage explicitly * chore: remove fix that belongs in extra PR * fix: syntax error * chore: fixing failing test.
The Lighthouse report often shows the following warning:
If an Ember app only has a single language then we can set that language in the
index.html, but when supporting multiple languages it gets a little more tricky. I would like to propose doing something like this:This should result in something like
<html lang="de">and will hopefully improve the Lighthouse score and screen reader support.The text was updated successfully, but these errors were encountered: