[Webkit-unassigned] [Bug 77864] [Qt] Initial implementation of accessibility support

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=77864





--- Comment #17 from Frederik Gladhorn <frederik.gladhorn at nokia.com>  2012-02-07 06:43:38 PST ---
>> Source/WebCore/accessibility/qt/AccessibilityObjectQt.cpp:37
>> +    return IncludeObject;
> 
> Are you sure you want to do this here? Normally, this function is meant to give flexibility to some ports that do not want to go ahead with the "default behavior" for some very specific cases, this falling back to DefaultBehavior if the AccessibilityObject does not match none of those very specific cases.
> 
> What you're saying here is that, basically, you want to expose *any* WebCore's Accessibility object in Qt, even those that should probably not be exposed (e.g. those returning 'true' in ariaIsHidden() and isPresentationalChildOfAriaRole()).
> 
> Maybe it's what you want, though. Pointing it out just in case, as it's normally not that way (if you check the mac port you'll see they also ignore some specific objects).

For now (debugging and getting this to run at all, I consciously include everything.
Filtering this in a sensible way is on my todo and I'll check the GTK implementation again.
Right now I feel that it's too much for one patch. This will come with implementing states and other interfaces which is also missing.

>> Source/WebCore/accessibility/qt/AccessibilityObjectWrapperQt.cpp:30
>> +*/
> 
> Wrong coding style. Place a '*' at the beginning of every line.

I don't see this in the style guide lines. I don't see the point of adding those stars... I can if it makes you happy though.

>> Source/WebCore/accessibility/qt/AccessibilityObjectWrapperQt.cpp:54
>> +    //     case AnnotationRole:
> 
> Only one space between the // and 'case'. Further commented lines should be the same way, respecting the indentation of course.

This is one of the next things to sort out anyway. I'd rather do it in a follow up commit though.

>> Source/WebCore/accessibility/qt/AccessibilityObjectWrapperQt.cpp:347
>> +    case QAccessible::Name: {
> 
> Why are you using '{'/'}' in this 'case' entry?

Because I declare String axTitle inside. If you don't have the parenthesis, you get "jump to case label/crosses initialization".

>> Source/WebKit/qt/Api/qwebviewaccessible.cpp:73
>> +    WebCore::AccessibilityObject* a = frame()->d->frame->document()->axObjectCache()->rootObject();
> 
> You probably want to add a couple of checks here: at least one for the result of frame->document(), and another for document->axObjectCache().
> 
> Also, 'a' is not a good name for a variable in WebKit. You should use something meaningful instead. Looks like 'rootObject', 'rootAccessibilityObject' or 'axRootObject' (this 'ax' prefix is kind of a convention in a11y related code) might work fine.
> 
> Last, I have no idea about QWebFrame, but I wonder if the 'frame()->d' statement could be replaced for something more understandable :)

d is just the pimpl, anyone knowing Qt knows what it's about.
I agree about the variable names and checks though.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.