[webkit-reviews] review requested: [Bug 53146] [GTK] Combo boxes should emit object:selection-changed even when collapsed : [Attachment 82943] Patch proposal + Layout test

[bugzilla-daemon at webkit.org]

Mario Sanchez Prada <msanchez at igalia.com> has asked  for review:
Bug 53146: [GTK] Combo boxes should emit object:selection-changed even when
collapsed
https://bugs.webkit.org/show_bug.cgi?id=53146

Attachment 82943: Patch proposal + Layout test
https://bugs.webkit.org/attachment.cgi?id=82943&action=review

------- Additional Comments from Mario Sanchez Prada <msanchez at igalia.com>
(In reply to comment #7)
> Reverted r78979 for reason:
> 
> causes multiple crashes on GTK
> 
> Committed r78987: <http://trac.webkit.org/changeset/78987>

Attaching a new version of the patch fixing that issue. The diff between the
new patch and the previous one is as follows:

--- a/Source/WebCore/accessibility/gtk/AXObjectCacheAtk.cpp
+++ b/Source/WebCore/accessibility/gtk/AXObjectCacheAtk.cpp
@@ -95,10 +95,10 @@ static void
notifyChildrenSelectionChange(AccessibilityObject* object)
	 return;
     AccessibilityObject* item = items.at(changedItemIndex).get();
 
-    // Ensure the oldFocusedObject belongs to the same document that
+    // Ensure oldFocusedObject belongs to the same list object that
     // the current item so further comparisons make sense. Otherwise,
     // just reset oldFocusedObject so it won't be taken into account.
-    if (item && oldFocusedObject && item->document() !=
oldFocusedObject->document())
+    if (item && oldFocusedObject && item->parentObject() !=
oldFocusedObject->parentObject())
	 oldFocusedObject = 0;


The problem was that at some point AccessibleObject:documentFrameView() (called
internally from AccessibilityObject::document()) was crashing because of
reaching and invalid object when called oldFocusedObject->document(). And this
was easily avoidable by just checking that the parent object (instead of the
document) for the currently selected item ('item') and the old focused object
are the same, which is more than enough.

Tested locally and this doesn't make other tests crash anymore.