[Webkit-unassigned] [Bug 50062] New: ASSERT failing for combo boxes when selection changes

[bugzilla-daemon at webkit.org]

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

           Summary: ASSERT failing for combo boxes when selection changes
           Product: WebKit
           Version: 528+ (Nightly build)
          Platform: PC
        OS/Version: Linux
            Status: NEW
          Severity: Normal
          Priority: P2
         Component: HTML DOM
        AssignedTo: webkit-unassigned at lists.webkit.org
        ReportedBy: msanchez at igalia.com
                CC: eric at webkit.org, zimmermann at kde.org,
                    mrobinson at webkit.org
            Blocks: 25678


As it was found recently through a new unit test coming with the patch for bug 25678 [1], there's a situation where the following ASSERT in WebCore/dom/SelectElement.cpp is failing for debug builds:

   void SelectElement::listBoxOnChange(SelectElementData& data, Element* element)
   {
        ASSERT(!data.usesMenuList() || data.multiple());

        Vector<bool>& lastOnChangeSelection = data.lastOnChangeSelection(); 
        const Vector<Element*>& items = data.listItems(element);

        [...]
   }

In the case found through that unit test, that assertion is reached through executing the AccessibilityObject::performDefaultAction() function for one of the HTMLOptionElements for a given combo box (that is, a SELECT element with a dropdown list which does not allow multiple selections).

Part of the problem seems to be, IMHO, that such a function is called explicitly from SelectElement::accessKeySetSelectedIndex(), instead of making sure that it's only done for listboxes:

   void SelectElement::accessKeySetSelectedIndex(SelectElementData& data, Element* element, int index)
   {    
       [...]

       listBoxOnChange(data, element);
       scrollToSelection(data, element);
   }

On top of that, I think there's another problem here, and it's about the assertion, which perhaps it's not the best one for that function (which is related to listboxes only). Following what it's being done in SelectElement::updateListBoxSelection()...

   void SelectElement::updateListBoxSelection(SelectElementData& data, Element* element, bool deselectOtherOptions)
   {
       ASSERT(element->renderer() && (element->renderer()->isListBox() || data.multiple()));
       [...]
   }

...I think it could make sense if the same assertion was done in listBoxOnChange, instead of  ASSERT(!data.usesMenuList() || data.multiple())

CCing people found in svn blame, blocking bug 25678 (as the patch there was temporarily rolled out due to this issue) and hoping to attach soon a patch I already have implementing the idea above (which is passing the tests.

[1] http://webkit-bots.igalia.com/amd64/svn_72644.core-when_1290573304-_-who_testatk-_-why_11.trace.html

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