Bug 36177 – Multiselect Popup - Listbox click simulation

• Component: WebCore Misc.
• Status: CLOSED FIXED

[Luiz Agostini]

The objective is to create in HTMLSelectElement a method that simulates a click on a listbox.

[Luiz Agostini]

Created attachment 50832 [details]
patch 1

[Kenneth Rohde Christiansen]

Comment on attachment 50832 [details]
patch 1

> +    void listBoxPopupClick(int listIndex, bool multi, bool shift, bool fireEvents = true);

What about something like updateListBoxItem

[Kenneth Rohde Christiansen]

Comment on attachment 50832 [details]
patch 1


> +void HTMLSelectElement::listBoxPopupClick(int listIndex, bool multi, bool shift, bool fireEvents)
> +{
> +    if (!multiple())
> +        setSelectedIndexByUser(listIndex, true, fireEvents);
> +    else {
> +        updateSelectedState(m_data, this, listIndex, multi, shift);
> +        if (fireEvents)
> +            listBoxOnChange();
> +    }
> +}

I don't really know this code so I might be wrong, but the true in setSelectedIndexByUser is for deselect, so I would guess that updateSelectedState might be calling this method.

Maybe it makes more sense to modify updateSelectedState to do the (!multiple()) check?

[Antonio Gomes]

 
+void HTMLSelectElement::listBoxPopupClick(int listIndex, bool multi, bool shift, bool fireEvents)
+{
+    if (!multiple())
+        setSelectedIndexByUser(listIndex, true, fireEvents);
+    else {
+        updateSelectedState(m_data, this, listIndex, multi, shift);
+        if (fireEvents)
+            listBoxOnChange();
+    }
+}

Luiz, yet in this method, what is the difference between |multi| and the calling |multiple()| ?

[Luiz Agostini]

(In reply to comment #2)
> (From update of attachment 50832 [details])
> > +    void listBoxPopupClick(int listIndex, bool multi, bool shift, bool fireEvents = true);
> 
> What about something like updateListBoxItem

What about updateListboxSelection ?

[Luiz Agostini]

(In reply to comment #4)
> +void HTMLSelectElement::listBoxPopupClick(int listIndex, bool multi, bool
> shift, bool fireEvents)
> +{
> +    if (!multiple())
> +        setSelectedIndexByUser(listIndex, true, fireEvents);
> +    else {
> +        updateSelectedState(m_data, this, listIndex, multi, shift);
> +        if (fireEvents)
> +            listBoxOnChange();
> +    }
> +}
> 
> Luiz, yet in this method, what is the difference between |multi| and the
> calling |multiple()| ?

multi is a selection of the user.
you can compare multi and shift with pressing keys ctrl and shift for linux listboxes.

[WebKit Review Bot]

Attachment 50832 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/924008

[Luiz Agostini]

(In reply to comment #7)
> Attachment 50832 [details] did not build on chromium:
> Build output: http://webkit-commit-queue.appspot.com/results/924008

The problem is that the bot still don' t have previous patches (36124).

[Luiz Agostini]

(In reply to comment #3)
> (From update of attachment 50832 [details])
> 
> > +void HTMLSelectElement::listBoxPopupClick(int listIndex, bool multi, bool shift, bool fireEvents)
> > +{
> > +    if (!multiple())
> > +        setSelectedIndexByUser(listIndex, true, fireEvents);
> > +    else {
> > +        updateSelectedState(m_data, this, listIndex, multi, shift);
> > +        if (fireEvents)
> > +            listBoxOnChange();
> > +    }
> > +}
> 
> I don't really know this code so I might be wrong, but the true in
> setSelectedIndexByUser is for deselect, so I would guess that
> updateSelectedState might be calling this method.
> 
> Maybe it makes more sense to modify updateSelectedState to do the (!multiple())
> check?

updateSelectedState is used by listBoxDefaultEventHandler so I would like to make no changes on it.

setSelectedIndexByUser checks the index (optionIndex == selectedIndex()) and may leave without considering other parameters.

Considering the objective of creating a click simulation (see bug description) I created a new method that handles all the relevant parameters.

With the bug description in mind I would suggest:

void HTMLSelectElement::listBoxPopupClick(int listIndex, bool multi, bool shift, bool fireEvents)
{
    updateSelectedState(m_data, this, listIndex, multiple() && multi, shift);
    if (fireEvents)
        listBoxOnChange();
}

[WebKit Review Bot]

Attachment 50832 [details] did not build on gtk:
Build output: http://webkit-commit-queue.appspot.com/results/869036

[Luiz Agostini]

(In reply to comment #9)
> (In reply to comment #3)
> > (From update of attachment 50832 [details] [details])
> > 
> > > +void HTMLSelectElement::listBoxPopupClick(int listIndex, bool multi, bool shift, bool fireEvents)
> > > +{
> > > +    if (!multiple())
> > > +        setSelectedIndexByUser(listIndex, true, fireEvents);
> > > +    else {
> > > +        updateSelectedState(m_data, this, listIndex, multi, shift);
> > > +        if (fireEvents)
> > > +            listBoxOnChange();
> > > +    }
> > > +}
> > 
> > I don't really know this code so I might be wrong, but the true in
> > setSelectedIndexByUser is for deselect, so I would guess that
> > updateSelectedState might be calling this method.
> > 
> > Maybe it makes more sense to modify updateSelectedState to do the (!multiple())
> > check?
> 
> updateSelectedState is used by listBoxDefaultEventHandler so I would like to
> make no changes on it.
> 
> setSelectedIndexByUser checks the index (optionIndex == selectedIndex()) and
> may leave without considering other parameters.
> 
> Considering the objective of creating a click simulation (see bug description)
> I created a new method that handles all the relevant parameters.
> 
> With the bug description in mind I would suggest:
> 
> void HTMLSelectElement::listBoxPopupClick(int listIndex, bool multi, bool
> shift, bool fireEvents)
> {
>     updateSelectedState(m_data, this, listIndex, multiple() && multi, shift);
>     if (fireEvents)
>         listBoxOnChange();
> }

after some tests I think that the first proposed implementation (the one in patch 1) is better. IMHO. :)

the idea is to use that same code used for handling listboxes clicks and that have been refactored in bug 36124 for multiple combos and to fallback to normal combo handling code for simple combos.

[Luiz Agostini]

(In reply to comment #5)
> (In reply to comment #2)
> > (From update of attachment 50832 [details] [details])
> > > +    void listBoxPopupClick(int listIndex, bool multi, bool shift, bool fireEvents = true);
> > 
> > What about something like updateListBoxItem
> 
> What about updateListboxSelection ?

IMHO click may be meaningful. to call this method simulates clicking on a listbox having command pressed (multi) and/or shift pressed (shift).

[Luiz Agostini]

(In reply to comment #10)
> Attachment 50832 [details] did not build on gtk:
> Build output: http://webkit-commit-queue.appspot.com/results/869036

The problem is that the bot still don' t have previous patches (36124).

[Luiz Agostini]

Created attachment 50923 [details]
patch 2

This patch is exactly equal to the previous one. Just to be sure that the bots will check it again now that needed code is landed.

[Kenneth Rohde Christiansen]

Comment on attachment 50923 [details]
patch 2

The thing that I don't like here, is that you are making a method that simulates click regardless of it being a multiple selection or a normal one.

This seems a bit strange to me, as these are currently two different code paths. The question is if they should be that or if that could be refactored.

Also multi and shift are very mouse specific, and for what you are trying to accomplish, you will never need multi nor shift, thus maybe it makes more sense to refactor updateSelectedState into a 

updateIndex(..., int optionIndex, bool fireOnChangeNow, ...) similar to 
SelectElement::setSelectedIndex(SelectElementData, Element, optionIndex, deselect, fireOnChangeNow, userDrivenChange); 

and have updateSelectedState call that? 

I guess updateSelectedState does more than you need, like repaints the items, for instance in the case you "select" an item with a mouse without holding down the keyboard keys.

[Luiz Agostini]

(In reply to comment #15)
> (From update of attachment 50923 [details])
> The thing that I don't like here, is that you are making a method that
> simulates click regardless of it being a multiple selection or a normal one.

I think now that the word 'simulation' should be removed from the bug title. It came from the fact that the very same code used in mose handling is been shared here.

Anyway I think that the name of the method listBoxPopupClick make clear what this method does and what it is used for.

> This seems a bit strange to me, as these are currently two different code
> paths. The question is if they should be that or if that could be refactored.

The need of two separated paths is not obvious to me and I think that the same code could be used for all cases. But for now I would like to keep focus on multiselect. Anyway this patch keeps the two different paths

Please consider that nothing really new is been implemented. Part of the mouse handling code has been refactored and is now been reused here for <select multiple> popups. All the other cases will handled as they were before to be sure that no side effects will show up.

> Also multi and shift are very mouse specific, and for what you are trying to
> accomplish, you will never need multi nor shift,

As this implementation is not platform specific it could be used even with mouses and keyboards. It is not mouse specific because it provides all the needed features for touchable devices but it also does not exclude mouses.

Actually multi is exactly the needed parameter for <select multiple> popups. It indicates if the selection of the list should turn to the pointed option only (multi == false) or if just the select state of the pointed option should toggle (multi == true).
 
> thus maybe it makes more sense to refactor updateSelectedState into a 
> 
> updateIndex(..., int optionIndex, bool fireOnChangeNow, ...) similar to 
> SelectElement::setSelectedIndex(SelectElementData, Element, optionIndex,
> deselect, fireOnChangeNow, userDrivenChange); 
> 
> and have updateSelectedState call that? 

I just think that it is not needed.

> I guess updateSelectedState does more than you need, like repaints the items,
> for instance in the case you "select" an item with a mouse without holding down the keyboard keys.

No repainting happens for menulists renders.

[Kenneth Rohde Christiansen]

Removing the [Qt] as this doesn't only touch Qt code. Actually it is not really Qt related at all.

[Kenneth Rohde Christiansen]

Ok, then I suggest renaming listBoxPopupClick to listBoxSelectItem.

I also got confused with multi, so if I understood you right, allowMultiplySelections might be a better name.

Looking at the header file it seems that there are other methods using 'bool fireOnChangeNow', so you might want to use that instead of 'bool fireEvents', if that is indeed the same.

[Luiz Agostini]

Created attachment 51029 [details]
patch 3

New patch considering Kenneth's suggestions.

[Luiz Agostini]

Created attachment 51030 [details]
patch 4

[Kenneth Rohde Christiansen]

Comment on attachment 51030 [details]
patch 4

r- as your ChangeLog is not in sync with your patch. for instance the method is now called listBoxSelectItem, but the ChangeLog mentions listBoxPopupClick

[Luiz Agostini]

Created attachment 51034 [details]
patch 5

[Kenneth Rohde Christiansen]

Comment on attachment 51034 [details]
patch 5


> +    virtual void listBoxSelectItem(int listIndex, bool allowMultiplySelections,
> +                           bool shift, bool fireOnChangeNow = true) = 0;

Please make this one line.

> +void HTMLSelectElement::listBoxSelectItem(int listIndex, bool allowMultiplySelections,
> +                                          bool shift, bool fireOnChangeNow)

here as well.

>  
> +    void listBoxSelectItem(int listIndex, bool allowMultiplySelections,
> +                           bool shift, bool fireOnChangeNow = true);

and here.

Apart from that LGTM.

[Luiz Agostini]

Created attachment 51048 [details]
patch 6

[WebKit Commit Bot]

Comment on attachment 51048 [details]
patch 6

Clearing flags on attachment: 51048

Committed r56180: <http://trac.webkit.org/changeset/56180>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.