Edge: fetch name of aria-alert text by traversing its children, which then allows live region changed event to work.#8472
Conversation
… then allows live region changed event to work. Re nvaccess#8466. When alerts are received, Edge fires live region changed event. However, the name of this element is empty, but thankfully, its children's names are text fragmenets. Therefore set name property by traversing its children, which then allows live region changed event to work.
LeonarddeR
left a comment
There was a problem hiding this comment.
Your current proposal creates a new python NVDAObject for every child and only deals with children at one level deep. I propose at least one of the following:
- Create a custom cache request to retrieve the children, similar to how this is done in the outlook app module.
- We're deealing with edge node objects, which have a tree interceptor. You can also retrieve the text by using obj.treeInterceptor.makeTextInfo(obj).text
Reviewed by @LeonarddeR (Babbage): because concatenation only works on first-level descendant, it may not pick up additional text from deeper descendants. Therefore use tree interceptor content as the element's name, which still allows live region changed event to work (thank you Leonard for this tip).
| def _get_name(self): | ||
| name=super(EdgeNode,self).name | ||
| # #8466: elements with aria-role=alert set fires live region changed event but does not expose the alert text as its name, wihch can be found in its descendants. | ||
| if "role" in self.ariaProperties and self.ariaProperties["role"] == "alert": |
There was a problem hiding this comment.
I think it might be good to check for the tree interceptor's existence, just to make sure. How about:
if self.ariaProperties.get("role") == "alert" and self.treeInterceptor:
|
Hi, ah yes, I thought about that for a second before making that change… Point taken, thanks.
@LeonarddeR commented on this pull request.
In source/NVDAObjects/UIA/edge.py <#8472 (comment)> :
@@ -422,6 +422,13 @@ def getNormalizedUIATextRangeFromElement(self,UIAElement):
charInfo.collapse(True)
return range
+ def _get_name(self):
+ name=super(EdgeNode,self).name
+ # #8466: elements with aria-role=alert set fires live region changed event but does not expose the alert text as its name, wihch can be found in its descendants.
+ if "role" in self.ariaProperties and self.ariaProperties["role"] == "alert":
I think it might be good to check for the tree interceptor's existence, just to make sure. How about:
if self.ariaProperties.get("role") == "alert" and self.treeInterceptor:
|
…#8466. Reviewed by @LeonarddeR (Babbage): just to make sure: tree interceptor must be live in order for tree interceptor based solution to work.
LeonarddeR
left a comment
There was a problem hiding this comment.
Note that review from @michaelDCurran is still required.
|
Hi, Besides resolving conflicts, I'll update what's new to include #8557, as that one is also identical to this one except that it deals with system alert event. Thanks. |
…rlying cause is the same.
|
Hi, To @LeonarddeR: requesting another round of review please. Thanks. |
michaelDCurran
left a comment
There was a problem hiding this comment.
I'm not comfortable with this approach. I think I would rather see code specific to liveRegionchange and SystemAlert events on this object, rather than overriding the name property. Reasons being:
- The object could already have an ARIA label or other author-provided name, which now gets lost.
- There could be a lot of text fetched in that object. In the past we have had problems with browsers that calculate name from descendants too much.
Requested by Mick Curran (NV Access): what if authors provide cusotm ARIA labels for the element in question? As of now, the name property does not take this into account. Instead, handlers for UIA system laert and live region change events will be defined with room for expansion when the need arises.
|
Hi, Right. For now, I'll define handlers for both of these, with NVDA announcing tree interceptor content (unless it has performance issues). This is so that improvements can be made as we encounter more alerts in the wild. Thanks for catching this flaw. |
|
In which cases do UIA_systemAlert or liveRegionChanged events apply to these alerts? Couldn't this lead to duplicated announcements somehow? |
|
|
||
| def event_liveRegionChange(self): | ||
| # #8466: elements with aria-role=alert set fires live region changed event but does not expose the alert text as its name, wihch can be found in its descendants. | ||
| if self.ariaProperties.get("role") == "alert" and self.treeInterceptor: |
There was a problem hiding this comment.
Wouldn't this make live region events go silent if this predicate is not True? I'd say you have to check that the object has no name as well. furthermore, if the predicate is False, please call the super method.
| def event_UIA_systemAlert(self): | ||
| # #8557: similar to live region change event, some elements does not have a name but text is scattered among its descendants. | ||
| if self.ariaProperties.get("role") == "alert" and self.treeInterceptor: | ||
| ui.message(self.treeInterceptor.makeTextInfo(self).text) |
There was a problem hiding this comment.
Same concerns apply here.
| return None | ||
|
|
||
| def event_liveRegionChange(self): | ||
| # #8466: elements with aria-role=alert set fires live region changed event but does not expose the alert text as its name, wihch can be found in its descendants. |
…stem alert. Caught by Leonard de Juijter (Babbage): live region change and system alert will fail if the alert is an aria alert, there is a name, and there is no tree interceptor. Thus catch this and call the superclass method.
|
@josephsl Plans to continue this pr? Probably not of very high importance due to current Edge being abandoned. |
|
Hi, probably unlikely unless progressive web apps require this PR. So let’s keep this aside for now. Thanks.
|
|
@feerrenrut maybe this is also worthy to mark as edge spartan for future reference, I cannot add labels to PRs. |
Link to issue number:
Resolves #8466
Summary of the issue:
In Edge, aria-role=alert isn't announcement properly due to the fact that there is no name for this element.
Description of how this pull request fixes the issue:
When alerts are received, Edge fires live region changed event. However, the name of this element is empty, but thankfully, its children's names are text fragments. Therefore set name property by traversing its children and combine them, which then allows live region changed event to work.
Testing performed:
Tested on Edge in Windows 10 Version 1803 and 1709.
Known issues with pull request:
None
Change log entry:
Bug fixes: In Microsoft Edge, NVDA will announce alerts marked with aria-role=alert property.