Bug 66282 - When changing the size of a menulist from x (x>1) to 1, the first item should be selected.
Summary: When changing the size of a menulist from x (x>1) to 1, the first item should...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-08-15 23:13 PDT by Jing Zhao
Modified: 2011-08-18 04:06 PDT (History)
7 users (show)

See Also:


Attachments
Patch (1.78 KB, patch)
2011-08-15 23:25 PDT, Jing Zhao
no flags Details | Formatted Diff | Diff
Patch (1.65 KB, patch)
2011-08-16 06:42 PDT, Jing Zhao
no flags Details | Formatted Diff | Diff
Patch (1.43 KB, patch)
2011-08-16 07:25 PDT, Jing Zhao
no flags Details | Formatted Diff | Diff
Patch (1.37 KB, patch)
2011-08-17 03:15 PDT, Jing Zhao
no flags Details | Formatted Diff | Diff
Patch (1.76 KB, patch)
2011-08-17 03:26 PDT, Jing Zhao
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jing Zhao 2011-08-15 23:13:31 PDT
Should set default selection for single selects
Comment 1 Jing Zhao 2011-08-15 23:25:38 PDT
Created attachment 104010 [details]
Patch
Comment 2 Jing Zhao 2011-08-15 23:32:41 PDT
Now we only set default selection for a single select when its size is 1. We should change that condition to if it's presented as a menulist, because in Android we present a single select as a menulist no matter what its size is.
Comment 3 Jing Zhao 2011-08-16 06:42:17 PDT
Created attachment 104034 [details]
Patch
Comment 4 Jing Zhao 2011-08-16 06:47:01 PDT
Xianzhu suggested not to select the first item when size > 1, because that's inconsistent with other platforms. If a developer assumes a select should be unselected because its size > 1, his/her page may have different behavior in Android browser.

I changed this bug to fix another problem: when changing the size of a menulist from x (x>1) to 1, the first item should be selected. The current logic assumes a menulist must have its size <= 1, which is true in most WebKit platforms, but in Android browser, a select is always presented as a menulist no matter what size it is.
Comment 5 Johnny(Jianning) Ding 2011-08-16 07:17:19 PDT
Comment on attachment 104034 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=104034&action=review

> Source/WebCore/html/HTMLSelectElement.cpp:266
> +        if ((oldUsesMenuList != m_data.usesMenuList() || (!oldUsesMenuList && m_data.size() != oldSize)) && attached())

Current WebKit code assumes that menuList is always only used for select(size<=1 && !m_multiple), but looks like Android browser breaks this rule. So in the second condition, even the oldUsesMenuList is true (which means m_data.usesMenuList() is also true, but does not mean the size of select is 1), the Select's size still can be changed to value other than 1 and the change still requires to re-generate the renderer object (call method reattach)

What do you think?
Comment 6 Jing Zhao 2011-08-16 07:25:11 PDT
Created attachment 104039 [details]
Patch
Comment 7 Jing Zhao 2011-08-16 07:28:02 PDT
Comment on attachment 104034 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=104034&action=review

>> Source/WebCore/html/HTMLSelectElement.cpp:266
>> +        if ((oldUsesMenuList != m_data.usesMenuList() || (!oldUsesMenuList && m_data.size() != oldSize)) && attached())
> 
> Current WebKit code assumes that menuList is always only used for select(size<=1 && !m_multiple), but looks like Android browser breaks this rule. So in the second condition, even the oldUsesMenuList is true (which means m_data.usesMenuList() is also true, but does not mean the size of select is 1), the Select's size still can be changed to value other than 1 and the change still requires to re-generate the renderer object (call method reattach)
> 
> What do you think?

Totally agreed. Just updated a new patch. Thanks!
Comment 8 Jing Zhao 2011-08-17 03:15:15 PDT
Created attachment 104164 [details]
Patch
Comment 9 Early Warning System Bot 2011-08-17 03:22:22 PDT
Comment on attachment 104164 [details]
Patch

Attachment 104164 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9402827
Comment 10 Jing Zhao 2011-08-17 03:26:35 PDT
Created attachment 104165 [details]
Patch
Comment 11 Jing Zhao 2011-08-17 03:28:23 PDT
There are 3 different cases when (m_data.size() != oldSize):

1. (oldUsesMenuList != m_data.usesMenuList()): menu list changes to list box or list box changes to menu list
2. (!oldUsesMenuList && m_data.size() != oldSize): list box changes size
3. (oldUsesMenuList && m_data.size() != oldSize): menu list changes size

The original condition excludes the third case because it doesn't exist in platforms other than Android browser. My change is to add back the third case.
Comment 12 Kent Tamura 2011-08-17 21:42:48 PDT
Comment on attachment 104165 [details]
Patch

The code change looks ok.
I'm afraid the test coverage. Do you have a plan to create LayoutTests/platforms/android/ directory? If so, you should add a test for this change into LayouTests/platforms/android/fast/forms/.
Comment 13 Jing Zhao 2011-08-18 03:17:12 PDT
I already have a test for this, and will commit it later when Android DumpRenderTree is ready.

Thanks!
Comment 14 Kent Tamura 2011-08-18 03:18:48 PDT
(In reply to comment #13)
> I already have a test for this, and will commit it later when Android DumpRenderTree is ready.

Ok, it's reasonable.
Comment 15 WebKit Review Bot 2011-08-18 04:06:06 PDT
Comment on attachment 104165 [details]
Patch

Clearing flags on attachment: 104165

Committed r93295: <http://trac.webkit.org/changeset/93295>
Comment 16 WebKit Review Bot 2011-08-18 04:06:11 PDT
All reviewed patches have been landed.  Closing bug.