Bug 36006 - Multiselect popups - rendering
Summary: Multiselect popups - rendering
Status: CLOSED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Luiz Agostini
URL:
Keywords: Qt
Depends on: 36177
Blocks: 35784
  Show dependency treegraph
 
Reported: 2010-03-11 03:00 PST by Luiz Agostini
Modified: 2010-05-07 00:39 PDT (History)
5 users (show)

See Also:


Attachments
patch (14.79 KB, patch)
2010-03-11 03:29 PST, Luiz Agostini
no flags Details | Formatted Diff | Diff
Rendering (11.79 KB, patch)
2010-03-14 22:57 PDT, Luiz Agostini
kenneth: review-
Details | Formatted Diff | Diff
SelectElement x Popup communication (18.40 KB, patch)
2010-03-14 23:41 PDT, Luiz Agostini
no flags Details | Formatted Diff | Diff
patch 1 (10.76 KB, patch)
2010-03-16 14:03 PDT, Luiz Agostini
no flags Details | Formatted Diff | Diff
patch 2 (10.82 KB, patch)
2010-03-18 11:53 PDT, Luiz Agostini
no flags Details | Formatted Diff | Diff
patch 3 (10.57 KB, patch)
2010-03-18 15:01 PDT, Luiz Agostini
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Luiz Agostini 2010-03-11 03:00:57 PST
Adding a compile time flag that forces all <select> elements to use popups.
Comment 1 Luiz Agostini 2010-03-11 03:29:40 PST
Created attachment 50483 [details]
patch
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 4 Luiz Agostini 2010-03-14 22:57:11 PDT
Created attachment 50683 [details]
Rendering
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 14 Luiz Agostini 2010-03-16 14:03:24 PDT
Created attachment 50835 [details]
patch 1
Comment 15 Luiz Agostini 2010-03-18 10:11:43 PDT
Removing the [Qt] as this doesn't only touch Qt code.
Comment 16 Luiz Agostini 2010-03-18 11:53:48 PDT
Created attachment 51067 [details]
patch 2
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 18 Luiz Agostini 2010-03-18 15:01:57 PDT
Created attachment 51101 [details]
patch 3
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.
Comment 22 Simon Hausmann 2010-05-07 00:38:27 PDT
Revision r56292 cherry-picked into qtwebkit-2.0 with commit b7bd94e99c8b3211e081275da66de05f454f56cf