[web] make text more crawlable; fix a JAWS bug #50794
Conversation
30a14d2 to
c5ece2b
Compare
| final List<RegExp> _crawlerRegexes = <RegExp>[ | ||
| // Covers the search engine and various specialized crawlers: https://developers.google.com/search/docs/crawling-indexing/overview-google-crawlers | ||
| RegExp(r'googlebot'), | ||
| RegExp(r'\-google'), |
There was a problem hiding this comment.
This a good start though it doesn't capture all the user agents that Google lists in that url (e.g. Google-Extended, GoogleOther).
This is fine for now but let's file an issue maybe to improve the regexes?
There was a problem hiding this comment.
I'm actually starting to wonder if the engine should contain any crawler patterns at all. Maybe these belong in a package?
mdebbar
left a comment
There was a problem hiding this comment.
I have a few minor suggestions but nothing major.
LGTM!
| DomElement? span = _labelSpan; | ||
|
|
||
| // Lazy-create and cache the span. | ||
| if (span == null) { | ||
| _labelSpan = span = createDomElement('span'); | ||
| semanticsObject.element.appendChild(span); | ||
| } | ||
|
|
||
| span.innerText = label; |
There was a problem hiding this comment.
You could do something fancier like this:
| DomElement? span = _labelSpan; | |
| // Lazy-create and cache the span. | |
| if (span == null) { | |
| _labelSpan = span = createDomElement('span'); | |
| semanticsObject.element.appendChild(span); | |
| } | |
| span.innerText = label; | |
| // Lazy-create and cache the span. | |
| _labelSpan ??= semanticsObject.element.appendChild(createDomElement('span')); | |
| _labelSpan.innerText = label; |
but it might hurt readability a bit.
There was a problem hiding this comment.
Hmm, tried it, but doesn't fully work. The local variable is needed so that the analyzer can prove that it's not null after initialization. _labelSpan is nullable and therefore requires a null assertion (line _labelSpan.innerText = label; does not compile). Also, appendChild return DomNode instead of DomElement, which requires an extra dynamic cast. Keeping the existing variant for now.
| DomElement? _labelSpan; | ||
|
|
||
| void _updateLabel(String label) { | ||
| owner.setAttribute('aria-label', label); |
There was a problem hiding this comment.
Maybe a comment here explaining why we are setting aria-label even when needsDomText is true?
There was a problem hiding this comment.
You made me realize something. I did this originally so that the aria label rolls up to the parent node, which is correctly sized. Span doesn't match the rect reported by the framework. However, since I'm only using the span for leaf nodes, I shouldn't need the span at all. I think I can just do semanticsObject.element.innerText = label. Let me give it a shot.
| /// It is expected that this function is called early in the app's | ||
| /// initialization, before the framework renders the semantics tree, e.g. prior | ||
| /// to invoking `runApp`. Calling it too late, such as after the first frame has | ||
| /// been rendered or in response to a user action, may result in incorrect | ||
| /// rendering of the semantics tree. The crawler may already have crawled the | ||
| /// app and failed to find the data it needed to index it. | ||
| void enableCrawlerMode() { | ||
| assert( | ||
| !_isCrawlerModeEnabled, | ||
| '`enableCrawlerMode` was called more than once.', | ||
| ); | ||
| _isCrawlerModeEnabled = true; | ||
| } | ||
|
|
||
| /// Whether the crawler mode is enabled. | ||
| bool get isCrawlerModeEnabled => _isCrawlerModeEnabled; | ||
| bool _isCrawlerModeEnabled = false; |
There was a problem hiding this comment.
Let's try to enforce the no-late-calls rule:
| /// It is expected that this function is called early in the app's | |
| /// initialization, before the framework renders the semantics tree, e.g. prior | |
| /// to invoking `runApp`. Calling it too late, such as after the first frame has | |
| /// been rendered or in response to a user action, may result in incorrect | |
| /// rendering of the semantics tree. The crawler may already have crawled the | |
| /// app and failed to find the data it needed to index it. | |
| void enableCrawlerMode() { | |
| assert( | |
| !_isCrawlerModeEnabled, | |
| '`enableCrawlerMode` was called more than once.', | |
| ); | |
| _isCrawlerModeEnabled = true; | |
| } | |
| /// Whether the crawler mode is enabled. | |
| bool get isCrawlerModeEnabled => _isCrawlerModeEnabled; | |
| bool _isCrawlerModeEnabled = false; | |
| /// It is expected that this function is called early in the app's | |
| /// initialization, before the framework renders the semantics tree, e.g. prior | |
| /// to invoking `runApp`. Calling it too late, such as after the first frame has | |
| /// been rendered or in response to a user action, will result in a `StateError`. | |
| /// The crawler may already have crawled the app and failed to find the data | |
| /// it needed to index it. | |
| void enableCrawlerMode() { | |
| assert( | |
| _isCrawlerModeEnabled != null, | |
| '`enableCrawlerMode` was called more than once.', | |
| ); | |
| _isCrawlerModeEnabled = true; | |
| } | |
| /// Whether the crawler mode is enabled. | |
| bool get isCrawlerModeEnabled { | |
| // Make sure the value is set so it can't be overriden later. | |
| return _isCrawlerModeEnabled ??= false; | |
| } | |
| bool? _isCrawlerModeEnabled; |
There was a problem hiding this comment.
Initializing _isCrawlerModeEnabled in the getter seems wrong. It means that you cannot call enableCrawlerMode() after calling the getter, i.e. the getter has a side-effect on the initialization lifecycle. For example, the following would throw an error with an incorrect error message:
if (!isCrawlerModeEnabled) {
enableCrawlerMode(); // throws '`enableCrawlerMode` was called more than once.', which is wrong; it's called once
}
A better version of the getter might be:
bool get isCrawlerModeEnabled => isCrawlerModeEnabled ?? false;
However, now there's no difference between a nullable version and a non-nullable version. You either go from null to true, or from false to true.
There was a problem hiding this comment.
What I'm aiming at is making sure that after the engine reads the value of isCrawlModeEnabled, it shouldn't be overridable anymore. Doing it in the getter is easier, but I agree it's not ideal to have side effects in a getter.
Another way of doing it would be to have a 2nd bool isCrawlModeSet and a preventFurtherCrawlModeChanges(). We probably don't want to expose these through dart:ui_web. So you may have to move the _isCrawlModeEnabled into the private part of the engine, and expose it through dart:ui_web via a getter.
Given the extra complexity it would take to add this check, I'm not sure anymore if it's worth doing it.
mdebbar
left a comment
There was a problem hiding this comment.
LGTM and I like the LeafLabelRepresentation enum!
flutter/engine@cb9dead...6710d10 2024-03-13 yjbanov@google.com [web] make text more crawlable; fix a JAWS bug (flutter/engine#50794) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC bdero@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
On Windows, where JAWS is a popular screen reader and, unfortunately, ignores
aria-labelon empty elements, and for user agents that are known web crawlers (that also ignorearia-label), render semantic text into the DOM using an additional<span>element.Fixes flutter/flutter#122607