[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 #15 from Mario Sanchez Prada <msanchez at igalia.com>  2012-02-07 05:01:19 PST ---
(From update of attachment 125795)
View in context: https://bugs.webkit.org/attachment.cgi?id=125795&action=review

Awesome patch, Frederik. Just performing an informal review now.

See my (hopefully useful) comments below...

> Source/WebCore/accessibility/AXObjectCache.cpp:300
> +

I would not include this kind of fixes in this patch (unless they are in the scope of an actual change), since it alters the diff in an unrelated way.

If you feel like fixing this kind of things, it's better to file a bug for that.

> Source/WebCore/accessibility/AccessibilityObject.h:310
> +

Ditto.

> Source/WebCore/accessibility/AccessibilityObject.h:663
> +    void setWrapper(AccessibilityObjectWrapper* wrapper)

I probably would not change this either, but as it's so close to an actual change (#elif), I think it makes sense.

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

> Source/WebCore/accessibility/qt/AccessibilityObjectQt.cpp:40
> +

Unneeded extra empty line.

> Source/WebCore/accessibility/qt/AccessibilityObjectWrapperQt.cpp:30
> +*/

Wrong coding style. Place a '*' at the beginning of every line.

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

> Source/WebCore/accessibility/qt/AccessibilityObjectWrapperQt.cpp:255
> +

Unneeded empty line.

> Source/WebCore/accessibility/qt/AccessibilityObjectWrapperQt.cpp:339
> +    }

You should use early returns here to remove unneeded levels of indentation. Also, you should probably add some extra nullchecks, like checking if the document returned by m_object->document() is a valid one.

Something like this, more or less:

    [...]
    // Check if this is the root object. If so, we need to return the Frame as parent
    if (m_object->parentObjectUnignored() || !m_object->isScrollView())
        return 0;

    AccessibilityObject* firstChild = m_object->firstChild();
    if (!firstChild || !firstChild->isWebArea())
        return 0;

    // We seem to be the root accessible, get the Frame
    Document* document = m_object->document();
    if (!document)
        return 0;

    HostWindow* hostWindow = document->view()->hostWindow();
    if (!hostWindow)
        return 0;

    QWebPageClient* pageClient = hostWindow->platformPageClient();
    if (!pageClient || !pageClient->isQWidgetClient())
        return 0;

    // FIXME: Writing the same for QGraphicsView should be trivial.
    QScopedPointer<QAccessibleInterface> webView(QAccessible::queryAccessibleInterface(pageClient->ownerWidget()));
    QScopedPointer<QAccessibleInterface> webPage(webView->child(0));
    QAccessibleInterface* webFrame = webPage->child(0);
        return webFrame;

    [...]

Yeah, I know in the ATK wrapper there's atkParentOfRootObject(). We should change that there at some point too :-)

> Source/WebCore/accessibility/qt/AccessibilityObjectWrapperQt.cpp:343
> +// FXIME: Create a helper used by gtk and here

Typo (FXIME) + missing period at the end of the line.

> Source/WebCore/accessibility/qt/AccessibilityObjectWrapperQt.cpp:347
> +    case QAccessible::Name: {

Why are you using '{'/'}' in this 'case' entry?

> Source/WebCore/accessibility/qt/AccessibilityObjectWrapperQt.h:18
> +*/

Missing '*' at the beginning of every line.

> Source/WebKit/qt/Api/qwebpage.h:451
> +

Unneeded empty line?

> Source/WebKit/qt/Api/qwebview.cpp:173
> +// The QAccessible interfaces changed API in Qt 5

Missing period at the end of the line.

> Source/WebKit/qt/Api/qwebviewaccessible.cpp:18
> +*/

Missing '*' at the beginning of every line.

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

> Source/WebKit/qt/Api/qwebviewaccessible.cpp:97
> +    WebCore::AccessibilityObject* a = frame()->d->frame->document()->axObjectCache()->rootObject();

Same here.

> Source/WebKit/qt/Api/qwebviewaccessible_p.h:18
> +*/

Missing '*' at the beginning of every line.

> Source/WebKit/qt/Api/qwebviewaccessible_p.h:30
> +

This empty line is not needed.

> Source/WebKit/qt/Api/qwebviewaccessible_p.h:35
> +

Same here.

> Source/WebKit/qt/Api/qwebviewaccessible_p.h:62
> +

...and here :-)

> Source/WebKit/qt/Api/qwebviewaccessible_p.h:80
> +

...and here too

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