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

gecko_ia2 vbuf backend: Don't unnecessarily calculate labelledByContent (and thus unnecessarily fetch labelledBy). #11205

Merged
merged 2 commits into from
Jun 9, 2020

Conversation

jcsteh
Copy link
Contributor

@jcsteh jcsteh commented May 26, 2020

Link to issue number:

None.

Summary of the issue:

In #10552, we started querying the labelledBy relation in the gecko_ia2 vbuf backend for all objects. In a large document (e.g. a 10000 row table, which does happen on GitHub, etc.), this can result in thousands of unnecessary calls, increasing page load times by several seconds.

Description of how this pull request fixes the issue:

The labelledByContent ControlField attribute is only used when the name attribute is set and the name is an explicit name.
Previously, this was always calculated, which required the labelledBy relation to be fetched even if we weren't going to use this.
Instead, only calculate labelledByContent if we're going to use it.

This involves moving the code which calculates the name attribute further down in the function, since we can only calculate labelledByContent after descendants have been added to the buffer. However, no other code in the function depends on us setting the name attribute on the node, so this doesn't change the rendering.

Testing performed:

As per #10552:

Simplified test case:

data:text/html,<button>begin</button><div tabindex="0" role="checkbox" aria-labelledby="inner-label"><div style="display:inline" id="inner-label">Simulate evil cat</div></div>

Known issues with pull request:

None.

…nt (and thus unnecessarily fetch labelledBy).

The labelledByContent ControlField attribute is only used when the name attribute is set and the name is an explicit name.
Previously, this was always calculated, which required the labelledBy relation to be fetched even if we weren't going to use this.
In a document with many objects, this could result in thousands of unnecessary calls.
Instead, only calculate labelledByContent if we're going to use it.
@feerrenrut
Copy link
Contributor

Thanks @jcsteh

In the description, you mention that:

labelledByContent ControlField attribute is only used when the name attribute is set and the name is an explicit name.

Are you making this assertion based on the python code?
I.E. the usage of "labelledByContent" in speech/__init__.py?

I'm a little wary of having that logic/assumption duplicated,
and potentially going out of sync.

@jcsteh
Copy link
Contributor Author

jcsteh commented Jun 2, 2020 via email

@feerrenrut
Copy link
Contributor

There is something about that which doesn't quite make sense to me. If name always matches the content of a labelledby relation, why would we ever need to follow that relation. My assumption is that it allows the use of structured HTML that can be read in a way specific to the screen reader. I assume in these situations name will be some approximation of what a screen reader may produce for the labelledby relation? Is it required that a browser provide name when labelledby is present?

@feerrenrut
Copy link
Contributor

In either case of whether this just happens to be true for the browsers we currently support, or it is mandated by a spec, or because it matches the way we use this information on the python side (and therefore doesn't need to be provided in other situations), it would be helpful for someone in the future trying to unwind this logic if there was an explicit comment to say which it is.

@jcsteh
Copy link
Contributor Author

jcsteh commented Jun 3, 2020

If name always matches the content of a labelledby relation, why would we ever need to follow that relation.

Only because we want to know whether the label is visible. If it is, we don't want to render the name as the "content" because we'd end up with duplicate text. However, we still set the "name" attribute so focus, quick navigation, etc. can report it. The problem in this case is that the content and the labelledBy target are one and the same, hence the need for the labelledByContent attribute you added.

I assume in these situations name will be some approximation of what a screen reader may produce for the labelledby relation?

It's the flattened text of the labelledBy relation, but that means it lacks semantics and formatting.

Is it required that a browser provide name when labelledby is present?

Yes. The explicit-name attribute was added to tell screen readers when the name is specified by aria-labelledby, aria-label, HTML label for, etc.

In either case of whether this just happens to be true for the browsers we currently support, or it is mandated by a spec, or because it matches the way we use this information on the python side (and therefore doesn't need to be provided in other situations), it would be helpful for someone in the future trying to unwind this logic if there was an explicit comment to say which it is.

I'm of course happy to add a comment here, but I don't quite understand what isn't clear, so I'm not sure what that comment should say.

@jcsteh

This comment has been minimized.

@feerrenrut
Copy link
Contributor

Thanks for the extra info @jcsteh that clears up a lot of uncertainty. One thing I was forgetting is that there are justifiable reasons to want the element that acts as a label to also be announced if it is on another part of the page.

I reached the following conclusion about why visibility matters: Visibility matters for the purposes of avoiding double speaking. We can't double speak an invisible target of a labelledby relation because we exclude hidden elements from our virtual buffer creation. However, it will still be used by the browser to calculate the accessible name, and thus still label the element.

I wrote the following set of questions and thought processes before reaching this conclusion. I'll leave them here in-case it is useful to anyone (probably myself) in the future.

Only because we want to know whether the label is visible.

First, I assume in this case we are talking about visibly displayed by the browser, rather than aria-hidden.
If a web author uses aria-labelledby and the target is intentionally not visible, why not speak it? Visual context may be enough, but for AT an explicit label might be necessary. For instance imagine a collapsible legend, the colors used may give enough context for most visual users if not they can expand a legend, for AT it would be useful to label the colored elements with the collapsed legend elements?
There is this article that suggests that visibility shouldn't matter: https://developer.paciellogroup.com/blog/2015/05/short-note-on-aria-labelledby-and-aria-describedby/

@feerrenrut
Copy link
Contributor

It would be interesting to know which part of this causes the performance impact. I'd guess fetching the label info (GeckoVBufBackend_t::getLabelInfo)

@feerrenrut feerrenrut merged commit 9f96d03 into nvaccess:master Jun 9, 2020
@nvaccessAuto nvaccessAuto added this to the 2020.2 milestone Jun 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants