Bug 25523 - The text displayed by push buttons is not exposed to assistive technologies
Summary: The text displayed by push buttons is not exposed to assistive technologies
Status: VERIFIED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks: 25531
  Show dependency treegraph
 
Reported: 2009-05-03 12:29 PDT by Joanmarie Diggs (irc: joanie)
Modified: 2009-07-27 14:16 PDT (History)
5 users (show)

See Also:


Attachments
aforementioned test case (607 bytes, text/html)
2009-05-03 12:30 PDT, Joanmarie Diggs (irc: joanie)
no flags Details
Patch proposal (2.89 KB, patch)
2009-07-16 09:18 PDT, Mario Sanchez Prada
no flags Details | Formatted Diff | Diff
Patch proposal (3.30 KB, patch)
2009-07-17 05:23 PDT, Mario Sanchez Prada
no flags Details | Formatted Diff | Diff
Add new public method text() to RenderButton and use it from AccessibilityRenderObject::stringValue(). (3.33 KB, patch)
2009-07-17 09:22 PDT, Mario Sanchez Prada
jmalonzo: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joanmarie Diggs (irc: joanie) 2009-05-03 12:29:40 PDT
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!
Comment 1 Joanmarie Diggs (irc: joanie) 2009-05-03 12:30:50 PDT
Created attachment 29965 [details]
aforementioned test case
Comment 2 Xan Lopez 2009-06-17 06:51:19 PDT
(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?
Comment 3 Joanmarie Diggs (irc: joanie) 2009-06-17 08:33:22 PDT
(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.
Comment 4 Mario Sanchez Prada 2009-07-16 09:18:36 PDT
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.
Comment 5 Joanmarie Diggs (irc: joanie) 2009-07-16 11:18:25 PDT
(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!!
Comment 6 Jan Alonzo 2009-07-17 02:04:47 PDT
Comment on attachment 32877 [details]
Patch proposal

> From 6a309cb208545a60ecf5281d93ef85d66ae4cb1a Mon Sep 17 00:00:00 2001
> From: Mario Sanchez Prada <msanchez@igalia.com>
> Date: Thu, 16 Jul 2009 15:38:26 +0200
> Subject: [PATCH] 2009-07-16  Mario Sanchez Prada <msanchez@igalia.com>
> 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.
Comment 7 Mario Sanchez Prada 2009-07-17 02:57:31 PDT
(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.
Comment 8 Jan Alonzo 2009-07-17 03:22:29 PDT
(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.
Comment 9 Mario Sanchez Prada 2009-07-17 04:09:43 PDT
(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?
Comment 10 Jan Alonzo 2009-07-17 05:06:21 PDT
(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.
Comment 11 Mario Sanchez Prada 2009-07-17 05:14:51 PDT
(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!
Comment 12 Mario Sanchez Prada 2009-07-17 05:23:53 PDT
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.
Comment 13 Mario Sanchez Prada 2009-07-17 09:22:14 PDT
Created attachment 32946 [details]
Add new public method text() to RenderButton and use it from
AccessibilityRenderObject::stringValue().

Fixed indentation issue in previous patch
Comment 14 Jan Alonzo 2009-07-17 14:05:45 PDT
Removed [Gtk] as the code change is not Gtk-specific.
Comment 15 Jan Alonzo 2009-07-17 19:48:15 PDT
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.
Comment 16 Jan Alonzo 2009-07-17 20:03:52 PDT
(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
Comment 17 Joanmarie Diggs (irc: joanie) 2009-07-27 14:16:09 PDT
Verifying. Thanks!