| Summary: | [GTK] Implement ROLE_COMBO_BOX | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Joanmarie Diggs <jdiggs> | ||||||
| Component: | Accessibility | Assignee: | Nobody <webkit-unassigned> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | apinheiro, cfleizach, gustavo, mario, mrobinson, walker.willie, xan.lopez | ||||||
| Priority: | P2 | Keywords: | Gtk | ||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | PC | ||||||||
| OS: | Linux | ||||||||
| Bug Depends on: | 50022, 50062 | ||||||||
| Bug Blocks: | 25531, 36146 | ||||||||
| Attachments: |
|
||||||||
|
Description
Joanmarie Diggs
2009-05-10 17:16:08 PDT
I started hacking at this over the weekend, and made quite a bit of progress. But it turns out to be somewhat more involved than I would have originally thought. So before I continue and start proposing patches, here's the deal...
Given this:
<body>
<select id="component" name="component" size="1">
<option value="Accessibility">Accessibility</option>
<option value="CSS">CSS</option>
<option selected value="Evangelism">Evangelism</option>
<option value="Forms">Forms</option>
</select>
</body>
DRT gives us this:
layer at (0,0) size 118x83
RenderView at (0,0) size 62x83
layer at (0,0) size 118x83
RenderBlock {HTML} at (0,0) size 62x83
RenderBody {BODY} at (8,8) size 46x67
RenderMenuList {SELECT} at (2,2) size 106x28 [bgcolor=#FFFFFF]
RenderBlock (anonymous) at (4,4) size 81x20
RenderText at (0,0) size 74x20
text run at (0,0) width 74: "Evangelism"
RenderText {#text} at (0,0) size 0x0
RenderText {#text} at (0,0) size 0x0
RenderText {#text} at (0,0) size 0x0
We expect this:
-> Document Frame
-> Evangelism (Combo Box)
-> Evangelism (Menu)
-> Accessibility (Menu Item)
-> CSS (Menu Item)
-> Evangelism (Menu Item)
-> Forms (Menu Item)
Associated problems:
1. Hierarchy's in the wrong order w.r.t. options. We need to either
swap the render objects associated with the impacted accessibles
or render the objects in the expected order somehow.
2. No *easy/immediate* way (that I've found) to go from what seems
to be a stub RenderText object to the associated String. (I am
able to re-create things via SelectElement and OptionElement...)
In other words, WebKitGtk's rendering w.r.t. HTML combo boxes doesn't mesh all that well with what we need/expect.
In addition: VoiceOver seems to deal with this object in its collapsed form strictly as a pop up button rather than as a combo box. So what other a11y platforms need/expect doesn't mesh all that well with what we need/expect.
I am managing to hack around these things, but it's starting to get ugly. Any brilliant insights would welcome. Otherwise, I'll try to get some patches up before too long which more concretely illustrate the issues.
Thanks!
Note to self: the fix just committed for bug 33773 might help solve this issue for us. (in which case, yea!) Hi Joanie, I've just started working on this one recently, and I have a couple of doubts about it. See below... (In reply to comment #0) > [...] > 1. The role should be ROLE_COMBO_BOX (currently WebKit is exposing combo boxes as > ROLE_UNKNOWN). Seems to be already fixed. > 2. The selectable items in the combo box should be exposed somehow. Both Gecko and Gtk+ > combo boxes do this by giving the accessible combo box a single child of ROLE_MENU. The > menu, in turn, has children of ROLE_MENU_ITEM, one for each selectable option in the combo box. Already fixed. > 3. AT's should be able to ask for the selected item. You could do this via the accessible name > (which seems to have already been done). But ideally the accessible selection interface will also be > implemented as that seems to be more reliable. Both Gecko and Gtk+ do this for the menu which > is the child of the combo box. Please see http://library.gnome.org/devel/atk/unstable/AtkSelection.html > for more information. In the patch I currently got, I've implemented the AtkSelection interface in the topmost object in that hierarchy, that is the one with role ATK_ROLE_COMBO_BOX (which has a child of role ATK_ROLE_MENU holding the items, with role ATK_ROLE_MENU_ITEM). This allows selecting, unselecting and asking for the selected item right from that topmost object (the combo box). Is it the expected behaviour? (asking this since I've seen different situations in Gecko and Gtk+, but because of the internals of WebCore this option seems to be the best one to me. But I can be wrong) > 4. The accessible stateset should include STATE_EXPANDABLE -- and STATE_EXPANDED when > appropriate. I didn't find a not hackish way yet to do this without hardcoding. My current solution is the following: [...] @@ -500,6 +501,12 @@ static void setAtkStateSetFromCoreObject(AccessibilityObject* coreObject, AtkSta atk_state_set_add_state(stateSet, ATK_STATE_SENSITIVE); } + if (coreObject->canSetExpandedAttribute()) + atk_state_set_add_state(stateSet, ATK_STATE_EXPANDABLE); + + if (coreObject->isExpanded()) + atk_state_set_add_state(stateSet, ATK_STATE_EXPANDED); + if (coreObject->canSetFocusAttribute()) atk_state_set_add_state(stateSet, ATK_STATE_FOCUSABLE); [...] ... but not sure whether this would be enough. > 5. The accessible action interface should be implemented so that ATs can interact with the combo box > (i.e. expand/collapse it). Gecko implements an "open" action; Gtk+ a "press" action. (Please see > http://library.gnome.org/devel/atk/unstable/AtkAction.html for more info.) This works out-of-the-box :-), because the implementation of the AtkAction interface just relays on performing the default action for the given coreObject in WebCore... so it's as easy as implement the interface and let WebCore either to do something or to return False in case nothing happened. But in this specific case, I can clearly open/close the combo box, selecting items... and so on. > 6. Appropriate events should be emitted when the selection changes. Gecko emits an > object:state-changed:focused event for the menu item; Gtk+ emits an object:selection-changed > event for the combo box. Which one would be the right event then to be issued and from where (assuming the combo box object right now)? (In reply to comment #1) > [...] > We expect this: > > -> Document Frame > -> Evangelism (Combo Box) > -> Evangelism (Menu) > -> Accessibility (Menu Item) > -> CSS (Menu Item) > -> Evangelism (Menu Item) > -> Forms (Menu Item) As far as I can see in Accerciser, that's exactly what I was geting before starting to work on this. Perhaps I'm misunderstanding things as the result of this description being already almost one year old :-) Could you confirm that this 'expected' output is nowadays the actual one? > Associated problems: > > 1. Hierarchy's in the wrong order w.r.t. options. We need to either > swap the render objects associated with the impacted accessibles > or render the objects in the expected order somehow. I don't understand properly what needs to be swapped here... perhaps another 'already fixed' thing? :-) > 2. No *easy/immediate* way (that I've found) to go from what seems > to be a stub RenderText object to the associated String. (I am > able to re-create things via SelectElement and OptionElement...) In my current patch, that's no longer an issue ;-): you just can retrieve the text through the implementation of the AtkText interface for each menu item (could you confirm this is correct? I just followed gecko here). > I am managing to hack around these things, but it's starting to get ugly. Any brilliant insights would > welcome. Otherwise, I'll try to get some patches up before too long which more concretely illustrate > the issues. Nothing brilliant yet, just some messy and experimental code at the moment (so that's why I'm not attaching anything yet), but still I'm on my way, and I hope that with some more time to finish this, some awesome help from your side (when possible, I don't want to put any pressure on you) answering the doubts on this comment and a final stage of polishing and writing unit tests could help me making it to a nice patch :-) Thanks in advance and sorry for the long comment! Created attachment 74116 [details] Patch proposal + Unit test After my long dissertation in comment #3 (sorry again!) I've came across a process of carefully checking what GTK and gecko does, making some assumptions here and there and, after all that work I finally got what looks to me it could be a complete patch to fix ts issue (new unit test included). However, I'm more than open to make major changes to the patch in case someone (Joanie?) thought some parts are not properly implemented from the POV of ATK-based AT's. I've tried to follow what GTK does and I think everything should be covered, but you never know... so that's it, just trying to help moving this thing forward. Hence, asking for review. Comment on attachment 74116 [details] Patch proposal + Unit test View in context: https://bugs.webkit.org/attachment.cgi?id=74116&action=review Nice patch. I have a couple questions... > WebCore/ChangeLog:23 > + of that methor to an overriden version of stringValue(), which is methor -> method > WebCore/accessibility/AccessibilityMenuListOption.cpp:113 > +String AccessibilityMenuListOption::stringValue() const I'm not sure about the naming here. Maybe Chris can give the nod. > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:768 > + const Vector<Element*> listItems = selectNode->listItems(); > > - // TODO: Combo boxes > + if (selectedIndex < 0 || selectedIndex >= (int)listItems.size()) Is it possible for the selectedIndex to be > listeItems.size()? It doesn't look like it after staring at SelectElement::selectedIndex. If you still need to cast to int, be sure to use a static_cast. > WebCore/platform/gtk/PopupMenuGtk.cpp:65 > - int x, y; > - gdk_window_get_origin(gtk_widget_get_window(GTK_WIDGET(view->hostWindow()->platformPageClient())), &x, &y); > + int x = 0; > + int y = 0; > + GdkWindow* window = gtk_widget_get_window(GTK_WIDGET(view->hostWindow()->platformPageClient())); > + if (window) > + gdk_window_get_origin(window, &x, &y); Is this change related? Created attachment 74127 [details] Patch proposal + Unit test (In reply to comment #5) > [...] > > WebCore/ChangeLog:23 > > + of that methor to an overriden version of stringValue(), which is > > methor -> method Ooops! Fixed. > > WebCore/accessibility/AccessibilityMenuListOption.cpp:113 > > +String AccessibilityMenuListOption::stringValue() const > > I'm not sure about the naming here. Maybe Chris can give the nod. Well, the naming must be exactly that one ('stringValue') since what I want to do is precisely to override the stringValue() method from the superclass AccessibilityObject, so no more black magic is needed and everything works without changing anything else :-) > > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:768 > > + const Vector<Element*> listItems = selectNode->listItems(); > > > > - // TODO: Combo boxes > > + if (selectedIndex < 0 || selectedIndex >= (int)listItems.size()) > > Is it possible for the selectedIndex to be > listeItems.size()? It doesn't look like it after staring at SelectElement::selectedIndex. If you still need to cast to int, be sure to use a static_cast. I thought of that as a nice sanity check, to explicitly make sure the returned value for selectedIndex is actually between those two limits. But if you think I'm being too paranoid I have no problems with removing that (which would also remove the previous line, btw). For the time being I'll leave it as it is. About the static_cast: sorry. Fixed! > > WebCore/platform/gtk/PopupMenuGtk.cpp:65 > > - int x, y; > > - gdk_window_get_origin(gtk_widget_get_window(GTK_WIDGET(view->hostWindow()->platformPageClient())), &x, &y); > > + int x = 0; > > + int y = 0; > > + GdkWindow* window = gtk_widget_get_window(GTK_WIDGET(view->hostWindow()->platformPageClient())); > > + if (window) > > + gdk_window_get_origin(window, &x, &y); > > Is this change related? Yes, it's related to this patch because without that extra null check the unit test will crash with a segfault, since the gdk_window_get_origin() function would be called over a NULL value, as returned from gtk_widget_get_window(). Now attaching a new patch. Comment on attachment 74127 [details]
Patch proposal + Unit test
looks good. r=me
Committed r72499: <http://trac.webkit.org/changeset/72499> This change seems to be causing lots of crashes on the bots. Here's a backtrace from http://webkit-bots.igalia.com/amd64/svn_72644.core-when_1290573304-_-who_testatk-_-why_11.trace.html warning: Can't read pathname for load map: Input/output error. Core was generated by `/home/slave/webkitgtk/gtk-linux-64-debug/build/WebKitBuild/Debug/Programs/unitt'. Program terminated with signal 11, Segmentation fault. #0 0x00002baaa52131e4 in WebCore::SelectElement::listBoxOnChange (data=..., element=0x1a8f5a0) at ../../WebCore/dom/SelectElement.cpp:180 180 ASSERT(!data.usesMenuList() || data.multiple()); Thread 2 (Thread 581): #0 0x00002baaaa4dd16c in pthread_cond_wait@@GLIBC_2.3.2 () from /lib/libpthread.so.0 #1 0x00002baaa5ed508e in WTF::ThreadCondition::wait (this=0x197e050, mutex=...) at ../../JavaScriptCore/wtf/ThreadingPthreads.cpp:351 #2 0x00002baaa54a6693 in WebCore::IconDatabase::syncThreadMainLoop (this=0x197df80) at ../../WebCore/loader/icon/IconDatabase.cpp:1420 #3 0x00002baaa54a4bbc in WebCore::IconDatabase::iconDatabaseSyncThread (this=0x197df80) at ../../WebCore/loader/icon/IconDatabase.cpp:1044 #4 0x00002baaa54a4702 in WebCore::IconDatabase::iconDatabaseSyncThreadStart (vIconDatabase=0x197df80) at ../../WebCore/loader/icon/IconDatabase.cpp:947 #5 0x00002baaa5ed4339 in WTF::threadEntryPoint (contextData=0x19865c0) at ../../JavaScriptCore/wtf/Threading.cpp:65 #6 0x00002baaaa4d88ba in start_thread () from /lib/libpthread.so.0 #7 0x00002baaaa7be02d in clone () from /lib/libc.so.6 #8 0x0000000000000000 in ?? () Thread 1 (Thread 580): #0 0x00002baaa52131e4 in WebCore::SelectElement::listBoxOnChange (data=..., element=0x1a8f5a0) at ../../WebCore/dom/SelectElement.cpp:180 #1 0x00002baaa5215fe2 in WebCore::SelectElement::accessKeySetSelectedIndex (data=..., element=0x1a8f5a0, index=0) at ../../WebCore/dom/SelectElement.cpp:953 #2 0x00002baaa5346b48 in WebCore::HTMLSelectElement::accessKeySetSelectedIndex (this=0x1a8f5a0, index=0) at ../../WebCore/html/HTMLSelectElement.cpp:378 #3 0x00002baaa5340030 in WebCore::HTMLOptionElement::accessKeyAction (this=0x1991540) at ../../WebCore/html/HTMLOptionElement.cpp:133 #4 0x00002baaa4f717d0 in WebCore::AccessibilityObject::press (this=0x1ac9b50) at ../../WebCore/accessibility/AccessibilityObject.cpp:147 #5 0x00002baaa5942013 in WebCore::AccessibilityObject::performDefaultAction (this=0x1ac9b50) at ../../WebCore/accessibility/AccessibilityObject.h:443 #6 0x00002baaa593c0f8 in webkit_accessible_action_do_action (action=0x19d69e0, i=0) at ../../WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:664 #7 0x000000000040357a in testWebkitAtkComboBox () at ../../WebKit/gtk/tests/testatk.c:285 #8 0x00002baaaa259ed3 in test_case_run (suite=0x193f140, path=0x2baaaa2a4ebe "") at /tmp/buildd/glib2.0-2.24.1/glib/gtestutils.c:1138 #9 g_test_run_suite_internal (suite=0x193f140, path=0x2baaaa2a4ebe "") at /tmp/buildd/glib2.0-2.24.1/glib/gtestutils.c:1187 #10 0x00002baaaa25a046 in g_test_run_suite_internal (suite=<value optimized out>, path=0x2baaaa2a4ebe "") at /tmp/buildd/glib2.0-2.24.1/glib/gtestutils.c:1197 #11 0x00002baaaa25a046 in g_test_run_suite_internal (suite=<value optimized out>, path=0x2baaaa2a4ebe "") at /tmp/buildd/glib2.0-2.24.1/glib/gtestutils.c:1197 #12 0x00002baaaa25a32b in IA__g_test_run_suite (suite=0x193f100) at /tmp/buildd/glib2.0-2.24.1/glib/gtestutils.c:1246 #13 0x000000000040c554 in main (argc=1, argv=0x7fff92600348) at ../../WebKit/gtk/tests/testatk.c:1335 (In reply to comment #9) > This change seems to be causing lots of crashes on the bots. Here's a backtrace from http://webkit-bots.igalia.com/amd64/svn_72644.core-when_1290573304-_-who_testatk-_-why_11.trace.html > OMG! Reopening bug as per the rollout then... (In reply to comment #9) > [...] > Program terminated with signal 11, Segmentation fault. > #0 0x00002baaa52131e4 in WebCore::SelectElement::listBoxOnChange (data=..., element=0x1a8f5a0) at ../../WebCore/dom/SelectElement.cpp:180 > 180 ASSERT(!data.usesMenuList() || data.multiple()); It's kind of "funny" (not the best word, though) to realize that the patch itself is not causing the crash but something else triggered by the unit test (probably unrelated to the issue being fixed). Anyway, as Martin said to me, the crash was worth as it probably uncovered another issue, which I already have a theory for... It looks like the problem is that this assertion because the combo box (dropdown list) being tested here is actually not compliant with such an statement: (gdb) p data.usesMenuList() $6 = true (gdb) p data.multiple() $7 = false This situation makes me think that perhaps what it's wrong here is the assertion itself, which might be wrong as it'd be proved by the fact that a combo box like this one actually "uses a menu list" while, at the same time, doesn't allow "multiple" selections. But perhaps, I'm missing something, so before proposing getting rid of that assertion I'd like to hear other's opinions and/or suggestions, since that part of the code (HTML elements) it's pretty new for me, so I'm somehow wild guessing at this point. Hence, adding Chris Fleizach to CC (and sorry Chris for adding you to a bug for the second time during today :P) (In reply to comment #11) > (In reply to comment #9) > > This situation makes me think that perhaps what it's wrong here is the assertion itself, which might be wrong as it'd be proved by the fact that a combo box like this one actually "uses a menu list" while, at the same time, doesn't allow "multiple" selections. > I can't say what this assert is trying to protect. best bet is to svn blame that line and CC the person who added it. (In reply to comment #12) > I can't say what this assert is trying to protect. best bet is to svn blame that line and CC the person who > added it. I just filed a new bug blocking this one (since I still think current patch for this bug is valid), adding to CC the people involved in svn blame, as you suggested: bug 50062. Hope to be able to re-integrate the patch here as soon as we get that other new bug fixed... (In reply to comment #13) > [...] > I just filed a new bug blocking this one (since I still think current patch for this bug is valid), adding to CC the people involved in svn blame, as you suggested: bug 50062. > > Hope to be able to re-integrate the patch here as soon as we get that other new bug fixed... Now that bug 50062 has been fixed, it's safe again to reapply this patch again (no changes needed), but I'd appreaciate at least some confirmation from one reviewer (specially from Chris, who reviewed+ the patch) before just going ahead with committing it back to SVN. Hence, waiting for confirmation. Thanks! (In reply to comment #14) > (In reply to comment #13) > > [...] > > I just filed a new bug blocking this one (since I still think current patch for this bug is valid), adding to CC the people involved in svn blame, as you suggested: bug 50062. > > > > Hope to be able to re-integrate the patch here as soon as we get that other new bug fixed... > > Now that bug 50062 has been fixed, it's safe again to reapply this patch again (no changes needed), but I'd appreaciate at least some confirmation from one reviewer (specially from Chris, who reviewed+ the patch) before just going ahead with committing it back to SVN. > > Hence, waiting for confirmation. Thanks! go for it! Committed r72958: <http://trac.webkit.org/changeset/72958> |