Bug 85527 – [Qt][WK2] Add support for multi-select list

• Component: WebKit2
• Status: RESOLVED FIXED

[Dinu Jacob]

Assertion failure on selecting an item from a multi-selection list:

ASSERTION FAILED: i < size()
Source/WTF/wtf/Vector.h(527) : T& WTF::Vector<T, inlineCapacity>::at(size_t) [with T = WebKit::PopupMenuItemModel::Item, unsigned int inlineCapacity = 0u, size_t = unsigned int]
1   0xb734ddfe WebKitBuild/Debug/lib/libWebKit2.so.1(_ZN3WTF6VectorIN6WebKit18PopupMenuItemModel4ItemELj0EE2atEj+0x56) [0xb734ddfe]
2   0xb734d946 WebKitBuild/Debug/lib/libWebKit2.so.1(_ZN3WTF6VectorIN6WebKit18PopupMenuItemModel4ItemELj0EEixEj+0x24) [0xb734d946]
3   0xb734c430 WebKitBuild/Debug/lib/libWebKit2.so.1(_ZN6WebKit18PopupMenuItemModel6selectEi+0x98) [0xb734c430]
4   0xb734bf4f WebKitBuild/Debug/lib/libWebKit2.so.1(_ZN6WebKit25ItemSelectorContextObject6acceptEi+0x2d) [0xb734bf4f]
5   0xb734d2d1 WebKitBuild/Debug/lib/libWebKit2.so.1(+0x6ba2d1) [0xb734d2d1]
6   0xb734d47e WebKitBuild/Debug/lib/libWebKit2.so.1(_ZN6WebKit25ItemSelectorContextObject11qt_metacallEN11QMetaObject4CallEiPPv+0x70) [0xb734d47e]

[Dinu Jacob]

Multi-select lists has an invalid selectedIndex(-1). In PopupMenuItemModel::select, attempt to m_items[oldIndex] with oldIndex = m_selectedModelIndex = -1 results in the assertion.

Current implementation supports only single selection list and hence shouldn't even display the selection list.

[Dinu Jacob]

Created attachment 140059 [details]
Patch

This patch just prevents the selection list from being displayed for multi-select lists. Changes are in the working to actually support multi-select list.

[WebKit Review Bot]

Attachment 140059 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQt.cpp:234:  Missing space before {  [whitespace/braces] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dinu Jacob]

Created attachment 140066 [details]
Patch with style fix

[Alexis Menard (darktears)]

(In reply to comment #4)
> Created an attachment (id=140066) [details]
> Patch with style fix

Question : what about other ports?

[Simon Hausmann]

Comment on attachment 140066 [details]
Patch with style fix

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

> Source/WebKit2/ChangeLog:7
> +        Do not display current popup-menu if a multi-select list is requested
> +        as current implementation only supports single selection.

Hm, wouldn't it be better to extend the itemSelector API to allow for the selection of multiple items? Isn't that what we need in the end anyway for the itemSelector API to be complete?

[Dinu Jacob]

(In reply to comment #6)
> (From update of attachment 140066 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=140066&action=review
> 
> > Source/WebKit2/ChangeLog:7
> > +        Do not display current popup-menu if a multi-select list is requested
> > +        as current implementation only supports single selection.
> 
> Hm, wouldn't it be better to extend the itemSelector API to allow for the selection of multiple items? Isn't that what we need in the end anyway for the itemSelector API to be complete?


Yes, that is the final target :) I mentioned in the patch comments that the actual support for multi-select list is being worked on. There are missing parts like this one(where UIProcess doesn't have information on whether the list allows multiple selections), 'selected' flag for each item, refactoring of the existing item selector model and context object before actual support for multi-select can be added. I was planning on doing them in increments to keep the changes more manageable.

[Dinu Jacob]

(In reply to comment #5)
> (In reply to comment #4)
> > Created an attachment (id=140066) [details] [details]
> > Patch with style fix
> 
> Question : what about other ports?

Didn't see other ports using this attribute in their implementations. So I added this only for Qt port (Was that the question? :) )

[Dinu Jacob]

Created attachment 144112 [details]
Patch

[Dinu Jacob]

Using the bug to add support for multi-select list. Changed the title to reflect that.

[WebKit Review Bot]

Attachment 144112 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQt.cpp:205:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQt.cpp:221:  Missing space after ,  [whitespace/comma] [3]
Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQt.cpp:295:  Missing space before {  [whitespace/braces] [5]
Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQt.cpp:306:  Missing space before {  [whitespace/braces] [5]
Source/WebKit2/UIProcess/WebPageProxy.cpp:2756:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/WebKit2/UIProcess/WebPageProxy.cpp:2756:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/WebPageProxy.cpp:2756:  Extra space after ( in function call  [whitespace/parens] [4]
Source/WebKit2/UIProcess/WebPageProxy.cpp:2757:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/UIProcess/WebPageProxy.cpp:2759:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebKit2/WebProcess/WebCoreSupport/WebPopupMenu.cpp:95:  Missing space after ,  [whitespace/comma] [3]
Total errors found: 10 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dinu Jacob]

Created attachment 144141 [details]
Patch with style fix

[Kenneth Rohde Christiansen]

Comment on attachment 144141 [details]
Patch with style fix

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

Just skimming thought... Would be great if Caio could have a look

> Source/WebKit2/Shared/PlatformPopupMenuData.cpp:67
> +#elif PLATFORM(QT)

I thnk there were another if def used in the theme itself when delegation of multiply selection was used

> Source/WebKit2/UIProcess/API/qt/tests/qmltests/DesktopBehavior/tst_multiSelect.qml:8
> +// FIXME: Moved to Desktop tests because we want to have mouseClick() to open the <select> tag. We can move it back
> +// when TestCase starts supporting touch events, see https://bugreports.qt.nokia.com/browse/QTBUG-23083.

Can you use experimental.test? I have some local patch adding better touch support there.

> Source/WebKit2/UIProcess/API/qt/tests/qmltests/common/multiselect.html:12
> +    var seloptions =document.getElementById('sel').options;
> +    var opt;
> +    var str = new String("");
> +    var value = new String("");
> +    for (var i=0, len=seloptions.length; i<len; i++)
> +    {

=document ?
len=sel ?

We should use consistent coding style

> Source/WebKit2/UIProcess/WebPageProxy.cpp:2733
> +void WebPageProxy::valueChangedForPopupMenu(WebPopupMenuProxy*, int32_t newSelectedIndex, bool closePopupMenu)

shouldClosePopuoMenu? closeWhenDone?

It really seems hacked in here. Why not a separate method/message?

> Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQt.cpp:196
> +        // Update the selected indices list

punctuation at end.

> Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQt.cpp:206
> +    } else {
> +
> +        int oldIndex = -1;

Why the added newline?

> Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQt.cpp:244
> +    qSort(list);

Why is the sortign needed?

> Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQt.h:71
> +    bool m_allowMultipleSelections;

Maybe this should be an enum? SelectionStyle { SingleSelection, MultiSelection }, just wondering

[Dinu Jacob]

(In reply to comment #13)
> (From update of attachment 144141 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=144141&action=review
> 
> Just skimming thought... Would be great if Caio could have a look
> 
> > Source/WebKit2/Shared/PlatformPopupMenuData.cpp:67
> > +#elif PLATFORM(QT)
> 
> I thnk there were another if def used in the theme itself when delegation of multiply selection was used
>
Please let me know if there is any other way of getting this information. 
> > Source/WebKit2/UIProcess/API/qt/tests/qmltests/DesktopBehavior/tst_multiSelect.qml:8
> > +// FIXME: Moved to Desktop tests because we want to have mouseClick() to open the <select> tag. We can move it back
> > +// when TestCase starts supporting touch events, see https://bugreports.qt.nokia.com/browse/QTBUG-23083.
> 
> Can you use experimental.test? I have some local patch adding better touch support there.
> 
Sure. I will use that.
> > Source/WebKit2/UIProcess/API/qt/tests/qmltests/common/multiselect.html:12
> > +    var seloptions =document.getElementById('sel').options;
> > +    var opt;
> > +    var str = new String("");
> > +    var value = new String("");
> > +    for (var i=0, len=seloptions.length; i<len; i++)
> > +    {
> 
> =document ?
> len=sel ?
> 
Will fix it.
> We should use consistent coding style
> 
> > Source/WebKit2/UIProcess/WebPageProxy.cpp:2733
> > +void WebPageProxy::valueChangedForPopupMenu(WebPopupMenuProxy*, int32_t newSelectedIndex, bool closePopupMenu)
> 
> shouldClosePopuoMenu? closeWhenDone?
> 
> It really seems hacked in here. Why not a separate method/message?
> 
I did this is to avoid making changes in other ports. I can add new methods for QT that splits this into two.

> > Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQt.cpp:196
> > +        // Update the selected indices list
> 
> punctuation at end.
> 
> > Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQt.cpp:206
> > +    } else {
> > +
> > +        int oldIndex = -1;
> 
> Why the added newline?
> 

> > Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQt.cpp:244
> > +    qSort(list);
> 
> Why is the sortign needed?
> 
For multi-select, more than one index can be in selected state. The smallest indexed item is what is displayed. The selected indices are sorted to determine the smallest when finally closing the popup menu.
> > Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQt.h:71
> > +    bool m_allowMultipleSelections;
> 
> Maybe this should be an enum? SelectionStyle { SingleSelection, MultiSelection }, just wondering
Yes, it might be better to use an enum

[Pierre Rossi]

Comment on attachment 144141 [details]
Patch with style fix

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

From an API point of view in QML, I'm missing something in that change: the fact that a multiple picker is needed or not. I think a "multiple" property of some sort should be exposed in the context object to enforce single selection when the select element expects only one item to be picked and allow picking multiple in other cases.
So essentially one could have two different components for that. I think I'd also have gone for overloads taking a list or a vector of the selected indices and passing that only once to the web process once the selection process is over rather than trying to stick too closely to the very synchronous listbox approach.

>>> Source/WebKit2/Shared/PlatformPopupMenuData.cpp:67
>>> +#elif PLATFORM(QT)
>> 
>> I thnk there were another if def used in the theme itself when delegation of multiply selection was used
> 
> Please let me know if there is any other way of getting this information.

Actually it's now a runtime thing in RenderTheme (RenderTheme::delegatesMenuListRendering which returns false by default but is reimplemented in the mobile theme to return true). So in that case I think PLATFORM(QT) is the right thing to use.

[Dinu Jacob]

(In reply to comment #15)
> (From update of attachment 144141 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=144141&action=review
> 
> From an API point of view in QML, I'm missing something in that change: the fact that a multiple picker is needed or not. I think a "multiple" property of some sort should be exposed in the context object to enforce single selection when the select element expects only one item to be picked and allow picking multiple in other cases.
> So essentially one could have two different components for that. I think I'd also have gone for overloads taking a list or a vector of the selected indices and passing that only once to the web process once the selection process is over rather than trying to stick too closely to the very synchronous listbox approach.
> 
Thanks. 
- Yes, I missed exposing that as a property in the context object though the information has been trickled down. I will add that.

- The reason why the change for each index is sent to WebProcess rather than using a list of indicies is for any onchange event listeners to trigger when the selection/de-selection happens.

> >>> Source/WebKit2/Shared/PlatformPopupMenuData.cpp:67
> >>> +#elif PLATFORM(QT)
> >> 
> >> I thnk there were another if def used in the theme itself when delegation of multiply selection was used
> > 
> > Please let me know if there is any other way of getting this information.
> 
> Actually it's now a runtime thing in RenderTheme (RenderTheme::delegatesMenuListRendering which returns false by default but is reimplemented in the mobile theme to return true). So in that case I think PLATFORM(QT) is the right thing to use.

[Dinu Jacob]

Created attachment 144606 [details]
Patch with fixes

[Tor Arne Vestbø]

Comment on attachment 144606 [details]
Patch with fixes

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

> Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_itemSelector.qml:24
> +                model.close()

I don't like this change. All the dialogs have accept(), reject(), and dismiss(), where the latter is a neutral "close" and the two former implies the latter.

> Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_multiSelect.qml:26
> +        Timer {
> +            running: true
> +            interval: 1
> +            onTriggered: {
> +                var selectedIndices = [0,1]
> +                if (model.allowMultiSelect && selectItems){
> +                    for (var i = 0; i <  selectedIndices.length; i++)
> +                        model.accept(selectedIndices[i])
> +                }
> +                model.close();

There's no need for a timer here anymore, you should be able to do the test in  Component.onCompleted: {} like the other dialog tests do.

[Dinu Jacob]

Created attachment 145120 [details]
Patch

Changes from latest review comments.

[Pierre Rossi]

Comment on attachment 145120 [details]
Patch

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

> Source/WebKit2/UIProcess/WebPageProxy.cpp:2759
> +#if PLATFORM(QT)

Could those be in WebPageProxyQt.cpp maybe ?

> Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQt.cpp:246
> +    qSort(list);

In general, couldn't we avoid this whole logic of determining this selectedOriginalIndex() in the UI process at that point and passing it along with the HidePopupMenu messgage ?

Why not update the Button's text directly in the WebProcess ?

Seems to me that something like this should work:
void RenderMenuList::listBoxSelectItem(int listIndex, bool allowMultiplySelections, bool shift, bool fireOnChangeNow)
{
    HTMLSelectElement* select = toHTMLSelectElement(node());
    select->listBoxSelectItem(listIndex, allowMultiplySelections, shift, fireOnChangeNow);
#if PLATFORM(QT)
    const int firstSelectedIndex = select->selectedIndex();
    if (listIndex <= firstSelectedIndex)
        setTextFromItem(firstSelectedIndex);
#endif
}

With the added benefit of updating the combo button directly during the selection process.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2344
> +#if PLATFORM(QT)

Same as above. WebPageQt.cpp ?

[Dinu Jacob]

(In reply to comment #20)
> (From update of attachment 145120 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145120&action=review
> 
> > Source/WebKit2/UIProcess/WebPageProxy.cpp:2759
> > +#if PLATFORM(QT)
> 
> Could those be in WebPageProxyQt.cpp maybe ?
> 
> > Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQt.cpp:246
> > +    qSort(list);
> 
> In general, couldn't we avoid this whole logic of determining this selectedOriginalIndex() in the UI process at that point and passing it along with the HidePopupMenu messgage ?
> 
> Why not update the Button's text directly in the WebProcess ?
> 
> Seems to me that something like this should work:
> void RenderMenuList::listBoxSelectItem(int listIndex, bool allowMultiplySelections, bool shift, bool fireOnChangeNow)
> {
>     HTMLSelectElement* select = toHTMLSelectElement(node());
>     select->listBoxSelectItem(listIndex, allowMultiplySelections, shift, fireOnChangeNow);
> #if PLATFORM(QT)
>     const int firstSelectedIndex = select->selectedIndex();
>     if (listIndex <= firstSelectedIndex)
>         setTextFromItem(firstSelectedIndex);
> #endif
> }
> 
> With the added benefit of updating the combo button directly during the selection process.

Yes, I think this is much cleaner. This will eliminate having to maintain the list of selected indices as well.
> 
> > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2344
> > +#if PLATFORM(QT)
> 
> Same as above. WebPageQt.cpp ?


Didn't realize there was a Qt specific file for WebPage.cpp and WebPageProxy.cpp. Will move the changes there.

[Dinu Jacob]

Created attachment 145192 [details]
Patch

[WebKit Review Bot]

Attachment 145192 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQt.cpp:194:  Extra space before ) in if  [whitespace/parens] [5]
Total errors found: 1 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dinu Jacob]

Created attachment 145197 [details]
Patch

[Tor Arne Vestbø]

Comment on attachment 145197 [details]
Patch

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

> Source/WebKit2/UIProcess/API/qt/tests/qmltests/WebView/tst_multiSelect.qml:23
> +            if (model.allowMultiSelect && selectItems){
> +                for (var i = 0; i <  selectedIndices.length; i++)
> +                    model.accept(selectedIndices[i])
> +            }
> +            model.dismiss();

I think perhaps a better API for this would be a bunch of select(index), with a following accept()

accept(index) would be shorthand for select(index) && accept()

[Dinu Jacob]

Created attachment 145393 [details]
Patch

[Dinu Jacob]

Created attachment 145404 [details]
Patch

[Allan Sandfeld Jensen]

Comment on attachment 145404 [details]
Patch

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

Otherwise it looks good I think. Have you pinged any reviewers recently?

> Source/WebCore/rendering/RenderMenuList.cpp:349
> +    setTextFromItem(firstSelectedIndex);

Missing indentation?

[Dinu Jacob]

(In reply to comment #28)
> (From update of attachment 145404 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145404&action=review
> 
> Otherwise it looks good I think. Have you pinged any reviewers recently?
> 
> > Source/WebCore/rendering/RenderMenuList.cpp:349
> > +    setTextFromItem(firstSelectedIndex);
> 
> Missing indentation?

Will fix that. No, I didn't ping any reviewers after the re-org at Nokia :( I will ping them now.

[Pierre Rossi]

Comment on attachment 145404 [details]
Patch

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

While we're at it on small refinements... ;)

> Source/WebCore/rendering/RenderMenuList.cpp:348
> +    if (listIndex <= firstSelectedIndex)

Changing this condition to take into account the de-selection of the last item would be icing on the cake.
Something along that line should do:
if (listIndex <= firstSelectedIndex || firstSelectedIndex < 0)

[Tor Arne Vestbø]

Comment on attachment 145404 [details]
Patch

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

> Source/WebCore/rendering/RenderMenuList.cpp:347
> +    const int firstSelectedIndex = select->selectedIndex();

As discussed on IRC, this should be a separate patch with ref. to possible spec behavior etc.

[Pierre Rossi]

Created attachment 153299 [details]
Patch

Updated Dinu's patch based on previous comments. Leaving RenderMenuList out of it for now.

[Kenneth Rohde Christiansen]

Comment on attachment 153299 [details]
Patch

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

I think this is fine for the initial commit. Why no test?

> Source/WebKit2/Shared/PlatformPopupMenuData.cpp:69
> +#elif PLATFORM(QT)
> +    encoder->encode(multipleSelections);
>  #endif

Other ports might want this, maybe we can find another define

> Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQt.cpp:63
> +    void selectItem(int);

isnt this more of a toggleItem? im just wondering

> Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQt.cpp:145
> +    if ((index == -1 ) && m_items.multiple())

some werd space there

[Pierre Rossi]

Comment on attachment 153299 [details]
Patch

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

>> Source/WebKit2/Shared/PlatformPopupMenuData.cpp:69
>>  #endif
> 
> Other ports might want this, maybe we can find another define

Technically we could expose it for everybody, and leave it up to each port to use it or not. AFAIK, we're the only WK2 port to delegate menu list rendering (formerly guarded by NO_LISTBOX_RENDERING). If anybody else shows interest in this, we can always change it.

>> Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQt.cpp:63
>> +    void selectItem(int);
> 
> isnt this more of a toggleItem? im just wondering

Agreed, I also think it is a more accurate description.

>> Source/WebKit2/UIProcess/qt/WebPopupMenuProxyQt.cpp:145
>> +    if ((index == -1 ) && m_items.multiple())
> 
> some werd space there

Will fix that before landing.

[Pierre Rossi]

Committed r123216: <http://trac.webkit.org/changeset/123216>