Spatial Navigation handling of space key in <select> appears to confuse listIndex...
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 19 Jul 2013 19:21:09 +0000 (19:21 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 19 Jul 2013 19:21:09 +0000 (19:21 +0000)
https://bugs.webkit.org/show_bug.cgi?id=99525

Source/WebCore:

HTMLSelect Element inherently contains the logic to focus option node and thus, implementing an explicit logic to find the focus index is not required.

Patch by Abhijeet Kandalkar <abhijeet.k@samsung.com> on 2013-07-19
Reviewed by Joseph Pecoraro.

Test: fast/spatial-navigation/snav-multiple-select-optgroup.html

* html/HTMLSelectElement.cpp:
(WebCore::HTMLSelectElement::listBoxDefaultEventHandler):
* page/SpatialNavigation.cpp:
(WebCore::canScrollInDirection):
* rendering/RenderListBox.cpp:
(WebCore::RenderListBox::addFocusRingRects):

LayoutTests:

Added testcase to test support of option group within HTMLSelect element.

Patch by Abhijeet Kandalkar <abhijeet.k@samsung.com> on 2013-07-19
Reviewed by Joseph Pecoraro.

* fast/spatial-navigation/snav-multiple-select-optgroup-expected.txt: Added.
* fast/spatial-navigation/snav-multiple-select-optgroup.html: Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@152919 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/fast/spatial-navigation/snav-multiple-select-optgroup-expected.txt [new file with mode: 0644]
LayoutTests/fast/spatial-navigation/snav-multiple-select-optgroup.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/html/HTMLSelectElement.cpp
Source/WebCore/page/SpatialNavigation.cpp
Source/WebCore/rendering/RenderListBox.cpp

index 3df208b..d325137 100644 (file)
@@ -1,3 +1,15 @@
+2013-07-19  Abhijeet Kandalkar  <abhijeet.k@samsung.com>
+
+        Spatial Navigation handling of space key in <select> appears to confuse listIndex and optionIndex.
+        https://bugs.webkit.org/show_bug.cgi?id=99525
+
+        Added testcase to test support of option group within HTMLSelect element.
+
+        Reviewed by Joseph Pecoraro.
+
+        * fast/spatial-navigation/snav-multiple-select-optgroup-expected.txt: Added.
+        * fast/spatial-navigation/snav-multiple-select-optgroup.html: Added.
+
 2013-07-19  David Hyatt  <hyatt@apple.com>
 
         OSX: ePub: Unable to select text in vertical Japanese book
diff --git a/LayoutTests/fast/spatial-navigation/snav-multiple-select-optgroup-expected.txt b/LayoutTests/fast/spatial-navigation/snav-multiple-select-optgroup-expected.txt
new file mode 100644 (file)
index 0000000..c19c1f7
--- /dev/null
@@ -0,0 +1,53 @@
+2      
+4              6
+8      
+PASS gFocusedDocument.activeElement.getAttribute("id") is "start"
+PASS gFocusedDocument.activeElement.getAttribute("id") is "start"
+PASS gFocusedDocument.activeElement.getAttribute("id") is "start"
+PASS gFocusedDocument.activeElement.getAttribute("id") is "8"
+PASS gFocusedDocument.activeElement.getAttribute("id") is "start"
+PASS gFocusedDocument.activeElement.getAttribute("id") is "start"
+PASS gFocusedDocument.activeElement.getAttribute("id") is "start"
+PASS gFocusedDocument.activeElement.getAttribute("id") is "2"
+PASS gFocusedDocument.activeElement.getAttribute("id") is "start"
+PASS gFocusedDocument.activeElement.getAttribute("id") is "6"
+PASS gFocusedDocument.activeElement.getAttribute("id") is "start"
+PASS gFocusedDocument.activeElement.getAttribute("id") is "4"
+PASS gFocusedDocument.activeElement.getAttribute("id") is "start"
+PASS gFocusedDocument.getElementById("start").options[0].selected is false
+PASS gFocusedDocument.getElementById("start").options[1].selected is false
+PASS gFocusedDocument.getElementById("start").options[2].selected is false
+PASS gFocusedDocument.getElementById("start").options[3].selected is false
+PASS gFocusedDocument.getElementById("start").options[0].selected is false
+PASS gFocusedDocument.getElementById("start").options[1].selected is false
+PASS gFocusedDocument.getElementById("start").options[2].selected is false
+PASS gFocusedDocument.getElementById("start").options[3].selected is false
+PASS gFocusedDocument.getElementById("start").options[0].selected is false
+PASS gFocusedDocument.getElementById("start").options[1].selected is true
+PASS gFocusedDocument.getElementById("start").options[2].selected is false
+PASS gFocusedDocument.getElementById("start").options[3].selected is false
+PASS gFocusedDocument.getElementById("start").options[0].selected is false
+PASS gFocusedDocument.getElementById("start").options[1].selected is true
+PASS gFocusedDocument.getElementById("start").options[2].selected is false
+PASS gFocusedDocument.getElementById("start").options[3].selected is false
+PASS gFocusedDocument.getElementById("start").options[0].selected is false
+PASS gFocusedDocument.getElementById("start").options[1].selected is true
+PASS gFocusedDocument.getElementById("start").options[2].selected is false
+PASS gFocusedDocument.getElementById("start").options[3].selected is true
+PASS gFocusedDocument.getElementById("start").options[0].selected is false
+PASS gFocusedDocument.getElementById("start").options[1].selected is true
+PASS gFocusedDocument.getElementById("start").options[2].selected is false
+PASS gFocusedDocument.getElementById("start").options[3].selected is true
+PASS gFocusedDocument.getElementById("start").options[0].selected is false
+PASS gFocusedDocument.getElementById("start").options[1].selected is false
+PASS gFocusedDocument.getElementById("start").options[2].selected is false
+PASS gFocusedDocument.getElementById("start").options[3].selected is true
+PASS gFocusedDocument.getElementById("start").options[0].selected is false
+PASS gFocusedDocument.getElementById("start").options[1].selected is false
+PASS gFocusedDocument.getElementById("start").options[2].selected is false
+PASS gFocusedDocument.getElementById("start").options[3].selected is true
+PASS gFocusedDocument.getElementById("start").options[0].selected is false
+PASS gFocusedDocument.getElementById("start").options[1].selected is true
+PASS gFocusedDocument.getElementById("start").options[2].selected is false
+PASS gFocusedDocument.getElementById("start").options[3].selected is true
+
diff --git a/LayoutTests/fast/spatial-navigation/snav-multiple-select-optgroup.html b/LayoutTests/fast/spatial-navigation/snav-multiple-select-optgroup.html
new file mode 100644 (file)
index 0000000..1f8bcac
--- /dev/null
@@ -0,0 +1,117 @@
+<html>
+  <!--
+    This test ensures the correctness of Spatial Navigation (SNav) algorithm over multiple select element.
+
+    * Pre-conditions:
+    1) TestRunner support for SNav enable/disable.
+
+    * Navigation steps:
+    1) Loads this page, focus goes to "start" automatically.
+    2) Focus moves away from select in 4 different directions to neighbor nodes and back.
+  -->
+  <head>
+    <script src="../js/resources/js-test-pre.js"></script>
+    <script src="resources/spatial-navigation-utils.js"></script>
+    <script type="application/javascript">
+
+    var resultMap = [
+      ["Down", "start"],
+      ["Down", "start"],
+      ["Down", "start"],
+      ["Down", "8"],
+      ["Up", "start"],
+      ["Up", "start"],
+      ["Up", "start"],
+      ["Up", "2"],
+      ["Down", "start"],
+      ["Right", "6"],
+      ["Left", "start"],
+      ["Left", "4"],
+      ["Right", "start"],
+      ["DONE", "DONE"]
+    ];
+
+    if (window.testRunner) {
+      testRunner.dumpAsText();
+      testRunner.setSpatialNavigationEnabled(true);
+      testRunner.overridePreference("WebKitTabToLinksPreferenceKey", 1);
+      testRunner.waitUntilDone();
+    }
+
+    function runTest()
+    {
+      // starting the test itself: get to a known place.
+      document.getElementById("start").focus();
+
+      initTest(resultMap, additionalTest);
+    }
+
+    function sendKeyAndCheckOptions(option0, option1, option2, option3)
+    {
+      shouldBe("gFocusedDocument.getElementById(\"start\").options[0].selected", option0);
+      shouldBe("gFocusedDocument.getElementById(\"start\").options[1].selected", option1);
+      shouldBe("gFocusedDocument.getElementById(\"start\").options[2].selected", option2);
+      shouldBe("gFocusedDocument.getElementById(\"start\").options[3].selected", option3);
+    }
+
+    function additionalTest()
+    {
+      document.getElementById("start").focus();         //move to 1st item
+      sendKeyAndCheckOptions("false", "false", "false", "false");
+      eventSender.keyDown("downArrow");                 //move to 2nd item
+      sendKeyAndCheckOptions("false", "false", "false", "false");
+      eventSender.keyDown(" ");                         //nothing should change
+      sendKeyAndCheckOptions("false", "true", "false", "false");
+      eventSender.keyDown("downArrow");                 //move to 4th item (3rd item is disabled) 
+      sendKeyAndCheckOptions("false", "true", "false", "false");
+      eventSender.keyDown(" ");                         //nothing should change
+      sendKeyAndCheckOptions("false", "true", "false", "true");
+      eventSender.keyDown("upArrow");                   //move back to 2nd item
+      sendKeyAndCheckOptions("false", "true", "false", "true");
+      eventSender.keyDown(" ");                         //noting should change
+      sendKeyAndCheckOptions("false", "false", "false", "true");
+      eventSender.keyDown("upArrow");                   //move back to 1st item
+      sendKeyAndCheckOptions("false", "false", "false", "true");
+      eventSender.keyDown("downArrow", ["shiftKey"]);   //shift-down to 2nd item
+      sendKeyAndCheckOptions("false", "true", "false", "true");
+
+      testCompleted();
+    }
+
+    function testCompleted()
+    {
+      if (window.testRunner)
+        testRunner.notifyDone();
+    }
+
+    window.onload = runTest;
+
+    </script>
+    <script src="js/resources/js-test-post.js"></script>
+  </head>
+  <body id="some-content" xmlns="http://www.w3.org/1999/xhtml">
+    <table style="text-align: left; width: 100%; margin-left: auto; margin-right: auto;" border="1" cellpadding="2" cellspacing="1">
+      <tbody>
+        <tr>
+          <td style="vertical-align: top; text-align: center;"></td>
+          <td style="vertical-align: top; text-align: center;"><a id="2" href="a">2</a></td>
+          <td style="vertical-align: top; text-align: center;"></td>
+        </tr>
+        <tr>
+          <td style="vertical-align: top; text-align: center;"><a id="4" href="a">4</a></td>
+          <td style="vertical-align: top; text-align: center;">
+                 <select id="start" multiple><optgroup label="o1"><option>1</option></optgroup><optgroup label="o2"><option>2</option></optgroup><optgroup label="o3"><option disabled>3</option></optgroup><optgroup label="o4"><option>4</option></optgroup>
+                 </select></td>
+          <td style="vertical-align: top; text-align: center;"><a id="6" href="a">6</a></td>
+        </tr>
+        <tr>
+          <td style="vertical-align: top; text-align: center;"></td>
+          <td style="vertical-align: top; text-align: center;"><a id="8" href="a">8</a></td>
+          <td style="vertical-align: top; text-align: center;"></td>
+        </tr>
+      </tbody>
+    </table>
+    <div id="console"></div>
+  </body>
+</html>
+
index ebb6982..bd81dd3 100644 (file)
@@ -1,3 +1,21 @@
+2013-07-19  Abhijeet Kandalkar  <abhijeet.k@samsung.com>
+
+        Spatial Navigation handling of space key in <select> appears to confuse listIndex and optionIndex.
+        https://bugs.webkit.org/show_bug.cgi?id=99525
+
+        HTMLSelect Element inherently contains the logic to focus option node and thus, implementing an explicit logic to find the focus index is not required.
+
+        Reviewed by Joseph Pecoraro.
+
+        Test: fast/spatial-navigation/snav-multiple-select-optgroup.html
+
+        * html/HTMLSelectElement.cpp:
+        (WebCore::HTMLSelectElement::listBoxDefaultEventHandler):
+        * page/SpatialNavigation.cpp:
+        (WebCore::canScrollInDirection):
+        * rendering/RenderListBox.cpp:
+        (WebCore::RenderListBox::addFocusRingRects):
+
 2013-07-19  David Hyatt  <hyatt@apple.com>
 
         OSX: ePub: Unable to select text in vertical Japanese book
index c2963ac..a437b37 100644 (file)
@@ -1452,7 +1452,10 @@ void HTMLSelectElement::listBoxDefaultEventHandler(Event* event)
         } else if (m_multiple && keyCode == ' ' && isSpatialNavigationEnabled(document()->frame())) {
             // Use space to toggle selection change.
             m_activeSelectionState = !m_activeSelectionState;
-            updateSelectedState(listToOptionIndex(m_activeSelectionEndIndex), true /*multi*/, false /*shift*/);
+            ASSERT(m_activeSelectionEndIndex >= 0
+                && m_activeSelectionEndIndex < static_cast<int>(listItems.size())
+                && listItems[m_activeSelectionEndIndex]->hasTagName(optionTag));
+            updateSelectedState(m_activeSelectionEndIndex, true /*multi*/, false /*shift*/);
             listBoxOnChange();
             event->setDefaultHandled();
         }
index 6f277a6..240ca91 100644 (file)
@@ -448,6 +448,10 @@ Node* scrollableEnclosingBoxOrParentFrameForNodeInDirection(FocusDirection direc
 bool canScrollInDirection(const Node* container, FocusDirection direction)
 {
     ASSERT(container);
+
+    if (container->hasTagName(HTMLNames::selectTag))
+        return false;
+
     if (container->isDocumentNode())
         return canScrollInDirection(toDocument(container)->frame(), direction);
 
index 1fbf819..0502c6d 100644 (file)
@@ -348,6 +348,7 @@ void RenderListBox::addFocusRingRects(Vector<IntRect>& rects, const LayoutPoint&
     for (int i = 0; i < size; ++i) {
         HTMLElement* element = listItems[i];
         if (isHTMLOptionElement(element) && !element->isDisabledFormControl()) {
+            select->setActiveSelectionEndIndex(i);
             rects.append(pixelSnappedIntRect(itemBoundingBoxRect(additionalOffset, i)));
             return;
         }