LayoutTests/fast/events/spatial-navigation/snav-multiple-select.html fails

• Component: DOM
• Status: RESOLVED FIXED

[Chang Shu]

The above test case fails when running the test case from a browser. It was working in DRT by mistake. After bug 48134 is fixed, the failure is unveiled.

[Yael]

I think this test should be removed. The support I added for multi select is only for the case when ENABLE_NO_LISTBOX_RENDERING is defined, and we cannot test that in a layout test. 
Due to the bug in the test infrastructure, I did not catch this. (sorry about that :-).

We already have manual tests that can test multi-select with ENABLE_NO_LISTBOX_RENDERING turned on, and we already have a bug report for adding support when the flag is not turned on https://bugs.webkit.org/show_bug.cgi?id=47094 .

[Chang Shu]

Created attachment 72502 [details]
fix patch

[Chang Shu]

(In reply to comment #1)
> I think this test should be removed. The support I added for multi select is only for the case when ENABLE_NO_LISTBOX_RENDERING is defined, and we cannot test that in a layout test. 
> Due to the bug in the test infrastructure, I did not catch this. (sorry about that :-).
> 
> We already have manual tests that can test multi-select with ENABLE_NO_LISTBOX_RENDERING turned on, and we already have a bug report for adding support when the flag is not turned on https://bugs.webkit.org/show_bug.cgi?id=47094 .

This test can still be valid for 47094.

[WebKit Review Bot]

Attachment 72502 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/dom/SelectElement.cpp:768:  Missing spaces around |  [whitespace/operators] [3]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yael]

(In reply to comment #3)
> (In reply to comment #1)
> > I think this test should be removed. The support I added for multi select is only for the case when ENABLE_NO_LISTBOX_RENDERING is defined, and we cannot test that in a layout test. 
> > Due to the bug in the test infrastructure, I did not catch this. (sorry about that :-).
> > 
> > We already have manual tests that can test multi-select with ENABLE_NO_LISTBOX_RENDERING turned on, and we already have a bug report for adding support when the flag is not turned on https://bugs.webkit.org/show_bug.cgi?id=47094 .
> 
> This test can still be valid for 47094.

After this patch, can the user navigate out of the select list _without_ changing the selected item?

[Antonio Gomes]

Comment on attachment 72502 [details]
fix patch

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

Thanks for fixing this!

> WebCore/dom/SelectElement.cpp:768
> +                if (keyIdentifier == "Left" || keyIdentifier == "Right"|| ((keyIdentifier == "Down" || keyIdentifier == "Up") && endIndex == data.activeSelectionEndIndex()))

Maybe we could have a helper method for this part

(keyIdentifier == "Down" || keyIdentifier == "Up") && endIndex == data.activeSelectionEndIndex()

just to make it clear what is going on?

[Chang Shu]

(In reply to comment #5)
> (In reply to comment #3)
> > (In reply to comment #1)
> > > I think this test should be removed. The support I added for multi select is only for the case when ENABLE_NO_LISTBOX_RENDERING is defined, and we cannot test that in a layout test. 
> > > Due to the bug in the test infrastructure, I did not catch this. (sorry about that :-).
> > > 
> > > We already have manual tests that can test multi-select with ENABLE_NO_LISTBOX_RENDERING turned on, and we already have a bug report for adding support when the flag is not turned on https://bugs.webkit.org/show_bug.cgi?id=47094 .
> > 
> > This test can still be valid for 47094.
> 
> After this patch, can the user navigate out of the select list _without_ changing the selected item?


No. A complete support will follow up and probably attach to 47094.

[Chang Shu]

(In reply to comment #6)
> (From update of attachment 72502 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=72502&action=review
> 
> Thanks for fixing this!
> 
> > WebCore/dom/SelectElement.cpp:768
> > +                if (keyIdentifier == "Left" || keyIdentifier == "Right"|| ((keyIdentifier == "Down" || keyIdentifier == "Up") && endIndex == data.activeSelectionEndIndex()))
> 
> Maybe we could have a helper method for this part
> 
> (keyIdentifier == "Down" || keyIdentifier == "Up") && endIndex == data.activeSelectionEndIndex()
> 
> just to make it clear what is going on?

Thanks for the review. I will change accordingly.

[Chang Shu]

Created attachment 72517 [details]
fix patch 2

Fixed coding style and added comments.

[Antonio Gomes]

Comment on attachment 72517 [details]
fix patch 2

r=me

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 72517 [details]:

fast/events/platform-wheelevent-in-scrolling-div.html
fast/text/international/vertical-text-glyph-test.html

Please file bugs against the tests.  These tests were authored by [e-mail] and [e-mail].  The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 72517 [details]
fix patch 2

Clearing flags on attachment: 72517

Committed r71094: <http://trac.webkit.org/changeset/71094>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.