Bug 129039 - [ATK] Wrong selected element at a given index in a list box.
Summary: [ATK] Wrong selected element at a given index in a list box.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Krzysztof Czech
URL:
Keywords: InRadar
Depends on: 129970
Blocks:
  Show dependency treegraph
 
Reported: 2014-02-19 05:16 PST by Krzysztof Czech
Modified: 2016-11-01 10:01 PDT (History)
11 users (show)

See Also:


Attachments
patch (19.03 KB, patch)
2014-02-19 05:36 PST, Krzysztof Czech
no flags Details | Formatted Diff | Diff
patch (19.05 KB, patch)
2014-02-19 05:44 PST, Krzysztof Czech
no flags Details | Formatted Diff | Diff
patch (22.05 KB, patch)
2014-02-20 03:33 PST, Krzysztof Czech
no flags Details | Formatted Diff | Diff
patch - fixed win build (22.83 KB, patch)
2014-02-20 06:21 PST, Krzysztof Czech
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Krzysztof Czech 2014-02-19 05:16:07 PST
The selected element at a given index was wrong. Current implementation tried to get one from collection of all selected elements instead of collection of real children of a listbox.
Comment 1 Radar WebKit Bug Importer 2014-02-19 05:16:21 PST
<rdar://problem/16108701>
Comment 2 Krzysztof Czech 2014-02-19 05:36:41 PST
Created attachment 224626 [details]
patch
Comment 3 Krzysztof Czech 2014-02-19 05:44:15 PST
Created attachment 224628 [details]
patch
Comment 4 chris fleizach 2014-02-19 09:08:10 PST
Comment on attachment 224628 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=224628&action=review

> Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceSelection.cpp:113
> +            return 0;

return nullptr

> Tools/DumpRenderTree/AccessibilityUIElement.cpp:1224
> +        indexNumber = JSValueToNumber(context, arguments[0], exception);

you might want to return early after you set indexNumber if it still equals -1, because once you cast to unsigned in removeSelectionChild, it will make it a UINT_MAX

> Tools/DumpRenderTree/AccessibilityUIElement.h:152
> +    void removeSelectionChildAtIndex(unsigned) const;

this should probably just be removeSelectionAtIndex. I feel like the "selectionChild" sounds strange in a "remove" context
Comment 5 Krzysztof Czech 2014-02-20 03:33:35 PST
Created attachment 224742 [details]
patch
Comment 6 Krzysztof Czech 2014-02-20 03:34:48 PST
> > Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceSelection.cpp:113
> > +            return 0;
> 
> return nullptr
Done, corrected other places as well.
> 
> > Tools/DumpRenderTree/AccessibilityUIElement.cpp:1224
> > +        indexNumber = JSValueToNumber(context, arguments[0], exception);
> 
> you might want to return early after you set indexNumber if it still equals -1, because once you cast to unsigned in removeSelectionChild, it will make it a UINT_MAX
Done.
> 
> > Tools/DumpRenderTree/AccessibilityUIElement.h:152
> > +    void removeSelectionChildAtIndex(unsigned) const;
> 
> this should probably just be removeSelectionAtIndex. I feel like the "selectionChild" sounds strange in a "remove" context
Done.

Thanks.
Comment 7 Krzysztof Czech 2014-02-20 06:21:39 PST
Created attachment 224752 [details]
patch - fixed win build
Comment 8 WebKit Commit Bot 2014-02-24 01:25:48 PST
Comment on attachment 224752 [details]
patch - fixed win build

Clearing flags on attachment: 224752

Committed r164577: <http://trac.webkit.org/changeset/164577>
Comment 9 WebKit Commit Bot 2014-02-24 01:25:51 PST
All reviewed patches have been landed.  Closing bug.
Comment 10 Daniel Bates 2014-02-24 11:15:16 PST
(In reply to comment #8)
> (From update of attachment 224752 [details])
> Clearing flags on attachment: 224752
> 
> Committed r164577: <http://trac.webkit.org/changeset/164577>

This broke the iOS DumpRenderTree build because AccessibilityUIElementMac.mm is excluded when building DumpRenderTree and neither AccessibilityUIElement::selectedChildAtIndex() nor AccessibilityUIElement::selectedChildrenCount() are defined in AccessibilityUIElementIOS.mm.

Build fix committed in <http://trac.webkit.org/changeset/164595>.
Comment 11 Joanmarie Diggs (irc: joanie) 2016-11-01 10:01:09 PDT
(In reply to comment #0)
> The selected element at a given index was wrong. Current implementation
> tried to get one from collection of all selected elements instead of
> collection of real children of a listbox.

A git blame led me here.

According to the documentation [1] atk_selection_ref_selection() takes "a gint specifying the index in the selection set. (e.g. the ith selection as opposed to the ith child)."

[1] https://developer.gnome.org/atk/stable/AtkSelection.html#atk-selection-ref-selection