CLOSED FIXED 36006
Multiselect popups - rendering
https://bugs.webkit.org/show_bug.cgi?id=36006
Summary Multiselect popups - rendering
Luiz Agostini
Reported 2010-03-11 03:00:57 PST
Adding a compile time flag that forces all <select> elements to use popups.
Attachments
patch (14.79 KB, patch)
2010-03-11 03:29 PST, Luiz Agostini
no flags
Rendering (11.79 KB, patch)
2010-03-14 22:57 PDT, Luiz Agostini
kenneth: review-
SelectElement x Popup communication (18.40 KB, patch)
2010-03-14 23:41 PDT, Luiz Agostini
no flags
patch 1 (10.76 KB, patch)
2010-03-16 14:03 PDT, Luiz Agostini
no flags
patch 2 (10.82 KB, patch)
2010-03-18 11:53 PDT, Luiz Agostini
no flags
patch 3 (10.57 KB, patch)
2010-03-18 15:01 PDT, Luiz Agostini
no flags
Luiz Agostini
Comment 1 2010-03-11 03:29:40 PST
Antti Koivisto
Comment 2 2010-03-11 06:03:38 PST
Comment on attachment 50483 [details] patch We talked about this in IRC: Please use the theme stylesheet mechanism for special appearances. No need for nasty hacks in RenderStyle and SelectElement. Eliminate SELECT_ALWAYS_USE_POPUP_MENU ifdef, switch this based on current style (Maemo5Style in this case) Casting to WebComboStyleOption is bit nasty. If QStyleOptionComplex is insufficient for passing everything, it would be better to subclass it and use that subclass everywhere in webkit Qt theme, not just in special case of combo box.
Luiz Agostini
Comment 3 2010-03-11 22:44:58 PST
(In reply to comment #2) > (From update of attachment 50483 [details]) > We talked about this in IRC: Very nice talk in IRC. A lot of things got more clear now. Thanks. > Please use the theme stylesheet mechanism for special appearances. No need for > nasty hacks in RenderStyle and SelectElement. For sure theme stylesheet mechanism is much better than making changes in core classes. However I could not find how to avoid changes in SelectElement. IMHO it is really needed. > Eliminate SELECT_ALWAYS_USE_POPUP_MENU ifdef, switch this based on current > style (Maemo5Style in this case) I don't want to make this changes enabled in maemo5 right now because if an special popup implementation is not supplied for <select multiple> then it makes <select multiple> == <select>. I am thinking about making this patch completely independent of maemo5 and create a new bug about maemo5 theme. The patch about maemo5 theme would then enable multiselect popup, provide the special multiple popup implementation and make some other theme improvements. What do you think? > Casting to WebComboStyleOption is bit nasty. If QStyleOptionComplex is > insufficient for passing everything, it would be better to subclass it and use > that subclass everywhere in webkit Qt theme, not just in special case of combo > box. Again you are right. However casting is quite common in QStyle and its subclasses implementations. QStyleOptionComboBox is an specialization of QStyleOptionComplex used for comboboxes. It is then not possible avoid special cases for combo boxes because at least QStyleOptionComboBox is needed. WebComboStyleOption subclasses QStyleOptionComboBox. With this approach I got: 1 - QStyle subclasses that are aware of WebComboStyleOption can easily get the information needed to render special combos. 2 - special combo rendering naturally falls back to normal combo rendering in QStyle subclasses that are not aware of WebComboStyleOption. Do you think it is really too nasty?
Luiz Agostini
Comment 4 2010-03-14 22:57:11 PDT
Created attachment 50683 [details] Rendering
Luiz Agostini
Comment 5 2010-03-14 23:41:47 PDT
Created attachment 50686 [details] SelectElement x Popup communication
Kenneth Rohde Christiansen
Comment 6 2010-03-15 14:30:19 PDT
Comment on attachment 50683 [details] Rendering > +++ b/WebCore/css/themeQtNoListboxes.css > @@ -0,0 +1,41 @@ > + * * Neither the name of Google Inc. nor the names of its > + * contributors may be used to endorse or promote products derived from > + * this software without specific prior written permission. why are you adding the above Google expection? > +/* These styles override other user-agent styles for Qt Maemo5. */ This isn't actually Maemo5 specific right, could very well be used for Symbian as well
Kenneth Rohde Christiansen
Comment 7 2010-03-15 21:49:08 PDT
Comment on attachment 50683 [details] Rendering > Adding a compile time flag that when used do: > > * force all <select> elements to use popups. > * adjust Qt rendering of the new combo elements. > * provide information to QStyle objects to differentiate beetwing normal combos and > the new multiselect combos. I guess other platforms might want to use something similar, so maybe it is a better idea to introduce a generic flag and combine it with PLATFORM(QT) if needed? ENABLE_NO_LISTBOXES also doesn't sound very Qt dependent, and from looking at the code it isn't so I would suggest you refrase yourself a bit in the ChangeLog. Also I dislike the name; Enabling a disabler sounds strange to me :-) or maybe find a better name. ENABLE(TOUCHABLE_SELECTORS) maybe?
Kenneth Rohde Christiansen
Comment 8 2010-03-15 21:52:36 PDT
Comment on attachment 50686 [details] SelectElement x Popup communication > void SelectElement::updateListBoxSelection(SelectElementData& data, Element* element, bool deselectOtherOptions) > +void SelectElement::listboxClickHandler(SelectElementData& data, Element* element, int listIndex, The existing methods uses listBox and not listbox in their names (notice the b), please fix.
Kenneth Rohde Christiansen
Comment 9 2010-03-15 21:54:55 PDT
(In reply to comment #8) > (From update of attachment 50686 [details]) > > > void SelectElement::updateListBoxSelection(SelectElementData& data, Element* element, bool deselectOtherOptions) > > +void SelectElement::listboxClickHandler(SelectElementData& data, Element* element, int listIndex, > > The existing methods uses listBox and not listbox in their names (notice the > b), please fix. Also, it sound be called SelectElement::listBoxClickEventHandler for consistency.
Kenneth Rohde Christiansen
Comment 10 2010-03-15 21:55:55 PDT
> Also, it sound be called SelectElement::listBoxClickEventHandler for > consistency. *should* not sound, obviously :-)
Kenneth Rohde Christiansen
Comment 11 2010-03-15 22:01:53 PDT
Comment on attachment 50686 [details] SelectElement x Popup communication > + virtual void listboxClick(int listIndex, bool multi, bool shift, bool fireOnChange = true); Is click really the right terminology here? it sounds very mouse dependent. What about listBoxItemChange? (notice the listbox vs. listBox again)
Antti Koivisto
Comment 12 2010-03-16 03:00:37 PDT
I already asked Luiz to do the SelectElement refactoring in a separate patch, here: https://bugs.webkit.org/show_bug.cgi?id=36124
Luiz Agostini
Comment 13 2010-03-16 13:15:43 PDT
the dependency was created just to be sure that the patches will get in the commit queue in proper order avoiding patch applying problems.
Luiz Agostini
Comment 14 2010-03-16 14:03:24 PDT
Created attachment 50835 [details] patch 1
Luiz Agostini
Comment 15 2010-03-18 10:11:43 PDT
Removing the [Qt] as this doesn't only touch Qt code.
Luiz Agostini
Comment 16 2010-03-18 11:53:48 PDT
Created attachment 51067 [details] patch 2
Dave Hyatt
Comment 17 2010-03-18 12:25:28 PDT
Comment on attachment 51067 [details] patch 2 > #if PLATFORM(QT) && ENABLE(TOUCHABLE_SELECTORS) Don't include the Qt platform ifdef. That should not be necessary. Just use ENABLE(TOUCHABLE_SELECTORS) by itself. The point of ENABLE is to let you define it on the platforms that use it and then platform ifdef gets hidden in the cross-platform code. I don't like the term "TOUCHABLE_SELECTORS" since it's not clear that we're talking about multiple select boxes. Really this feature is about collapsing list boxes in the page and using an external popup to make your choices. I think NO_LISTBOX_RENDERING would be more clear, as per your suggestion on IRC. The rest of the patch looks fine to me.
Luiz Agostini
Comment 18 2010-03-18 15:01:57 PDT
Created attachment 51101 [details] patch 3
Antti Koivisto
Comment 19 2010-03-19 09:29:16 PDT
Comment on attachment 51101 [details] patch 3 Looks good.
WebKit Commit Bot
Comment 20 2010-03-19 19:35:07 PDT
Comment on attachment 51101 [details] patch 3 Clearing flags on attachment: 51101 Committed r56292: <http://trac.webkit.org/changeset/56292>
WebKit Commit Bot
Comment 21 2010-03-19 19:35:13 PDT
All reviewed patches have been landed. Closing bug.
Simon Hausmann
Comment 22 2010-05-07 00:38:27 PDT
Revision r56292 cherry-picked into qtwebkit-2.0 with commit b7bd94e99c8b3211e081275da66de05f454f56cf
Note You need to log in before you can comment on or make changes to this bug.