Bug 37530

Summary: No default selection for <select multiple> menu lists.
Product: WebKit Reporter: Luiz Agostini <luiz>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: CLOSED FIXED    
Severity: Normal CC: adele, hausmann, kenneth, koivisto, tonikitoo
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 35784    
Attachments:
Description Flags
Patch
eric: review-
patch 2
none
patch 3 commit-queue: commit-queue-

Description Luiz Agostini 2010-04-13 16:15:47 PDT
No default selection for <select multiple> menu lists.
Comment 1 Luiz Agostini 2010-04-13 16:31:42 PDT
Created attachment 53297 [details]
Patch
Comment 2 Eric Seidel (no email) 2010-04-14 13:08:26 PDT
Comment on attachment 53297 [details]
Patch

This either needs a test case,or the ChangeLog needs to explain why one is not possible.  Seems a LayoutTest should be possible, certainly a manual-test is.
Comment 3 Adele Peterson 2010-04-14 13:17:19 PDT
There are lots of examples of layout tests for <select multiple> that you can look at.  You'll also want to make sure to run all LayoutTests to see if this changes behavior in any of the existing tests.
Comment 4 Luiz Agostini 2010-04-14 14:19:40 PDT
Created attachment 53368 [details]
patch 2
Comment 5 Luiz Agostini 2010-04-14 14:23:18 PDT
This patch was motivated by the fact that usesMenuList() will return always true if NO_LISTBOX_RENDERING is enabled.
As NO_LISTBOX_RENDERING is a compile time flag and as it is enabled by default just for maemo5 LayoutTests would not be effective.
I added a manual test in latest patch.
Comment 6 Luiz Agostini 2010-04-14 14:30:47 PDT
(In reply to comment #3)
> There are lots of examples of layout tests for <select multiple> that you can
> look at.  You'll also want to make sure to run all LayoutTests to see if this
> changes behavior in any of the existing tests.

I added the actual feature in previous patches that are already landed. They caused no regression.
Comment 7 Antti Koivisto 2010-04-14 17:13:12 PDT
Comment on attachment 53368 [details]
patch 2

> This patch makes it possible for <select multiple> and <select size="n gt 1"> elements to have no selected option
> when rendered as menu lists.

I don't quite understand what is the problem and how this patch solves it. Could you explain this in the ChangeLog too? You say "makes it possible" but why, and how?
Comment 8 Luiz Agostini 2010-04-14 18:22:47 PDT
(In reply to comment #7)
> (From update of attachment 53368 [details])
> > This patch makes it possible for <select multiple> and <select size="n gt 1"> elements to have no selected option
> > when rendered as menu lists.
> 
> I don't quite understand what is the problem and how this patch solves it.
> Could you explain this in the ChangeLog too? You say "makes it possible" but
> why, and how?

The target here is to have exactly the same behavior with and without NO_LISTBOX_RENDERING enabled. I mean, the very same <select> element in the very same html file must have the very same selection after loading the page with and without  NO_LISTBOX_RENDERING enabled.

The problem is that, for menu lists, if the selection is not indicated by the html file the first <option> will be selected after loading the page or reseting the form. Please consider bugzilla advanced search form: https://bugs.webkit.org/query.cgi?format=advanced . When the page is loaded many of its listboxes do not have a selected element (for example product, component, version). But when NO_LISTBOX_RENDERING is enabled the listboxes will became menu lists and then they will have an item selected after the page is loaded. That is the behavior difference that this patch corrects.

As when NO_LISTBOX_RENDERING is enabled usesMenuList() always returns true usesMenuList() cannot be used to decide about initial selection of the elements. This patch replaces (usesMenuLists()) by (!multiple && size <= 1) where initial selection is considered (methods recalcListItems and reset).
Comment 9 Antonio Gomes 2010-04-14 20:12:05 PDT
Great explaination! pls add that to the ChangeLog, if you have not yet

(In reply to comment #8)
> (In reply to comment #7)
> > (From update of attachment 53368 [details] [details])
> > > This patch makes it possible for <select multiple> and <select size="n gt 1"> elements to have no selected option
> > > when rendered as menu lists.
> > 
> > I don't quite understand what is the problem and how this patch solves it.
> > Could you explain this in the ChangeLog too? You say "makes it possible" but
> > why, and how?
> 
> The target here is to have exactly the same behavior with and without
> NO_LISTBOX_RENDERING enabled. I mean, the very same <select> element in the
> very same html file must have the very same selection after loading the page
> with and without  NO_LISTBOX_RENDERING enabled.
> 
> The problem is that, for menu lists, if the selection is not indicated by the
> html file the first <option> will be selected after loading the page or
> reseting the form. Please consider bugzilla advanced search form:
> https://bugs.webkit.org/query.cgi?format=advanced . When the page is loaded
> many of its listboxes do not have a selected element (for example product,
> component, version). But when NO_LISTBOX_RENDERING is enabled the listboxes
> will became menu lists and then they will have an item selected after the page
> is loaded. That is the behavior difference that this patch corrects.
> 
> As when NO_LISTBOX_RENDERING is enabled usesMenuList() always returns true
> usesMenuList() cannot be used to decide about initial selection of the
> elements. This patch replaces (usesMenuLists()) by (!multiple && size <= 1)
> where initial selection is considered (methods recalcListItems and reset).
Comment 10 Luiz Agostini 2010-04-15 06:58:25 PDT
Created attachment 53433 [details]
patch 3
Comment 11 Antti Koivisto 2010-04-15 16:52:21 PDT
Comment on attachment 53433 [details]
patch 3

The code here is really convoluted, I wonder if there is a way to clean it up.

Anyway, this seems correct and shouldn't change behavior without NO_LISTBOX_RENDERING as far as I can see, so r=me
Comment 12 WebKit Commit Bot 2010-04-15 22:59:25 PDT
Comment on attachment 53433 [details]
patch 3

Clearing flags on attachment: 53433

Committed r57714: <http://trac.webkit.org/changeset/57714>
Comment 13 WebKit Commit Bot 2010-04-15 23:09:11 PDT
Comment on attachment 53433 [details]
patch 3

Rejecting patch 53433 from commit-queue.

Failed to run "['/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', '--force']" exit_code: 1
Last 500 characters of output:
++++++++
 3 files changed, 62 insertions(+), 4 deletions(-)
 create mode 100644 WebCore/manual-tests/no-listbox-rendering.html


-------------------------------------------------------------------
patching file WebCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file WebCore/dom/SelectElement.cpp
Hunk #1 FAILED at 270.
Hunk #2 FAILED at 499.
2 out of 2 hunks FAILED -- saving rejects to file WebCore/dom/SelectElement.cpp.rej
patching file WebCore/manual-tests/no-listbox-rendering.html

Full output: http://webkit-commit-queue.appspot.com/results/1533517
Comment 14 Luiz Agostini 2010-04-16 12:02:07 PDT
landed in r57714.
Comment 15 Simon Hausmann 2010-04-20 12:28:51 PDT
Revision 57714 cherry-picked into qtwebkit-2.0 with commit 41626128813739703756079cb7fa3e290274678c