[webkit-changes] [88232] trunk/Source

Tue, 07 Jun 2011 02:44:53 -0700

Title: [88232] trunk/Source
Revision
88232
Author
[email protected]
Date
2011-06-07 02:44:22 -0700 (Tue, 07 Jun 2011)

Log Message

2011-06-07  Naoki Takano  <[email protected]>

        Reviewed by Kent Tamura.

        [Chromium] Click event is not fired for a menulist <select>
        https://bugs.webkit.org/show_bug.cgi?id=60563

        Tests: SelectItemEventFire, SelectItemKeyEvent, SelectItemRemoveSelectOnChange and SelectItemRemoveSelectOnChange in webkit_unit_tests.

        * platform/chromium/PopupMenuChromium.cpp:
        (WebCore::PopupContainer::showPopup): Set m_focusedNode from m_frameView.
        (WebCore::PopupListBox::handleMouseReleaseEvent): Call dispatchMouseEvent to forward the event only if select popup.
        (WebCore::PopupListBox::acceptIndex): Change to return accepted or not.
2011-06-07  Naoki Takano  <[email protected]>

        Reviewed by Kent Tamura.

        [Chromium] Click event is not fired for a menulist <select>
        https://bugs.webkit.org/show_bug.cgi?id=60563

        * tests/PopupMenuTest.cpp:
        (WebKit::TestPopupMenuClient::TestPopupMenuClient): Initialize m_node.
        (WebKit::TestPopupMenuClient::valueChanged): To fire 'change' event, forward the event like RenderMenuList.
        (WebKit::TestPopupMenuClient::itemIsEnabled): Change to return true or false according to disabled item or not.
        (WebKit::TestPopupMenuClient::setDisabledIndex): Set disabled index to simulate disabled item.
        (WebKit::TestPopupMenuClient::setFocusedNode): Set focused node to dispatch the event.
        (WebKit::SelectPopupMenuTest::SelectPopupMenuTest): Add baseURL.
        (WebKit::SelectPopupMenuTest::TearDown): Add UnregisterAllMockedURLs() call.
        (WebKit::SelectPopupMenuTest::registerMockedURLLoad): To simulate html load, call RegisterMockedURL().
        (WebKit::SelectPopupMenuTest::serveRequests): Call ServeAsynchronousMockedRequests().
        (WebKit::SelectPopupMenuTest::loadFrame): Simulate load frame with url string.
        (WebKit::TEST_F): Implement SelectItemEventFire, SelectItemKeyEvent, SelectItemRemoveSelectOnChange and SelectItemRemoveSelectOnChange.
        * tests/data/select_event.html: Added for SelectItemEventFire and SelectItemKeyEvent.
        * tests/data/select_event_remove_on_change.html: Added SelectItemRemoveSelectOnChange.
        * tests/data/select_event_remove_on_click.html: Added SelectItemRemoveSelectOnChange.
        * WebKit.gyp: Added PopupMenuTest.cpp. Because webkit_support dependency is added, we can't build PopupMenuTest.cpp with Chromium-win (shared) configuration.

Modified Paths

Added Paths

Diff

Modified: trunk/Source/WebCore/ChangeLog (88231 => 88232)


--- trunk/Source/WebCore/ChangeLog	2011-06-07 09:39:32 UTC (rev 88231)
+++ trunk/Source/WebCore/ChangeLog	2011-06-07 09:44:22 UTC (rev 88232)
@@ -1,3 +1,17 @@
+2011-06-07  Naoki Takano  <[email protected]>
+
+        Reviewed by Kent Tamura.
+
+        [Chromium] Click event is not fired for a menulist <select>
+        https://bugs.webkit.org/show_bug.cgi?id=60563
+
+        Tests: SelectItemEventFire, SelectItemKeyEvent, SelectItemRemoveSelectOnChange and SelectItemRemoveSelectOnChange in webkit_unit_tests.
+
+        * platform/chromium/PopupMenuChromium.cpp:
+        (WebCore::PopupContainer::showPopup): Set m_focusedNode from m_frameView.
+        (WebCore::PopupListBox::handleMouseReleaseEvent): Call dispatchMouseEvent to forward the event only if select popup.
+        (WebCore::PopupListBox::acceptIndex): Change to return accepted or not.
+
 2011-06-07  Andras Becsi  <[email protected]>
 
         Reviewed by Yury Semikhatsky.

Modified: trunk/Source/WebCore/platform/chromium/PopupMenuChromium.cpp (88231 => 88232)


--- trunk/Source/WebCore/platform/chromium/PopupMenuChromium.cpp	2011-06-07 09:39:32 UTC (rev 88231)
+++ trunk/Source/WebCore/platform/chromium/PopupMenuChromium.cpp	2011-06-07 09:44:22 UTC (rev 88232)
@@ -188,8 +188,8 @@
     void selectIndex(int index);
 
     // Accepts the selected index as the value to be displayed in the <select> widget on
-    // the web page, and closes the popup.
-    void acceptIndex(int index);
+    // the web page, and closes the popup. Returns true if index is accepted.
+    bool acceptIndex(int index);
 
     // Clears the selection (so no row appears selected).
     void clearSelection();
@@ -277,6 +277,9 @@
 
     // If width exeeds screen width, we have to clip it.
     int m_maxWindowWidth;
+
+    // To forward last mouse release event.
+    RefPtr<Node> m_focusedNode;
 };
 
 static PlatformMouseEvent constructRelativeMouseEvent(const PlatformMouseEvent& e,
@@ -407,6 +410,7 @@
 void PopupContainer::showPopup(FrameView* view)
 {
     m_frameView = view;
+    listBox()->m_focusedNode = m_frameView->frame()->document()->focusedNode();
 
     ChromeClientChromium* chromeClient = chromeClientChromium();
     if (chromeClient) {
@@ -679,7 +683,16 @@
     if (!isPointInBounds(event.pos()))
         return true;
 
-    acceptIndex(pointToRowIndex(event.pos()));
+    // Need to check before calling acceptIndex(), because m_popupClient might be removed in acceptIndex() calling because of event handler.
+    bool isSelectPopup = m_popupClient->menuStyle().menuType() == PopupMenuStyle::SelectPopup;
+    if (acceptIndex(pointToRowIndex(event.pos())) && m_focusedNode && isSelectPopup) {
+        m_focusedNode->dispatchMouseEvent(event, eventNames().mouseupEvent);
+        m_focusedNode->dispatchMouseEvent(event, eventNames().clickEvent);
+
+        // Clear m_focusedNode here, because we cannot clear in hidePopup() which is called before dispatchMouseEvent() is called.
+        m_focusedNode = 0;
+    }
+
     return true;
 }
 
@@ -1075,21 +1088,21 @@
     return -1;
 }
 
-void PopupListBox::acceptIndex(int index)
+bool PopupListBox::acceptIndex(int index)
 {
     // Clear m_acceptedIndexOnAbandon once user accepts the selected index.
     if (m_acceptedIndexOnAbandon >= 0)
         m_acceptedIndexOnAbandon = -1;
 
     if (index >= numItems())
-        return;
+        return false;
 
     if (index < 0) {
         if (m_popupClient) {
             // Enter pressed with no selection, just close the popup.
             hidePopup();
         }
-        return;
+        return false;
     }
 
     if (isSelectableItem(index)) {
@@ -1100,7 +1113,11 @@
 
         // Tell the <select> PopupMenuClient what index was selected.
         m_popupClient->valueChanged(index);
+
+        return true;
     }
+
+    return false;
 }
 
 void PopupListBox::selectIndex(int index)

Modified: trunk/Source/WebKit/chromium/ChangeLog (88231 => 88232)


--- trunk/Source/WebKit/chromium/ChangeLog	2011-06-07 09:39:32 UTC (rev 88231)
+++ trunk/Source/WebKit/chromium/ChangeLog	2011-06-07 09:44:22 UTC (rev 88232)
@@ -1,3 +1,27 @@
+2011-06-07  Naoki Takano  <[email protected]>
+
+        Reviewed by Kent Tamura.
+
+        [Chromium] Click event is not fired for a menulist <select>
+        https://bugs.webkit.org/show_bug.cgi?id=60563
+
+        * tests/PopupMenuTest.cpp:
+        (WebKit::TestPopupMenuClient::TestPopupMenuClient): Initialize m_node.
+        (WebKit::TestPopupMenuClient::valueChanged): To fire 'change' event, forward the event like RenderMenuList.
+        (WebKit::TestPopupMenuClient::itemIsEnabled): Change to return true or false according to disabled item or not.
+        (WebKit::TestPopupMenuClient::setDisabledIndex): Set disabled index to simulate disabled item.
+        (WebKit::TestPopupMenuClient::setFocusedNode): Set focused node to dispatch the event.
+        (WebKit::SelectPopupMenuTest::SelectPopupMenuTest): Add baseURL.
+        (WebKit::SelectPopupMenuTest::TearDown): Add UnregisterAllMockedURLs() call.
+        (WebKit::SelectPopupMenuTest::registerMockedURLLoad): To simulate html load, call RegisterMockedURL().
+        (WebKit::SelectPopupMenuTest::serveRequests): Call ServeAsynchronousMockedRequests().
+        (WebKit::SelectPopupMenuTest::loadFrame): Simulate load frame with url string.
+        (WebKit::TEST_F): Implement SelectItemEventFire, SelectItemKeyEvent, SelectItemRemoveSelectOnChange and SelectItemRemoveSelectOnChange.
+        * tests/data/select_event.html: Added for SelectItemEventFire and SelectItemKeyEvent.
+        * tests/data/select_event_remove_on_change.html: Added SelectItemRemoveSelectOnChange.
+        * tests/data/select_event_remove_on_click.html: Added SelectItemRemoveSelectOnChange.
+        * WebKit.gyp: Added PopupMenuTest.cpp. Because webkit_support dependency is added, we can't build PopupMenuTest.cpp with Chromium-win (shared) configuration.
+
 2011-06-06  Nico Weber  <[email protected]>
 
         Reviewed by James Robinson.

Modified: trunk/Source/WebKit/chromium/WebKit.gyp (88231 => 88232)


--- trunk/Source/WebKit/chromium/WebKit.gyp	2011-06-07 09:39:32 UTC (rev 88231)
+++ trunk/Source/WebKit/chromium/WebKit.gyp	2011-06-07 09:44:22 UTC (rev 88232)
@@ -647,6 +647,7 @@
                                 'tests/WebFrameTest.cpp',
                                 'tests/WebPageNewSerializerTest.cpp',
                                 'tests/WebPageSerializerTest.cpp',
+                                'tests/PopupMenuTest.cpp',
                             ],
                             'conditions': [
                                 ['OS=="win" or OS=="mac"', {

Modified: trunk/Source/WebKit/chromium/tests/PopupMenuTest.cpp (88231 => 88232)


--- trunk/Source/WebKit/chromium/tests/PopupMenuTest.cpp	2011-06-07 09:39:32 UTC (rev 88231)
+++ trunk/Source/WebKit/chromium/tests/PopupMenuTest.cpp	2011-06-07 09:44:22 UTC (rev 88232)
@@ -31,19 +31,33 @@
 #include "config.h"
 
 #include <gtest/gtest.h>
+#include <webkit/support/webkit_support.h>
 
 #include "Color.h"
+#include "Element.h"
+#include "FrameView.h"
 #include "KeyboardCodes.h"
 #include "PopupMenu.h"
 #include "PopupMenuClient.h"
 #include "PopupMenuChromium.h"
+#include "SelectElement.h"
+#include "WebDocument.h"
+#include "WebElement.h"
+#include "WebFrame.h"
 #include "WebFrameClient.h"
 #include "WebFrameImpl.h"
 #include "WebInputEvent.h"
 #include "WebPopupMenuImpl.h"
 #include "WebScreenInfo.h"
+#include "WebSettings.h"
+#include "WebString.h"
+#include "WebURL.h"
+#include "WebURLRequest.h"
+#include "WebURLResponse.h"
+#include "WebView.h"
 #include "WebViewClient.h"
 #include "WebViewImpl.h"
+#include "v8.h"
 
 using namespace WebCore;
 using namespace WebKit;
@@ -53,11 +67,15 @@
 class TestPopupMenuClient : public PopupMenuClient {
 public:
     // Item at index 0 is selected by default.
-    TestPopupMenuClient() : m_selectIndex(0) { }
+    TestPopupMenuClient() : m_selectIndex(0), m_node(0) { }
     virtual ~TestPopupMenuClient() {}
     virtual void valueChanged(unsigned listIndex, bool fireEvents = true)
     {
         m_selectIndex = listIndex;
+        if (m_node) {
+            SelectElement* select = toSelectElement(static_cast<Element*>(m_node));
+            select->setSelectedIndexByUser(select->listToOptionIndex(listIndex), true, fireEvents);
+        }
     }
     virtual void selectionChanged(unsigned, bool) {}
     virtual void selectionCleared() {}
@@ -72,7 +90,7 @@
     virtual String itemIcon(unsigned) const { return String(); }
     virtual String itemToolTip(unsigned listIndex) const { return itemText(listIndex); }
     virtual String itemAccessibilityText(unsigned listIndex) const { return itemText(listIndex); }
-    virtual bool itemIsEnabled(unsigned listIndex) const { return true; }
+    virtual bool itemIsEnabled(unsigned listIndex) const { return m_disabledIndexSet.find(listIndex) == m_disabledIndexSet.end(); }
     virtual PopupMenuStyle itemStyle(unsigned listIndex) const
     {
         Font font(FontPlatformData(12.0, false, false), false);
@@ -92,14 +110,19 @@
     virtual bool shouldPopOver() const { return false; }
     virtual bool valueShouldChangeOnHotTrack() const { return false; }
     virtual void setTextFromItem(unsigned listIndex) { }
-    
+
     virtual FontSelector* fontSelector() const { return 0; }
     virtual HostWindow* hostWindow() const { return 0; }
-    
+
     virtual PassRefPtr<Scrollbar> createScrollbar(ScrollableArea*, ScrollbarOrientation, ScrollbarControlSize) { return 0; }
 
+    void setDisabledIndex(unsigned index) { m_disabledIndexSet.insert(index); }
+    void setFocusedNode(Node* node) { m_node = node; }
+
 private:
     unsigned m_selectIndex;
+    std::set<unsigned> m_disabledIndexSet;
+    Node* m_node;
 };
 
 class TestWebWidgetClient : public WebWidgetClient {
@@ -150,6 +173,7 @@
 class SelectPopupMenuTest : public testing::Test {
 public:
     SelectPopupMenuTest()
+        : baseURL("http://www.test.com/")
     {
     }
 
@@ -165,6 +189,7 @@
     {
         m_popupMenu = 0;
         m_webView->close();
+        webkit_support::UnregisterAllMockedURLs();
     }
 
     // Returns true if there currently is a select popup in the WebView.
@@ -220,12 +245,39 @@
         m_webView->selectPopup()->handleMouseReleaseEvent(mouseEvent);
     }
 
+    void registerMockedURLLoad(const std::string& fileName)
+    {
+        WebURLResponse response;
+        response.initialize();
+        response.setMIMEType("text/html");
+
+        std::string filePath = webkit_support::GetWebKitRootDir().utf8();
+        filePath += "/Source/WebKit/chromium/tests/data/popup/";
+        filePath += fileName;
+
+        webkit_support::RegisterMockedURL(WebURL(GURL(baseURL + fileName)), response, WebString::fromUTF8(filePath));
+    }
+
+    void serveRequests()
+    {
+        webkit_support::ServeAsynchronousMockedRequests();
+    }
+
+    void loadFrame(WebFrame* frame, const std::string& fileName)
+    {
+        WebURLRequest urlRequest;
+        urlRequest.initialize();
+        urlRequest.setURL(WebURL(GURL(baseURL + fileName)));
+        frame->loadRequest(urlRequest);
+    }
+
 protected:
     TestWebViewClient m_webviewClient;
     WebViewImpl* m_webView;
     TestWebFrameClient m_webFrameClient;
     TestPopupMenuClient m_popupMenuClient;
     RefPtr<PopupMenu> m_popupMenu;
+    std::string baseURL;
 };
 
 // Tests that show/hide and repeats.  Select popups are reused in web pages when
@@ -350,4 +402,114 @@
     EXPECT_EQ(2, selectedIndex());
 }
 
+TEST_F(SelectPopupMenuTest, SelectItemEventFire)
+{
+    registerMockedURLLoad("select_event.html");
+    m_webView->settings()->setJavaScriptEnabled(true);
+    loadFrame(m_webView->mainFrame(), "select_event.html");
+    serveRequests();
+
+    m_popupMenuClient.setFocusedNode(static_cast<WebFrameImpl*>(m_webView->mainFrame())->frameView()->frame()->document()->focusedNode());
+
+    showPopup();
+
+    int menuHeight = m_webView->selectPopup()->menuItemHeight();
+    // menuHeight * 0.5 means the Y position on the item at index 0.
+    IntPoint row1Point(2, menuHeight * 0.5);
+    simulateLeftMouseDownEvent(row1Point);
+    simulateLeftMouseUpEvent(row1Point);
+
+    WebElement element = m_webView->mainFrame()->document().getElementById("message");
+
+    // mousedown event is held by select node, and we don't simulate the event for the node.
+    // So we can only see mouseup and click event.
+    EXPECT_STREQ("upclick", std::string(element.innerText().utf8()).c_str());
+
+    // Disable the item at index 1.
+    m_popupMenuClient.setDisabledIndex(1);
+
+    showPopup();
+    // menuHeight * 1.5 means the Y position on the item at index 1.
+    row1Point.setY(menuHeight * 1.5);
+    simulateLeftMouseDownEvent(row1Point);
+    simulateLeftMouseUpEvent(row1Point);
+
+    // The item at index 1 is disabled, so the text should not be changed.
+    EXPECT_STREQ("upclick", std::string(element.innerText().utf8()).c_str());
+
+    showPopup();
+    // menuHeight * 2.5 means the Y position on the item at index 2.
+    row1Point.setY(menuHeight * 2.5);
+    simulateLeftMouseDownEvent(row1Point);
+    simulateLeftMouseUpEvent(row1Point);
+
+    // The item is changed to the item at index 2, from index 0, so change event is fired.
+    EXPECT_STREQ("upclickchangeupclick", std::string(element.innerText().utf8()).c_str());
+}
+
+TEST_F(SelectPopupMenuTest, SelectItemKeyEvent)
+{
+    registerMockedURLLoad("select_event.html");
+    m_webView->settings()->setJavaScriptEnabled(true);
+    loadFrame(m_webView->mainFrame(), "select_event.html");
+    serveRequests();
+
+    m_popupMenuClient.setFocusedNode(static_cast<WebFrameImpl*>(m_webView->mainFrame())->frameView()->frame()->document()->focusedNode());
+
+    showPopup();
+
+    // Siumulate to choose the item at index 1 with keyboard.
+    simulateKeyDownEvent(VKEY_DOWN);
+    simulateKeyDownEvent(VKEY_DOWN);
+    simulateKeyDownEvent(VKEY_RETURN);
+
+    WebElement element = m_webView->mainFrame()->document().getElementById("message");
+    // We only can see change event but no other mouse related events.
+    EXPECT_STREQ("change", std::string(element.innerText().utf8()).c_str());
+}
+
+TEST_F(SelectPopupMenuTest, SelectItemRemoveSelectOnChange)
+{
+    // Make sure no crash, even if select node is removed on 'change' event handler.
+    registerMockedURLLoad("select_event_remove_on_change.html");
+    m_webView->settings()->setJavaScriptEnabled(true);
+    loadFrame(m_webView->mainFrame(), "select_event_remove_on_change.html");
+    serveRequests();
+
+    m_popupMenuClient.setFocusedNode(static_cast<WebFrameImpl*>(m_webView->mainFrame())->frameView()->frame()->document()->focusedNode());
+
+    showPopup();
+
+    int menuHeight = m_webView->selectPopup()->menuItemHeight();
+    // menuHeight * 1.5 means the Y position on the item at index 1.
+    IntPoint row1Point(2, menuHeight * 1.5);
+    simulateLeftMouseDownEvent(row1Point);
+    simulateLeftMouseUpEvent(row1Point);
+
+    WebElement element = m_webView->mainFrame()->document().getElementById("message");
+    EXPECT_STREQ("change", std::string(element.innerText().utf8()).c_str());
+}
+
+TEST_F(SelectPopupMenuTest, SelectItemRemoveSelectOnClick)
+{
+    // Make sure no crash, even if select node is removed on 'click' event handler.
+    registerMockedURLLoad("select_event_remove_on_click.html");
+    m_webView->settings()->setJavaScriptEnabled(true);
+    loadFrame(m_webView->mainFrame(), "select_event_remove_on_click.html");
+    serveRequests();
+
+    m_popupMenuClient.setFocusedNode(static_cast<WebFrameImpl*>(m_webView->mainFrame())->frameView()->frame()->document()->focusedNode());
+
+    showPopup();
+
+    int menuHeight = m_webView->selectPopup()->menuItemHeight();
+    // menuHeight * 1.5 means the Y position on the item at index 1.
+    IntPoint row1Point(2, menuHeight * 1.5);
+    simulateLeftMouseDownEvent(row1Point);
+    simulateLeftMouseUpEvent(row1Point);
+
+    WebElement element = m_webView->mainFrame()->document().getElementById("message");
+    EXPECT_STREQ("click", std::string(element.innerText().utf8()).c_str());
+}
+
 } // namespace

Added: trunk/Source/WebKit/chromium/tests/data/popup/select_event.html (0 => 88232)


--- trunk/Source/WebKit/chromium/tests/data/popup/select_event.html	                        (rev 0)
+++ trunk/Source/WebKit/chromium/tests/data/popup/select_event.html	2011-06-07 09:44:22 UTC (rev 88232)
@@ -0,0 +1,33 @@
+<!DOCTYPE html>
+<html>
+<body>
+<div id="message"></div>
+<select id="s" autofocus>
+<option>1</option>
+<option>2</option>
+<option>3</option>
+</select>
+<script>
+function handler(e) {
+  document.getElementById('message').innerText += "down";
+}
+
+function handler2(e) {
+  document.getElementById('message').innerText += "click";
+}
+
+function handler3(e) {
+  document.getElementById('message').innerText += "up";
+}
+
+function handler4(e) {
+  document.getElementById('message').innerText += "change";
+}
+
+document.getElementById('s').addEventListener('mousedown', handler);
+document.getElementById('s').addEventListener('click', handler2);
+document.getElementById('s').addEventListener('mouseup', handler3);
+document.getElementById('s').addEventListener('change', handler4);
+</script>
+</body>
+</html>
\ No newline at end of file

Added: trunk/Source/WebKit/chromium/tests/data/popup/select_event_remove_on_change.html (0 => 88232)


--- trunk/Source/WebKit/chromium/tests/data/popup/select_event_remove_on_change.html	                        (rev 0)
+++ trunk/Source/WebKit/chromium/tests/data/popup/select_event_remove_on_change.html	2011-06-07 09:44:22 UTC (rev 88232)
@@ -0,0 +1,20 @@
+<!DOCTYPE html>
+<html>
+<body>
+<div id="message"></div>
+<select id="s" autofocus>
+<option>1</option>
+<option>2</option>
+<option>3</option>
+</select>
+<script>
+function handler(e) {
+  document.getElementById('message').innerText += "change";
+  select = document.getElementById('s');
+  select.parentNode.removeChild(select);
+}
+
+document.getElementById('s').addEventListener('change', handler);
+</script>
+</body>
+</html>
\ No newline at end of file

Added: trunk/Source/WebKit/chromium/tests/data/popup/select_event_remove_on_click.html (0 => 88232)


--- trunk/Source/WebKit/chromium/tests/data/popup/select_event_remove_on_click.html	                        (rev 0)
+++ trunk/Source/WebKit/chromium/tests/data/popup/select_event_remove_on_click.html	2011-06-07 09:44:22 UTC (rev 88232)
@@ -0,0 +1,20 @@
+<!DOCTYPE html>
+<html>
+<body>
+<div id="message"></div>
+<select id="s" autofocus>
+<option>1</option>
+<option>2</option>
+<option>3</option>
+</select>
+<script>
+function handler(e) {
+  document.getElementById('message').innerText += "click";
+  select = document.getElementById('s');
+  select.parentNode.removeChild(select);
+}
+
+document.getElementById('s').addEventListener('click', handler);
+</script>
+</body>
+</html>
\ No newline at end of file

_______________________________________________
webkit-changes mailing list
[email protected]
http://lists.webkit.org/mailman/listinfo.cgi/webkit-changes

Reply via email to