Bug 66282 – When changing the size of a menulist from x (x>1) to 1, the first item should be selected.

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Jing Zhao]

Should set default selection for single selects

[Jing Zhao]

Created attachment 104010 [details]
Patch

[Jing Zhao]

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.

[Jing Zhao]

Created attachment 104034 [details]
Patch

[Jing Zhao]

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.

[Johnny(Jianning) Ding]

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?

[Jing Zhao]

Created attachment 104039 [details]
Patch

[Jing Zhao]

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!

[Jing Zhao]

Created attachment 104164 [details]
Patch

[Early Warning System Bot]

Comment on attachment 104164 [details]
Patch

Attachment 104164 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9402827

[Jing Zhao]

Created attachment 104165 [details]
Patch

[Jing Zhao]

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.

[Kent Tamura]

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

[Jing Zhao]

I already have a test for this, and will commit it later when Android DumpRenderTree is ready.

Thanks!

[Kent Tamura]

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

[WebKit Review Bot]

Comment on attachment 104165 [details]
Patch

Clearing flags on attachment: 104165

Committed r93295: <http://trac.webkit.org/changeset/93295>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.