Bug 36006 – Multiselect popups - rendering

• Component: WebCore Misc.
• Status: CLOSED FIXED

[Luiz Agostini]

Adding a compile time flag that forces all <select> elements to use popups.

[Luiz Agostini]

Created attachment 50483 [details]
patch

[Antti Koivisto]

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]

(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]

Created attachment 50683 [details]
Rendering

[Luiz Agostini]

Created attachment 50686 [details]
SelectElement x Popup communication

[Kenneth Rohde Christiansen]

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 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 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]

(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]

> Also, it sound be called SelectElement::listBoxClickEventHandler for
> consistency.

*should* not sound, obviously :-)

[Kenneth Rohde Christiansen]

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]

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]

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]

Created attachment 50835 [details]
patch 1

[Luiz Agostini]

Removing the [Qt] as this doesn't only touch Qt code.

[Luiz Agostini]

Created attachment 51067 [details]
patch 2

[Dave Hyatt]

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]

Created attachment 51101 [details]
patch 3

[Antti Koivisto]

Comment on attachment 51101 [details]
patch 3

Looks good.

[WebKit Commit Bot]

Comment on attachment 51101 [details]
patch 3

Clearing flags on attachment: 51101

Committed r56292: <http://trac.webkit.org/changeset/56292>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Hausmann]

Revision r56292 cherry-picked into qtwebkit-2.0 with commit b7bd94e99c8b3211e081275da66de05f454f56cf