[Webkit-unassigned] [Bug 76389] Form select option not de-selecting

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=76389


Kent Tamura <tkent at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #124049|review?                     |review-
               Flag|                            |




--- Comment #8 from Kent Tamura <tkent at chromium.org>  2012-01-30 02:27:24 PST ---
(From update of attachment 124049)
View in context: https://bugs.webkit.org/attachment.cgi?id=124049&action=review

> Source/WebCore/ChangeLog:8
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Test: fast/forms/select-option-selecting.html

You had better mention rationale of the change. e.g. specification says so, other browsers work so.

>> Source/WebCore/html/HTMLSelectElement.cpp:771
>> +        selectOption(m_multiple || size() > 1 ? -1 : nextSelectableListIndex(-1));
> 
> I think for clarity we tend to parenthesize the whole conditional even though rules of operator priority makes this correct anyway.
> 
> However, given the more complex expression, I would instead opt to just break this up into an "else if" and "else" clause, for readability and parallelism.

Should this be !usesMenuList()?

>> LayoutTests/ChangeLog:9
>> +        * fast/forms/select-option-selecting.html: Added.
> 
> I've never quite understood where to place these tests-- I assume to keep the folder manageable we should be putting new tests in fast/forms/select/ instead? It looks like newer tests are being added there.

Jon is right.  We had better put tests for <select> into fast/forms/select/.    Bug 65915

> LayoutTests/fast/forms/select-option-selecting.html:33
> +var normal = document.forms[0].elements.normal;

'normal' is not a helpful name.  It should be menuListWithoutSize or something.

> LayoutTests/fast/forms/select-option-selecting.html:37
> +normal.options[2].selected = true;
> +normal.options[2].selected = false;

Please add
 debug('Set and reset normal.options[2].selected');
to improve readability of test result.

> LayoutTests/fast/forms/select-option-selecting.html:42
> +var defaultSizedNormal = document.forms[0].elements.defaultSizedNormal;

'defaultSize' is confusing with having no size.  Should be 'menuListWIthSize1'  or something?

> LayoutTests/fast/forms/select-option-selecting.html:51
> +var sizedNormal = document.forms[0].elements.sizedNormal;

'sizedNormal' is not a good name.  'singleListBox'?

> LayoutTests/fast/forms/select-option-selecting.html:60
> +sizedNormal.selectedIndex = 1;
> +shouldBe("sizedNormal.selectedIndex", "1");

The test result is not readable.  This should be:
  shouldBe("sizedNormal.selectedIndex = 1; sizedNormal.selectedIndex", "1");
or
 debug("Setting 1 to selectedIndex");
 sizedNormal.selectedIndex = 1;
 shouldBe(...)

> LayoutTests/fast/forms/select-option-selecting.html:64
> +sizedNormal.selectedIndex = -1;
> +shouldBe("sizedNormal.selectedIndex", "-1");

ditto.

> LayoutTests/fast/forms/select-option-selecting.html:69
> +var multiple = document.forms[0].elements.multiple;

How about changing the name to 'multipleListBox'?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.