Bug 37530

Created attachment 53297 [details] Patch

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.

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.

Created attachment 53368 [details] patch 2

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.

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

(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).

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).

Created attachment 53433 [details] patch 3

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 on attachment 53433 [details] patch 3 Clearing flags on attachment: 53433 Committed r57714: <http://trac.webkit.org/changeset/57714>

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

landed in r57714.

Revision 57714 cherry-picked into qtwebkit-2.0 with commit 41626128813739703756079cb7fa3e290274678c