Bug 25523 – The text displayed by push buttons is not exposed to assistive technologies

• Component: Accessibility
• Status: VERIFIED FIXED

[Joanmarie Diggs]

Step to reproduce:

1. Examine the two push buttons in the (to be) attached test case using WebKit and Accerciser.

Expected result: The text displayed for each push button (i.e. "Clear" and "Submit") would be exposed to assistive technologies.

Actual result: The text displayed for each push button does not seem to be exposed at all to assistive technologies.

Recommendation: Please expose the text displayed by a push button as the accessible name of that button. Thanks!

[Joanmarie Diggs]

Created attachment 29965 [details]
aforementioned test case

[Xan Lopez]

(In reply to comment #0)
> Recommendation: Please expose the text displayed by a push button as the
> accessible name of that button. Thanks!
> 

Do you mean the accessible name from AtkObject or the accessible name from AtkAction?

[Joanmarie Diggs]

(In reply to comment #2)
> (In reply to comment #0)
> > Recommendation: Please expose the text displayed by a push button as the
> > accessible name of that button. Thanks!
> > 
> 
> Do you mean the accessible name from AtkObject or the accessible name from
> AtkAction?
> 
AtkObject. The AtkAction would be press or click or something like that.

[Mario Sanchez Prada]

Created attachment 32877 [details]
Patch proposal

Added new public method text to RenderButton and used from AccessibilityRenderObject::stringValue() to expose the 'name' property of AtkObject.

[Joanmarie Diggs]

(In reply to comment #4)
> Created an attachment (id=32877) [details]
> Patch proposal
> 
> Added new public method text to RenderButton and used from
> AccessibilityRenderObject::stringValue() to expose the 'name' property of
> AtkObject.

Tested with both Accerciser and Orca. That certainly solves the problem for me. Thanks!!

[Jan Alonzo]

Comment on attachment 32877 [details]
Patch proposal

> From 6a309cb208545a60ecf5281d93ef85d66ae4cb1a Mon Sep 17 00:00:00 2001
> From: Mario Sanchez Prada <[e-mail]>
> Date: Thu, 16 Jul 2009 15:38:26 +0200
> Subject: [PATCH] 2009-07-16  Mario Sanchez Prada <[e-mail]>
> MIME-Version: 1.0
> Content-Type: multipart/mixed; boundary="------------1.6.0.4"
> 
> This is a multi-part message in MIME format.
> --------------1.6.0.4
> Content-Type: text/plain; charset=UTF-8; format=fixed
> Content-Transfer-Encoding: 8bit
> 
> 
> Reviewed by NOBODY (OOPS!).
> 
> https://bugs.webkit.org/show_bug.cgi?id=25523
> [GTK] The text displayed by push buttons is not exposed to assistive technologies
> 
> Add new public method text to RenderButton and use it from
> AccessibilityRenderObject::stringValue().

Hi Mario. This patch requires a ChangeLog.

> +String RenderButton::text() const
> +{
> +    return m_buttonText ? m_buttonText->text() : 0;
> +}
> +

This can be inlined in RenderButton.h. And also, RenderButton::text() should return const String instead of just String.

[Mario Sanchez Prada]

(In reply to comment #6)
> Hi Mario. This patch requires a ChangeLog.

Ok, I'll send a patch with the ChangeLog modified, although I'm afraid I still could not fill myself the "Reviewed by <reviewer>" line, so I'll leave a placeholder there.

Or is there a better way to do this? (I'm new contributing to Webkit so any tip here would be appreciated)

> > +String RenderButton::text() const
> > +{
> > +    return m_buttonText ? m_buttonText->text() : 0;
> > +}
> > +
> 
> This can be inlined in RenderButton.h. And also, RenderButton::text() should
> return const String instead of just String.

I did it this way as that's how I've seen it's done in other places, such as RenderMenuList.[h|cpp]. Anyway, I've no problem in implementing it as an inline funcion in the .h file if that's ok.

I'll come up in some minutes with a new patch addressing these issues then.

[Jan Alonzo]

(In reply to comment #7)
> (In reply to comment #6)
> > Hi Mario. This patch requires a ChangeLog.
> 
> Ok, I'll send a patch with the ChangeLog modified, although I'm afraid I still
> could not fill myself the "Reviewed by <reviewer>" line, so I'll leave a
> placeholder there.
> 
> Or is there a better way to do this? (I'm new contributing to Webkit so any tip
> here would be appreciated)

Please use WebKitTools/Scripts/prepare-ChangeLog. There are also some notes in http://webkit.org/coding/contributing.html on how to contribute.

I would also suggest to make the patch description more verbose so other reviewers will notice that the patch applies to platform-independent code even though it's against the Gtk port.

Thanks.

[Mario Sanchez Prada]

(In reply to comment #8)
> [...]
> Please use WebKitTools/Scripts/prepare-ChangeLog. There are also some notes in
> http://webkit.org/coding/contributing.html on how to contribute.
> 
> I would also suggest to make the patch description more verbose so other
> reviewers will notice that the patch applies to platform-independent code even
> though it's against the Gtk port.

Thank. Just a quick question: when setting the 'review:?' flag, who should I put in the Requestee field? Should I put one specific WebKit reviewer (e.g. Xan Lopez) or should I leave it blank?

[Jan Alonzo]

(In reply to comment #9)
> (In reply to comment #8)
> > [...]
> > Please use WebKitTools/Scripts/prepare-ChangeLog. There are also some notes in
> > http://webkit.org/coding/contributing.html on how to contribute.
> > 
> > I would also suggest to make the patch description more verbose so other
> > reviewers will notice that the patch applies to platform-independent code even
> > though it's against the Gtk port.
> 
> Thank. Just a quick question: when setting the 'review:?' flag, who should I
> put in the Requestee field? Should I put one specific WebKit reviewer (e.g. Xan
> Lopez) or should I leave it blank?

We usually just leave it blank unless we really think that the patch needs to be reviewed by someone with the expertise in the area where code is being modified.

[Mario Sanchez Prada]

(In reply to comment #10)
> We usually just leave it blank unless we really think that the patch needs to
> be reviewed by someone with the expertise in the area where code is being
> modified.

Ok, I'll attach the patch then leaving tha field blank.

Thanks!

[Mario Sanchez Prada]

Created attachment 32932 [details]
Patch proposal

New version of the patch attached. Please notice it touches platform-independent code, even though it's motivated because of a WebKitGtk a11y bug.

(In reply to comment #7)
> > This can be inlined in RenderButton.h. And also, RenderButton::text() should
> > return const String instead of just String.
> 
> I did it this way as that's how I've seen it's done in other places, such as
> RenderMenuList.[h|cpp]. Anyway, I've no problem in implementing it as an inline
> funcion in the .h file if that's ok.

At last, I did not add the method as inline because its implementation relies on the private attribute 'RenderTextFragment* m_buttonText' (m_buttonText->text() is used) and that class is not available at compilation time when compiling the RenderButton.h file, so it fails to compile. Therefore, I've leave it as it was right now (same way than in RenderMenuList.[h|cpp]), just adding the 'const' modifier to the return value of the function.

[Mario Sanchez Prada]

Created attachment 32946 [details]
Add new public method text() to RenderButton and use it from
AccessibilityRenderObject::stringValue().

Fixed indentation issue in previous patch

[Jan Alonzo]

Removed [Gtk] as the code change is not Gtk-specific.

[Jan Alonzo]

Comment on attachment 32946 [details]
Add new public method text() to RenderButton and use it from
AccessibilityRenderObject::stringValue().

> -    
> +    const String text() const;
> +

I stand corrected and it should return a String not const String. The reason is because RenderText::text() doesn't return a const String (RenderMenuList return a String for RenderMenuList::text() as well, not const String).

r=me with changing it back to just String.

[Jan Alonzo]

(In reply to comment #15)
> (From update of attachment 32946 [details])
> > -    
> > +    const String text() const;
> > +
> 
> I stand corrected and it should return a String not const String. The reason is
> because RenderText::text() doesn't return a const String (RenderMenuList return
> a String for RenderMenuList::text() as well, not const String).
> 
> r=me with changing it back to just String.

Landed as http://trac.webkit.org/changeset/46080

[Joanmarie Diggs]

Verifying. Thanks!