Gecko: Report label in browse mode if it isn't rendered anywhere else #7513
Merged
Conversation
…a node if the name was set explicitly, the name was not used for the content, and the name did not come from another visible label.
feerrenrut
suggested changes
Aug 29, 2017
|
|
||
| const bool isImgMap = role == ROLE_SYSTEM_GRAPHIC && childCount > 0; | ||
| // Whether the name of this node has been explicitly set (as opposed to calculated by descendant) | ||
| const bool nameIsExplicit = ((IA2AttribsMapIt = IA2AttribsMap.find(L"explicit-name")) != IA2AttribsMap.end() && IA2AttribsMapIt->second == L"true"); |
Contributor
There was a problem hiding this comment.
I think this would be clearer as two statements rather than nested parans. Eg:
IA2AttribsMapIt = IA2AttribsMap.find(L"explicit-name")
// Whether the name of this node has been explicitly set (as opposed to calculated by descendant)
const bool nameIsExplicit = IA2AttribsMapIt != IA2AttribsMap.end() && IA2AttribsMapIt->second == L"true";
Also note that the outside parans are not necessary.
| || ((role == ROLE_SYSTEM_CHECKBUTTON || role == ROLE_SYSTEM_RADIOBUTTON) && !isLabelVisible(pacc)); | ||
|
|
||
| || role == ROLE_SYSTEM_LINK || role == ROLE_SYSTEM_PUSHBUTTON || role == IA2_ROLE_TOGGLE_BUTTON || role == ROLE_SYSTEM_MENUITEM || (role == ROLE_SYSTEM_GRAPHIC && !isImgMap) || (role == ROLE_SYSTEM_TEXT && !isEditable) || role == IA2_ROLE_HEADING || role == ROLE_SYSTEM_PAGETAB || role == ROLE_SYSTEM_BUTTONMENU; | ||
| // || ((role == ROLE_SYSTEM_CHECKBUTTON || role == ROLE_SYSTEM_RADIOBUTTON) && !labelVisible); |
Contributor
There was a problem hiding this comment.
should this be commented out?
| || role == ROLE_SYSTEM_LINK || role == ROLE_SYSTEM_PUSHBUTTON || role == IA2_ROLE_TOGGLE_BUTTON || role == ROLE_SYSTEM_MENUITEM || (role == ROLE_SYSTEM_GRAPHIC && !isImgMap) || (role == ROLE_SYSTEM_TEXT && !isEditable) || role == IA2_ROLE_HEADING || role == ROLE_SYSTEM_PAGETAB || role == ROLE_SYSTEM_BUTTONMENU | ||
| || ((role == ROLE_SYSTEM_CHECKBUTTON || role == ROLE_SYSTEM_RADIOBUTTON) && !isLabelVisible(pacc)); | ||
|
|
||
| || role == ROLE_SYSTEM_LINK || role == ROLE_SYSTEM_PUSHBUTTON || role == IA2_ROLE_TOGGLE_BUTTON || role == ROLE_SYSTEM_MENUITEM || (role == ROLE_SYSTEM_GRAPHIC && !isImgMap) || (role == ROLE_SYSTEM_TEXT && !isEditable) || role == IA2_ROLE_HEADING || role == ROLE_SYSTEM_PAGETAB || role == ROLE_SYSTEM_BUTTONMENU; |
Contributor
There was a problem hiding this comment.
my preference would be to put each of these or conditions onto their own line. This line is quite long at the moment!
| // || ((role == ROLE_SYSTEM_CHECKBUTTON || role == ROLE_SYSTEM_RADIOBUTTON) && !labelVisible); | ||
| // Whether this node has a visible label somewhere else in the tree | ||
| const bool labelVisible = canDetectLabelVisibility // Not all browsers support getting a node's labelledBy node | ||
| &&nameIsExplicit&&name&&name[0] //this node must actually have an explicit name |
Contributor
There was a problem hiding this comment.
Could you put some spaces in here?
Why name[0]? I assume the intent is to check that this is not a zero length string? If so, could you make it clearer that that is the intent of this. It would be handy if there was a isNullOrEmpty function for BSTR*
feerrenrut
approved these changes
Aug 30, 2017
michaelDCurran
added a commit
that referenced
this pull request
Aug 30, 2017
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Link to issue number:
Partial fix for #4773
Summary of the issue:
Sometimes a web author will use aria-label on a node to convey screen-reader specific information, however the node also has useful and valid content itself (E.g. an edit field, or an iFrame).
NVDA already reports this label when moving to these controls with quick navigation or tabbing, but it should also report it when reading the control with the browse mode caret, as this label will not be rendered anywhere else.
Of course care must be taken to ensure this is in deed a label set by aria-label and not say a label from aria-labelledby or an associated html label tag. The one exception to this is if the associated label node is hidden from the screen.
Description of how this pull request fixes the issue:
In Firefox and other Mozilla Gecko products, we now convey to NVDA that these labels must always be reported, by exposing the 'alwaysReportName' attribute on these nodes from the vbufbackend.
Testing performed:
Tested browsers by arrowing up and down the page with NVDA's screen layout mode off, using the document from
sr_only_labels.html.txt
Known issues with pull request:
Note that it is not supported in other browsers because:
Change log entry:
Bug fixes:
Accessible labels for controls in Mozilla Firefox are now more readily reported in browse mode when the label does not appear as content itself. (#4773)