Bug 40718 – [Qt] Platform plugin's multi-select does not take OptGroup into account

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Yael]

When QtWebKit and the example platform plugin are configured with NO_LISTBOX_RENDERING enabled, OptGroup elements are not taken into account. This results in the platform plugin reporting the wrong indexes as being selected.

In addition, Orbit does not have a callback when an element is selected or de-selected, so all the selections are reported when the dialog is closed.
WebCore is expecting the dialog to report state change, not selection, and that results in wrong selections.

To fix both problems, we need the initial selection data to be available to the platform plugin throughout the lifetime of the dialog, however the data is deleted as soon as the dialog is shown. 
(The data is created on the stack in SelectInputMethodWrapper::show()).

[Yael]

Created attachment 58953 [details]
Patch.

[Kenneth Rohde Christiansen]

Comment on attachment 58953 [details]
Patch.

WebKit/qt/examples/platformplugin/WebPlugin.h:56
 +      virtual bool isMultiple() const { return false; }
I think this is what the extension was supposed to be used for

WebKit/qt/examples/platformplugin/WebPlugin.cpp:117
 +      emit selectItem(idx, m_popup->isMultiple(), false);
isnt this used for single as well? 

WebKit/qt/WebCoreSupport/QtPlatformPlugin.h:51
 +      SelectData* m_selectData;
isnt there a way to keep this away from the header?

WebCore/dom/SelectElement.cpp:92
 +  #if ENABLE(NO_LISTBOX_RENDERING)
We should really turn this into a runtime thing so that it can even consult the plugin to see if it (the plugin) implements multiselection (using the extension)

[Yael]

(In reply to comment #2)
> (From update of attachment 58953 [details])
> WebKit/qt/examples/platformplugin/WebPlugin.h:56
>  +      virtual bool isMultiple() const { return false; }
> I think this is what the extension was supposed to be used for
> 
This virtual function is for checking if a specific select element is defined as multi or not, while the extension is to check if the plugin supports miltiple dialogs.

> WebKit/qt/examples/platformplugin/WebPlugin.cpp:117
>  +      emit selectItem(idx, m_popup->isMultiple(), false);
> isnt this used for single as well? 
> 
Yes, and when this is used for single select, we should pass false, not true for the second parameter. This is why I added the above virtual function :-)

> WebKit/qt/WebCoreSupport/QtPlatformPlugin.h:51
>  +      SelectData* m_selectData;
> isnt there a way to keep this away from the header?
> 
I am not sure how to do that without introducing a memory leak or a wrapper for this class.
Please keep in mind that the header is internal implementation of the sample plugin, I don't think it is a problem that this is in the header.

> WebCore/dom/SelectElement.cpp:92
>  +  #if ENABLE(NO_LISTBOX_RENDERING)
> We should really turn this into a runtime thing so that it can even consult the plugin to see if it (the plugin) implements multiselection (using the extension)
I don't think there is a use case for supporting a full screen dialog for single select, and not for multi select. In small devices, we always want both.

[Yael]

(In reply to comment #3)
> (In reply to comment #2)
> > WebKit/qt/WebCoreSupport/QtPlatformPlugin.h:51
> >  +      SelectData* m_selectData;
> > isnt there a way to keep this away from the header?
> > 
> I am not sure how to do that without introducing a memory leak or a wrapper for this class.
> Please keep in mind that the header is internal implementation of the sample plugin, I don't think it is a problem that this is in the header.

My bad, of course the header file is not part of the plugin, but in WebCoreSupport, but I still don't think this should be a problem, as The class definition is still hidden.

[Luiz Agostini]

Comment on attachment 58953 [details]
Patch.

WebCore/dom/SelectElement.cpp:93
 +      if (!data.multiple() && data.size() <= 1) {
Why is it needed?

WebCore/rendering/RenderMenuList.cpp:317
 +      if (allowMultiplySelections)
This is not right. The combobox behavior must be based on it being multiple or not and not on any parameters of this method. Parameters allowMultiplySelections and shift must be ignored if the combobox is not null.

I beleave that the right thing to do is:

diff --git a/WebCore/html/HTMLSelectElement.cpp b/WebCore/html/HTMLSelectElement.cpp
index 9aacbb6..2dcac59 100644
--- a/WebCore/html/HTMLSelectElement.cpp
+++ b/WebCore/html/HTMLSelectElement.cpp
@@ -107,7 +107,7 @@ void HTMLSelectElement::setSelectedIndexByUser(int optionIndex, bool deselect, b
 void HTMLSelectElement::listBoxSelectItem(int listIndex, bool allowMultiplySelections, bool shift, bool fireOnChangeNow)
 {
     if (!multiple())
-        setSelectedIndexByUser(listIndex, true, fireOnChangeNow);
+        setSelectedIndexByUser(listToOptionIndex(listIndex), true, fireOnChangeNow);
     else {
         updateSelectedState(m_data, this, listIndex, allowMultiplySelections, shift);
         if (fireOnChangeNow)
diff --git a/WebCore/rendering/RenderMenuList.cpp b/WebCore/rendering/RenderMenuList.cpp
index 871c10f..77fe3c2 100644
--- a/WebCore/rendering/RenderMenuList.cpp
+++ b/WebCore/rendering/RenderMenuList.cpp
@@ -314,7 +314,7 @@ void RenderMenuList::valueChanged(unsigned listIndex, bool fireOnChange)
 void RenderMenuList::listBoxSelectItem(int listIndex, bool allowMultiplySelections, bool shift, bool fireOnChangeNow)
 {
     SelectElement* select = toSelectElement(static_cast<Element*>(node()));
-    select->listBoxSelectItem(select->listToOptionIndex(listIndex), allowMultiplySelections, shift, fireOnChangeNow);
+    select->listBoxSelectItem(listIndex, allowMultiplySelections, shift, fireOnChangeNow);
 }

WebKit/qt/WebCoreSupport/QtPlatformPlugin.h:51
 +      SelectData* m_selectData;
You can just declare m_selectData as a pointer to QWebSelectData instead of a pointer to SelectData. No memory will leak because none of those classes own any memory that should be released. But please help me with one big mistake I made: I declared inline the QWebSelectData's destructor and it should be virtual instead. The same thing for QWebSelectMethod. Could you please fix it?

A virtual destructor should be added to class QWebNotificationData as well.

Shouldn't you check if m_selectData is not null and destroy it before creating a new one in show().

WebKit/qt/examples/platformplugin/WebPlugin.h:36
 +      virtual bool isMultiple() const = 0;
This is not needed. Parameters allowMultipleSelections and shift will be ignored if the combobox is not multiple.

[Yael]

Created attachment 59028 [details]
Patch, updated based on comment #5

[Luiz Agostini]

LGTM

[WebKit Commit Bot]

Comment on attachment 59028 [details]
Patch, updated based on comment #5

Clearing flags on attachment: 59028

Committed r61487: <http://trac.webkit.org/changeset/61487>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Hausmann]

The platform plugin is not part of the 2.0 release, so removing from the release branch inclusion bug.