Bug 25413 – [GTK] Please expose the level of headings.

• Component: Accessibility
• Status: RESOLVED FIXED

[Joanmarie Diggs]

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!

[Joanmarie Diggs]

Created attachment 29806 [details]
aforementioned test case

[Joanmarie Diggs]

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!!

[Xan Lopez]

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.

[Joanmarie Diggs]

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. :-)

[Xan Lopez]

(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*> :)

[Joanmarie Diggs]

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.

[Xan Lopez]

Comment on attachment 41362 [details]
proposed patch - take 2

woot, r=me

[WebKit Commit Bot]

Comment on attachment 41362 [details]
proposed patch - take 2

Clearing flags on attachment: 41362

Committed r49743: <http://trac.webkit.org/changeset/49743>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.