[Webkit-unassigned] [Bug 25679] [GTK] Improve accessibility of focusable lists

[bugzilla-daemon at webkit.org]

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


Mario Sanchez Prada <msanchez at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #72945|0                           |1
        is obsolete|                            |
  Attachment #73250|                            |review?
               Flag|                            |




--- Comment #28 from Mario Sanchez Prada <msanchez at igalia.com>  2010-11-08 10:46:45 PST ---
Created an attachment (id=73250)
 --> (https://bugs.webkit.org/attachment.cgi?id=73250&action=review)
Part 3 - emit 'selected' and 'focused' signals

Thanks for the review, Chris. Now attaching a new patch. See my comments below...

(In reply to comment #27)
> (From update of attachment 72945 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=72945&action=review
> 
> > WebCore/ChangeLog:14
> > +        'focus-event', along with apropriate detail along with each of
> 
> spelling -> apropriate

Fixed.

> > WebCore/accessibility/gtk/AXObjectCacheAtk.cpp:55
> > +    AccessibilityRenderObject::AccessibilityChildrenVector items = object->children();
> 
> this should be AccessibilityObject::AXChildrenVector

Fixed.

> > WebCore/accessibility/gtk/AXObjectCacheAtk.cpp:56
> > +    SelectElement* select = toSelectElement(static_cast<Element*>(object->node()));
> 
> you should check if select == 0 after this line

Fixed.

> > WebCore/accessibility/gtk/AXObjectCacheAtk.cpp:58
> > +    if (changedItemIndex < 0 || changedItemIndex >= (int)items.size())
> 
> use static cast here

Fixed.

> > WebCore/accessibility/gtk/AXObjectCacheAtk.cpp:65
> > +    static RefPtr<AccessibilityObject> oldFocusedObject;
> 
> this static should be at the top of the method. 

Fixed.

> i also believe there's a standard way to initialize statics, that you should look for

I guess you mean initializating the var '= 0'. If so -> "Fixed!"

> > WebCore/accessibility/gtk/AXObjectCacheAtk.cpp:83
> > +        g_signal_emit_by_name(axItem, "focus-event", isSelected);
> 
> do you want to emit focus-event every time this happens? or do you want to limit it to when axItem != oldFocusedItem

Yes, that's the expected behavior. The axItem != oldFocusedItem check for the old item is meant to avoid sending the same signal twice in the hypothetical case they were the same object, but for the current one (axItem) we should definitely always emit those signals.

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