Fix: Incorrect display of HTML select multiple tag on Android

Fix: Incorrect display of HTML select multiple tag on Android

This CL adds to RenderMenuList::setTextFromOption(int) so that when the
associated HTML <select> element has the multiple attribute, the <select>
element's options are iterated over and a count of the number of selected
options is determined. When the number of selected options is 0 or 2 or
more, then the RenderMenuList's text is set to "SELECTED_COUNT selected"
(e.g. "2 selected"). When exactly one option is selected, then the text
is set to the selected option's text.

This is similar to iOS. When 0 or 2 or more options are selected, then
the text of the menulist button is set to "SELECTED_COUNT Items". When
exactly one option is selected, then the text is set to the selected
option's text.

Depends on:
- https://codereview.chromium.org/280483002/
- https://codereview.chromium.org/295933003/

BUG=314704
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=175010

[dtrebbien, 2014-05-24 22:35:34]

Hi Adam and Kent,

This is the change that I had in mind to fix Issue 314704: Incorrect display of
HTML select multiple tag on Android:
https://code.google.com/p/chromium/issues/detail?id=314704

I was able to test on Linux and Mac, but I can't build for Android currently. 
Though I checked that the menu list button text would be correct on Android by
changing `Source/core/rendering/RenderTheme.h' locally (hacking
delegatesMenuListRendering() to return true).

For the LayoutTests/fast/forms/select/menulist-appearance-basic.html test, there
are "expected" screenshots for all platforms that will need to be updated.

I expect that the Android and Windows test bots will fail when running the test
because I haven't built for those platforms or updated the
`menulist-appearance-basic-expected.txt' file.  Is there a way to view the
output of the test (text and image diffs) when the bots run it?

[tkent, 2014-05-27 00:45:41]

On 2014/05/24 22:35:34, dtrebbien wrote:
> For the LayoutTests/fast/forms/select/menulist-appearance-basic.html test,
there
> are "expected" screenshots for all platforms that will need to be updated.
> 
> I expect that the Android and Windows test bots will fail when running the
test
> because I haven't built for those platforms or updated the
> `menulist-appearance-basic-expected.txt' file.  Is there a way to view the
> output of the test (text and image diffs) when the bots run it?

Yes, you can get the results from try bots. However we don't have layout test
bot for Android.

So, I recommend to add the following line to LayoutTests/TestExpectations:
crbug.com/314704 fast/forms/select/menulist-appearance-basic.html [
NeedsRebaseline ]

A bot will add new expectations automatically.

[tkent, 2014-05-27 00:46:08]

-abarth, +keishi

[tkent, 2014-05-27 00:52:10]

https://codereview.chromium.org/302473003/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderMenuList.cpp (right):

https://codereview.chromium.org/302473003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMenuList.cpp:229: if (isHTMLOptionElement(*element))
{
nit: We prefer early exit.  That is to say,
    if (isHTMLOptionElement(*element))
        continue;

https://codereview.chromium.org/302473003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMenuList.cpp:231: if (optionElement->selected()) {
if (toHTMLOptionElement(element)->selected()) {

is shorter.

https://codereview.chromium.org/302473003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMenuList.cpp:234: firstSelectedIndex = i;
if (++selectedCount == 1)
    firstSelectedIndex = i;

is shorter.

https://codereview.chromium.org/302473003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMenuList.cpp:241: ASSERT(0 <= firstSelectedIndex &&
firstSelectedIndex < size);
Do not put two conditions with && into a single ASSERT. We should make two
ASSERTs.
ASSERT(0 <= firstSelectedIndex);
ASSERT(firstSelectedIndex < size);

https://codereview.chromium.org/302473003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderMenuList.cpp:247: Locale& locale = select->locale();
This code applies the else clause for selectedCount==0.  Is it intentional?

[dtrebbien, 2014-05-27 11:48:37]

On 2014/05/27 00:45:41, tkent wrote:
> On 2014/05/24 22:35:34, dtrebbien wrote:
> > For the LayoutTests/fast/forms/select/menulist-appearance-basic.html test,
> there
> > are "expected" screenshots for all platforms that will need to be updated.
> > 
> > I expect that the Android and Windows test bots will fail when running the
> test
> > because I haven't built for those platforms or updated the
> > `menulist-appearance-basic-expected.txt' file.  Is there a way to view the
> > output of the test (text and image diffs) when the bots run it?
> 
> Yes, you can get the results from try bots. However we don't have layout test
> bot for Android.
> 
> So, I recommend to add the following line to LayoutTests/TestExpectations:
> crbug.com/314704 fast/forms/select/menulist-appearance-basic.html [
> NeedsRebaseline ]
> 
> A bot will add new expectations automatically.

In the new patch set I have added a "NeedsRebaseline" line.

For future reference, how do I look at the results from try bots?  When I open,
for example,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/8971 ,
I see in the stdio logs:
    788 [19746/32027] fast/forms/select/menulist-appearance-basic.html failed
unexpectedly (image diff)
    05:07:38.002 7807 worker/0 fast/forms/select/menulist-appearance-basic.html
failed:
    05:07:38.002 7807 worker/0  image diff
.. which was expected, but there is no link to view the image diff.

[dtrebbien, 2014-05-27 11:52:41]

On 2014/05/27 00:52:10, tkent wrote:
>
https://codereview.chromium.org/302473003/diff/1/Source/core/rendering/Render...
> File Source/core/rendering/RenderMenuList.cpp (right):
> 
>
https://codereview.chromium.org/302473003/diff/1/Source/core/rendering/Render...
> Source/core/rendering/RenderMenuList.cpp:229: if
(isHTMLOptionElement(*element))
> {
> nit: We prefer early exit.  That is to say,
>     if (isHTMLOptionElement(*element))
>         continue;

Done.

> 
>
https://codereview.chromium.org/302473003/diff/1/Source/core/rendering/Render...
> Source/core/rendering/RenderMenuList.cpp:231: if (optionElement->selected()) {
> if (toHTMLOptionElement(element)->selected()) {
> 
> is shorter.

Done.

> 
>
https://codereview.chromium.org/302473003/diff/1/Source/core/rendering/Render...
> Source/core/rendering/RenderMenuList.cpp:234: firstSelectedIndex = i;
> if (++selectedCount == 1)
>     firstSelectedIndex = i;
> 
> is shorter.

Done.

> 
>
https://codereview.chromium.org/302473003/diff/1/Source/core/rendering/Render...
> Source/core/rendering/RenderMenuList.cpp:241: ASSERT(0 <= firstSelectedIndex
&&
> firstSelectedIndex < size);
> Do not put two conditions with && into a single ASSERT. We should make two
> ASSERTs.
> ASSERT(0 <= firstSelectedIndex);
> ASSERT(firstSelectedIndex < size);

Done.

> 
>
https://codereview.chromium.org/302473003/diff/1/Source/core/rendering/Render...
> Source/core/rendering/RenderMenuList.cpp:247: Locale& locale =
select->locale();
> This code applies the else clause for selectedCount==0.  Is it intentional?

It is.  For selectedCount==0, I currently have it using the message so that "0
selected" is displayed.  This is like iOS, which displays "0 Items".

Would it be better to display no text like single <select> elements do when no
option is selected?

[tkent, 2014-05-27 23:08:51]

lgtm

https://codereview.chromium.org/302473003/diff/10001/LayoutTests/TestExpectat...
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/302473003/diff/10001/LayoutTests/TestExpectat...
LayoutTests/TestExpectations:1012: crbug.com/314704
fast/forms/select/menulist-appearance-basic.html [ NeedsRebaseline ]
nit: TestExpectations is very unstable.  I recommend adding new line not at the
bottom of the file.

[commit-bot: I haz the power, 2014-05-27 23:09:11]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dtrebbien@gmail.com/302473003/10001

[commit-bot: I haz the power, 2014-05-27 23:09:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-27 23:09:27]

Failed to apply patch for LayoutTests/TestExpectations:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file LayoutTests/TestExpectations
  Hunk #1 FAILED at 1009.
  1 out of 1 hunk FAILED -- saving rejects to file
LayoutTests/TestExpectations.rej

Patch:       LayoutTests/TestExpectations
Index: LayoutTests/TestExpectations
diff --git a/LayoutTests/TestExpectations b/LayoutTests/TestExpectations
index
c83ed7da7eded63306c07343aaf82dbd17ad0011..f7e56fc26781292a627a3b8e54d69795431b11ce
100644
--- a/LayoutTests/TestExpectations
+++ b/LayoutTests/TestExpectations
@@ -1009,3 +1009,4 @@ crbug.com/171639
fast/canvas/canvas-text-ideographic-space.html [ NeedsRebaselin
 crbug.com/171639 virtual/gpu/fast/canvas/canvas-text-ideographic-space.html [
NeedsRebaseline ]
 crbug.com/171639 canvas/philip/tests/2d.text.measure.width.space.html [
NeedsRebaseline ]
 crbug.com/171639
virtual/gpu/canvas/philip/tests/2d.text.measure.width.space.html [
NeedsRebaseline ]
+crbug.com/314704 fast/forms/select/menulist-appearance-basic.html [
NeedsRebaseline ]

[dtrebbien, 2014-05-28 00:00:27]

On 2014/05/27 23:09:27, I haz the power (commit-bot) wrote:
> Failed to apply patch for LayoutTests/TestExpectations:
> While running patch -p1 --forward --force --no-backup-if-mismatch;
>   patching file LayoutTests/TestExpectations
>   Hunk #1 FAILED at 1009.
>   1 out of 1 hunk FAILED -- saving rejects to file
> LayoutTests/TestExpectations.rej
> 
> Patch:       LayoutTests/TestExpectations
> Index: LayoutTests/TestExpectations
> diff --git a/LayoutTests/TestExpectations b/LayoutTests/TestExpectations
> index
>
c83ed7da7eded63306c07343aaf82dbd17ad0011..f7e56fc26781292a627a3b8e54d69795431b11ce
> 100644
> --- a/LayoutTests/TestExpectations
> +++ b/LayoutTests/TestExpectations
> @@ -1009,3 +1009,4 @@ crbug.com/171639
> fast/canvas/canvas-text-ideographic-space.html [ NeedsRebaselin
>  crbug.com/171639 virtual/gpu/fast/canvas/canvas-text-ideographic-space.html [
> NeedsRebaseline ]
>  crbug.com/171639 canvas/philip/tests/2d.text.measure.width.space.html [
> NeedsRebaseline ]
>  crbug.com/171639
> virtual/gpu/canvas/philip/tests/2d.text.measure.width.space.html [
> NeedsRebaseline ]
> +crbug.com/314704 fast/forms/select/menulist-appearance-basic.html [
> NeedsRebaseline ]


I rebased the patch (see Patch Set 3).

[tkent, 2014-05-28 00:01:41]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-05-28 00:01:53]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dtrebbien@gmail.com/302473003/30001

[commit-bot: I haz the power, 2014-05-28 01:45:18]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/9716)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/9378)
  linux_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)

[commit-bot: I haz the power, 2014-05-28 01:45:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-28 01:45:54]

Try jobs failed on following builders:
  linux_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)

[tkent, 2014-05-28 04:20:12]

On 2014/05/27 11:48:37, dtrebbien wrote:
> On 2014/05/27 00:45:41, tkent wrote:
> > On 2014/05/24 22:35:34, dtrebbien wrote:
> > > For the LayoutTests/fast/forms/select/menulist-appearance-basic.html test,
> > there
> > > are "expected" screenshots for all platforms that will need to be updated.
> > > 
> > > I expect that the Android and Windows test bots will fail when running the
> > test
> > > because I haven't built for those platforms or updated the
> > > `menulist-appearance-basic-expected.txt' file.  Is there a way to view the
> > > output of the test (text and image diffs) when the bots run it?
> > 
> > Yes, you can get the results from try bots. However we don't have layout
test
> > bot for Android.
> > 
> > So, I recommend to add the following line to LayoutTests/TestExpectations:
> > crbug.com/314704 fast/forms/select/menulist-appearance-basic.html [
> > NeedsRebaseline ]
> > 
> > A bot will add new expectations automatically.
> 
> In the new patch set I have added a "NeedsRebaseline" line.
> 
> For future reference, how do I look at the results from try bots?  When I
open,
> for example,
> http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/8971
,
> I see in the stdio logs:
>     788 [19746/32027] fast/forms/select/menulist-appearance-basic.html failed
> unexpectedly (image diff)
>     05:07:38.002 7807 worker/0
fast/forms/select/menulist-appearance-basic.html
> failed:
>     05:07:38.002 7807 worker/0  image diff
> .. which was expected, but there is no link to view the image diff.

Click a link with "layout_test_results" in "18. archive_webkit_tests_results"
block.

[tkent, 2014-05-28 04:21:01]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-05-28 04:21:12]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dtrebbien@gmail.com/302473003/30001

[commit-bot: I haz the power, 2014-05-28 04:21:46]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)

[commit-bot: I haz the power, 2014-05-28 04:22:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-28 04:22:12]

Try jobs failed on following builders:
  linux_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)

[tkent, 2014-05-28 09:06:40]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-05-28 09:06:47]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dtrebbien@gmail.com/302473003/30001

[commit-bot: I haz the power, 2014-05-28 09:07:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-28 09:07:02]

Failed to apply patch for LayoutTests/TestExpectations:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file LayoutTests/TestExpectations
  Hunk #1 FAILED at 998.
  1 out of 1 hunk FAILED -- saving rejects to file
LayoutTests/TestExpectations.rej

Patch:       LayoutTests/TestExpectations
Index: LayoutTests/TestExpectations
diff --git a/LayoutTests/TestExpectations b/LayoutTests/TestExpectations
index
5f40022ad11b766e9bae04baeae94cc3b898b8f6..5d2e848c44c838f27d2fd621fcbedc90dee95cfe
100644
--- a/LayoutTests/TestExpectations
+++ b/LayoutTests/TestExpectations
@@ -998,3 +998,5 @@ crbug.com/376534
http/tests/inspector/network/network-xhr-redirect-method.html [
 crbug.com/377013 [ Mac Debug ] fast/repaint/4776765.html [ Pass Failure ]
 
 crbug.com/239722 http/tests/websocket/connection-throttling.html [ Failure ]
+
+crbug.com/314704 fast/forms/select/menulist-appearance-basic.html [
NeedsRebaseline ]

[dtrebbien, 2014-05-28 22:12:56]

On 2014/05/28 09:07:02, I haz the power (commit-bot) wrote:
> Failed to apply patch for LayoutTests/TestExpectations:
> While running patch -p1 --forward --force --no-backup-if-mismatch;
>   patching file LayoutTests/TestExpectations
>   Hunk #1 FAILED at 998.
>   1 out of 1 hunk FAILED -- saving rejects to file
> LayoutTests/TestExpectations.rej
> 
> Patch:       LayoutTests/TestExpectations
> Index: LayoutTests/TestExpectations
> diff --git a/LayoutTests/TestExpectations b/LayoutTests/TestExpectations
> index
>
5f40022ad11b766e9bae04baeae94cc3b898b8f6..5d2e848c44c838f27d2fd621fcbedc90dee95cfe
> 100644
> --- a/LayoutTests/TestExpectations
> +++ b/LayoutTests/TestExpectations
> @@ -998,3 +998,5 @@ crbug.com/376534
> http/tests/inspector/network/network-xhr-redirect-method.html [
>  crbug.com/377013 [ Mac Debug ] fast/repaint/4776765.html [ Pass Failure ]
>  
>  crbug.com/239722 http/tests/websocket/connection-throttling.html [ Failure ]
> +
> +crbug.com/314704 fast/forms/select/menulist-appearance-basic.html [
> NeedsRebaseline ]


Second rebase uploaded.

[tkent, 2014-05-28 22:14:58]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-05-28 22:15:27]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dtrebbien@gmail.com/302473003/50001

[commit-bot: I haz the power, 2014-05-28 23:34:08]

Change committed as 175010