[webkit-reviews] review requested: [Bug 25679] [GTK] Improve accessibility of focusable lists : [Attachment 73250] Part 3 - emit 'selected' and 'focused' signals

[bugzilla-daemon at webkit.org]

Mario Sanchez Prada <msanchez at igalia.com> has asked  for review:
Bug 25679: [GTK] Improve accessibility of focusable lists
https://bugs.webkit.org/show_bug.cgi?id=25679

Attachment 73250: Part 3 - emit 'selected' and 'focused' signals
https://bugs.webkit.org/attachment.cgi?id=73250&action=review

------- Additional Comments from Mario Sanchez Prada <msanchez at igalia.com>
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.