Bug 25413 - [GTK] Please expose the level of headings.
Summary: [GTK] Please expose the level of headings.
Status: RESOLVED 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: 21546
Blocks: 25531
  Show dependency treegraph
 
Reported: 2009-04-26 16:41 PDT by Joanmarie Diggs (irc: joanie)
Modified: 2009-10-17 13:00 PDT (History)
4 users (show)

See Also:


Attachments
aforementioned test case (432 bytes, text/html)
2009-04-26 16:43 PDT, Joanmarie Diggs (irc: joanie)
no flags Details
proposed patch - take 1 (2.96 KB, patch)
2009-10-17 03:02 PDT, Joanmarie Diggs (irc: joanie)
xan.lopez: review-
Details | Formatted Diff | Diff
proposed patch - take 2 (2.92 KB, patch)
2009-10-17 12:34 PDT, Joanmarie Diggs (irc: joanie)
no flags 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-04-26 16:41:51 PDT
Steps to reproduce:

1. View the (to be) attached document in WebKit Gtk.

2. Examine any of the headings using Accerciser's Interface Viewer.

Expected results: There would be some way to identify the level of the heading.

Actual results: (As best as I can tell) the level of the heading is not being exposed to assistive technologies.

Comments:

1. The reason why it is helpful to have this information is because it enables the user to quickly scan through and/or navigate within a document in which headings have been properly applied.

2. FWIW, the way that Gecko exposes this information is via an object attribute (e.g. 'level:2'). That doesn't mean WebKit should follow suit if y'all have a better idea; merely pointing out this as an option. :-)

Thanks!
Comment 1 Joanmarie Diggs (irc: joanie) 2009-04-26 16:43:02 PDT
Created attachment 29806 [details]
aforementioned test case
Comment 2 Joanmarie Diggs (irc: joanie) 2009-10-17 03:02:24 PDT
Created attachment 41355 [details]
proposed patch - take 1

I decided to take a stab at an easier one. :-)

This works for me. Xan, please review and give pointers. Thanks in advance!!
Comment 3 Xan Lopez 2009-10-17 03:22:37 PDT
Comment on attachment 41355 [details]
proposed patch - take 1

This patch is amazing, great job!

A few small comments:

> +static AtkAttributeSet* addAttributeToSet(AtkAttributeSet* attributeSet, const char* name, const char* value)
> +{
> +    AtkAttribute* attribute = (AtkAttribute*)g_malloc(sizeof(AtkAttribute));

WebKit style guide tells us to use C++ style castings, so you need to do something like:

AtkAttribute* attribute = static_cast<AtkAttribute>(g_malloc(sizeof(AtkAttribute)));


> +    attribute->name = g_strdup(name);
> +    attribute->value = g_strdup(value);
> +    attributeSet = g_slist_prepend(attributeSet, attribute);
> +
> +    return attributeSet;
> +}
> +
> +static AtkAttributeSet* webkit_accessible_get_attributes(AtkObject* object)
> +{
> +    AtkAttributeSet* attributeSet = NULL;
> +
> +    int headingLevel = core(object)->headingLevel();
> +    if (headingLevel) {
> +        String value = String::number(headingLevel);
> +        attributeSet = addAttributeToSet(attributeSet, (const char*)"level", returnString(value));

- Is the (const char*) casting for "level" really needed? I think it should be const char* already.
- Using returnString() here is a tiny hack, since the function was really intended for return values in public APIs implementations. Do thinks work if you pass value.utf8().data() to the function?

I'm marking it as r- while we work on those final details.
Comment 4 Joanmarie Diggs (irc: joanie) 2009-10-17 03:56:23 PDT
Thanks for the feedback, Xan!

Notes (mostly to myself so that I remember them after I get some sleep. :-) )

> WebKit style guide tells us to use C++ style castings, so you need to do
> something like:
> 
> AtkAttribute* attribute =
> static_cast<AtkAttribute>(g_malloc(sizeof(AtkAttribute)));

When I try that I get an error:

~~~~~~
WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp: In function ‘AtkAttributeSet* addAttributeToSet(AtkAttributeSet*, const char*, const char*)’:
WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:179: error: no matching function for call to ‘_AtkAttribute::_AtkAttribute(void*)’
~~~~~~

But I'm new to C++. I bet if I research it and think about it, it'll all become clear. I'll do that "tomorrow."

> - Is the (const char*) casting for "level" really needed? I think it should be
> const char* already.

You are correct. My bad.

> - Using returnString() here is a tiny hack, since the function was really
> intended for return values in public APIs implementations. Do thinks work if
> you pass value.utf8().data() to the function?

They do indeed. Cool.

> I'm marking it as r- while we work on those final details.

Thanks much for the review! I'll look into the first issue/error after some sleep. :-)
Comment 5 Xan Lopez 2009-10-17 10:47:59 PDT
(In reply to comment #4)
> Thanks for the feedback, Xan!
> 
> Notes (mostly to myself so that I remember them after I get some sleep. :-) )
> 
> > WebKit style guide tells us to use C++ style castings, so you need to do
> > something like:
> > 
> > AtkAttribute* attribute =
> > static_cast<AtkAttribute>(g_malloc(sizeof(AtkAttribute)));
> 
> When I try that I get an error:
> 
> ~~~~~~
> WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp: In function
> ‘AtkAttributeSet* addAttributeToSet(AtkAttributeSet*, const char*, const
> char*)’:
> WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:179: error: no
> matching function for call to ‘_AtkAttribute::_AtkAttribute(void*)’
> ~~~~~~
> 
> But I'm new to C++. I bet if I research it and think about it, it'll all become
> clear. I'll do that "tomorrow."

I'm sorry, there was a missing '*' in the static_cast, it should read static_cast<AtkAttribute*> :)
Comment 6 Joanmarie Diggs (irc: joanie) 2009-10-17 12:34:21 PDT
Created attachment 41362 [details]
proposed patch - take 2

(In reply to comment #5)
> I'm sorry, there was a missing '*' in the static_cast, it should read
> static_cast<AtkAttribute*> :)

Heheh. Even *I* see that NOW. :-) I really, really needed to sleep. But if I didn't see it, I would have been embarrassed when that turned out to be the "issue," so thanks for providing it. :-)

This version makes that change along with the two others you pointed out.
Comment 7 Xan Lopez 2009-10-17 12:48:17 PDT
Comment on attachment 41362 [details]
proposed patch - take 2

woot, r=me
Comment 8 WebKit Commit Bot 2009-10-17 13:00:42 PDT
Comment on attachment 41362 [details]
proposed patch - take 2

Clearing flags on attachment: 41362

Committed r49743: <http://trac.webkit.org/changeset/49743>
Comment 9 WebKit Commit Bot 2009-10-17 13:00:46 PDT
All reviewed patches have been landed.  Closing bug.