Bug 25413

Summary:

[GTK] Please expose the level of headings.

Product:

WebKit

Reporter:

Joanmarie Diggs <jdiggs>

Component:

Accessibility

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

apinheiro, commit-queue, walker.willie, xan.lopez

Priority:

Keywords:

Gtk

Version:

528+ (Nightly build)

Hardware:

OS:

Linux

Bug Depends on:

21546

Bug Blocks:

25531

Attachments:

Description	Flags
aforementioned test case	none
proposed patch - take 1	xan.lopez: review-
proposed patch - take 2	none

Joanmarie Diggs

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

Attachments
aforementioned test case (432 bytes, text/html) 2009-04-26 16:43 PDT, Joanmarie Diggs	no flags	Details
proposed patch - take 1 (2.96 KB, patch) 2009-10-17 03:02 PDT, Joanmarie Diggs	xan.lopez: review-	Details Formatted Diff Diff
proposed patch - take 2 (2.92 KB, patch) 2009-10-17 12:34 PDT, Joanmarie Diggs	no flags	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Joanmarie Diggs

Comment 1 2009-04-26 16:43:02 PDT

Created attachment 29806 [details] aforementioned test case

Joanmarie Diggs

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

Xan Lopez

Comment 3 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.

Joanmarie Diggs

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

Xan Lopez

Comment 5 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*> :)

Joanmarie Diggs

Comment 6 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.

Xan Lopez

Comment 7 2009-10-17 12:48:17 PDT

Comment on attachment 41362 [details] proposed patch - take 2 woot, r=me

WebKit Commit Bot

Comment 8 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>

WebKit Commit Bot

Comment 9 2009-10-17 13:00:46 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.