Bug 48145

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 .

Created attachment 72502 [details] fix patch

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

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.

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

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?

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

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

Created attachment 72517 [details] fix patch 2 Fixed coding style and added comments.

Comment on attachment 72517 [details] fix patch 2 r=me

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 aestes@apple.com and jamesr@chromium.org. The commit-queue is continuing to process your patch.

Comment on attachment 72517 [details] fix patch 2 Clearing flags on attachment: 72517 Committed r71094: <http://trac.webkit.org/changeset/71094>

All reviewed patches have been landed. Closing bug.