Handle Firefox annotation with unknown role, simplify datastructures#14526
Conversation
|
cc @nvdaes, who reported this privately |
michaelDCurran
left a comment
There was a problem hiding this comment.
Changing the raise to return None looks good.
But I notice that you have changed typing of all the properties from iterator to Generator. This may more correctly reflect the truth, but it does then raise (most likely an existing issue) that returning generators from cachable properties may be problematic. If the property is called once then a new generator is fetched, and code may iterate over some or all of it. But if the property is again fetched within the same core cycle, that same generator will be returned, which has already be partially or fully exhausted. these properties really should only return lists or tuples...
See test results for failed build of commit cea73e5296 |
The code which handles all these properties ensures to consider this by turning them into tuples where appropriate. |
See test results for failed build of commit 4dbd937f36 |
Link to issue number:
Follow up of #14507, #14480
Summary of the issue:
Firefox incorrectly throws an error when browsing annotated content with an unsupported role.
Instead an unknown role should be reported: i.e. "has details".
The typing for annotation data structures were incorrect.
Description of user facing changes
When browsing content in Firefox with an annotation with unsupported role, "has details" is reported instead of throwing an error.
Description of development approach
Update typing to be more accurate for annotation data structures, log and return None in the Firefox case to match Chromium behaviour.
Use Tuples rather than generators for
annotations.targetsandannotations.roles.Drops the superfluous/unused
annotations.summariesTesting strategy:
Known issues with pull request:
Change log entries:
N/A unreleased bug
Code Review Checklist: