Summary: | [Qt] Platform plugin's multi-select does not take OptGroup into account | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Yael <yael> | ||||||
Component: | WebKit Qt | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | commit-queue, hausmann, kenneth, luiz | ||||||
Priority: | P2 | Keywords: | Qt | ||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | All | ||||||||
OS: | OS X 10.5 | ||||||||
Attachments: |
|
Description
Yael
2010-06-16 10:34:23 PDT
Created attachment 58953 [details]
Patch.
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)
(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. (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. 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.
Created attachment 59028 [details] Patch, updated based on comment #5 LGTM Comment on attachment 59028 [details] Patch, updated based on comment #5 Clearing flags on attachment: 59028 Committed r61487: <http://trac.webkit.org/changeset/61487> All reviewed patches have been landed. Closing bug. The platform plugin is not part of the 2.0 release, so removing from the release branch inclusion bug. |