Bug 253513 – AX: Consolidate the text() and stringValue() methods for static text, text control and password field objects.

• Component: Accessibility
• Status: RESOLVED FIXED

[Andres Gonzalez]

stringValue() should return text() for these types of objects.

[Radar WebKit Bug Importer]

<rdar://problem/106360128>

[Andres Gonzalez]

Created attachment 465339 [details]
Patch

[Tyler Wilcock]

Comment on attachment 465339 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=465339&action=review

> COMMIT_MESSAGE:7
> +The implementation of the text() and stringValue() for these types of objects is fragmented between the AccessibilityNodeObject and AccessibilityRenderObject classes. This patches eliminates some duplicated code and makes it easier to determine what these methods are returning for these roles.

Typo, "patches" should probably be "patch".

> COMMIT_MESSAGE:8
> +As a byproduct, the patch remove ariaRoleAttributed() from the AXCoreObject interface since it is not actually needed there. Some code cleanup.

Typos here.

As a byproduct, the patch remove ariaRoleAttributed()

Should maybe read:

As a byproduct, the patch removes ariaRoleAttribute()

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:2195
> +    if (isPasswordField())
> +        return passwordFieldValue();

I think you need to rebase — I changed this to be `isSecureField` and `secureFieldValue` in a recent patch.

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:2198
> +    // In the former case, go first for any alt text that may have been specified.

Nitpick on this wording -- feel free to take it or leave it.

// In the former case, go first for any alt text that may have been specified.
// In the former case, prefer any alt text that may have been specified.

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:2199
> +    // If no alt text present, fallback to the inline static text case where textUnderElement is used.

// If no alt text present
// If no alt text is present

[Andres Gonzalez]

Created attachment 465352 [details]
Patch

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #3)
> Comment on attachment 465339 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=465339&action=review
> 
> > COMMIT_MESSAGE:7
> > +The implementation of the text() and stringValue() for these types of objects is fragmented between the AccessibilityNodeObject and AccessibilityRenderObject classes. This patches eliminates some duplicated code and makes it easier to determine what these methods are returning for these roles.
> 
> Typo, "patches" should probably be "patch".
> 
> > COMMIT_MESSAGE:8
> > +As a byproduct, the patch remove ariaRoleAttributed() from the AXCoreObject interface since it is not actually needed there. Some code cleanup.
> 
> Typos here.
> 
> As a byproduct, the patch remove ariaRoleAttributed()
> 
> Should maybe read:
> 
> As a byproduct, the patch removes ariaRoleAttribute()
> 
> > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:2195
> > +    if (isPasswordField())
> > +        return passwordFieldValue();
> 
> I think you need to rebase — I changed this to be `isSecureField` and
> `secureFieldValue` in a recent patch.
> 
> > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:2198
> > +    // In the former case, go first for any alt text that may have been specified.
> 
> Nitpick on this wording -- feel free to take it or leave it.
> 
> // In the former case, go first for any alt text that may have been
> specified.
> // In the former case, prefer any alt text that may have been specified.
> 
> > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:2199
> > +    // If no alt text present, fallback to the inline static text case where textUnderElement is used.
> 
> // If no alt text present
> // If no alt text is present

All fixed. Sorry for my poor writing this morning.

[Andres Gonzalez]

Created attachment 465357 [details]
Patch

[chris fleizach]

Comment on attachment 465357 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=465357&action=review

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:-754
> -    if (isARIAStaticText()) {

do we lose any functionality by removing this block of text?

[Andres Gonzalez]

(In reply to chris fleizach from comment #7)
> Comment on attachment 465357 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=465357&action=review
> 
> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:-754
> > -    if (isARIAStaticText()) {
> 
> do we lose any functionality by removing this block of text?

No because:
- isARIAStaticText implies isStaticText
- if isStataticText then we call AccessibilityNodeObject::text which now do the right thing for both ARIA static text and actual static text.

[EWS]

Committed 261424@main (56bd520fb3a3): <https://commits.webkit.org/261424@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 465357 [details].