|Summary:||Multiselect popups - rendering|
|Product:||WebKit||Reporter:||Luiz Agostini <luiz>|
|Component:||WebCore Misc.||Assignee:||Luiz Agostini <luiz>|
|Severity:||Normal||CC:||commit-queue, hausmann, kenneth, koivisto, yael|
|Version:||528+ (Nightly build)|
|Bug Depends on:||36177|
Description Luiz Agostini 2010-03-11 03:00:57 PST
Adding a compile time flag that forces all <select> elements to use popups.
Comment 2 Antti Koivisto 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.
Comment 3 Luiz Agostini 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?
Comment 5 Luiz Agostini 2010-03-14 23:41:47 PDT
Created attachment 50686 [details] SelectElement x Popup communication
Comment 6 Kenneth Rohde Christiansen 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
Comment 7 Kenneth Rohde Christiansen 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?
Comment 8 Kenneth Rohde Christiansen 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.
Comment 9 Kenneth Rohde Christiansen 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.
Comment 10 Kenneth Rohde Christiansen 2010-03-15 21:55:55 PDT
> Also, it sound be called SelectElement::listBoxClickEventHandler for > consistency. *should* not sound, obviously :-)
Comment 11 Kenneth Rohde Christiansen 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)
Comment 12 Antti Koivisto 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
Comment 13 Luiz Agostini 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.
Comment 15 Luiz Agostini 2010-03-18 10:11:43 PDT
Removing the [Qt] as this doesn't only touch Qt code.
Comment 17 Dave Hyatt 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.
Comment 19 Antti Koivisto 2010-03-19 09:29:16 PDT
Comment on attachment 51101 [details] patch 3 Looks good.
Comment 20 WebKit Commit Bot 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>
Comment 21 WebKit Commit Bot 2010-03-19 19:35:13 PDT
All reviewed patches have been landed. Closing bug.