[macOS] Select element doesn't show popup if select element had lost focus while...
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 28 Mar 2019 06:22:42 +0000 (06:22 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 28 Mar 2019 06:22:42 +0000 (06:22 +0000)
https://bugs.webkit.org/show_bug.cgi?id=196336

Reviewed by Tim Horton.

Source/WebCore:

* rendering/RenderMenuList.cpp:
(RenderMenuList::popupDidHide): Added a comment.

Source/WebKit:

The bug was caused by WebPopupMenu::hide never notifying PopupClient that the popup had been dismissed.
This resulted in RenderMenuList::m_popupIsVisible to be never reset.

Also fixed a bug in WebPopupMenuProxyMac::hidePopupMenu that this function was never dismissing
the popup as the selector "dismissPopUp", on the contrary to its name, does not dimiss the popup.
Send cancelTracking to NSMenu instead, which DOES dismiss the popup.

Tests: fast/forms/select/mac-wk2/blur-dismisses-select-popup.html
       fast/forms/select/mac-wk2/open-select-popup-after-dismissing-by-blur.html

* UIProcess/mac/WebPopupMenuProxyMac.mm:
(WebKit::WebPopupMenuProxyMac::hidePopupMenu):
* WebProcess/WebCoreSupport/WebPopupMenu.cpp:
(WebKit::WebPopupMenu::hide):

Source/WebKitLegacy/mac:

Fixed the bug that we were not actually dismissing the popup in PopupMenuMac::hide as done in WebKit2.

Unfortunately no new tests since intenals.isSelectPopupVisible would always return false in WebKit1.

* WebCoreSupport/PopupMenuMac.mm:
(PopupMenuMac::hide):

LayoutTests:

Added regression tests for dismissing the select element's popup menu by bluring the element then re-opening the popup.
Unfortunately these tests are only enabled in WebKit2 since intenals.isSelectPopupVisible would always return false in WebKit1.

* TestExpectations:
* fast/forms/select/mac-wk2: Added.
* fast/forms/select/mac-wk2/blur-dismisses-select-popup-expected.html: Added.
* fast/forms/select/mac-wk2/blur-dismisses-select-popup.html: Added.
* fast/forms/select/mac-wk2/open-select-popup-after-dismissing-by-blur-expected.txt: Added.
* fast/forms/select/mac-wk2/open-select-popup-after-dismissing-by-blur.html: Added.
* platform/mac-wk2/TestExpectations:

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

14 files changed:
LayoutTests/ChangeLog
LayoutTests/TestExpectations
LayoutTests/fast/forms/select/mac-wk2/blur-dismisses-select-popup-expected.html [new file with mode: 0644]
LayoutTests/fast/forms/select/mac-wk2/blur-dismisses-select-popup.html [new file with mode: 0644]
LayoutTests/fast/forms/select/mac-wk2/open-select-popup-after-dismissing-by-blur-expected.txt [new file with mode: 0644]
LayoutTests/fast/forms/select/mac-wk2/open-select-popup-after-dismissing-by-blur.html [new file with mode: 0644]
LayoutTests/platform/mac-wk2/TestExpectations
Source/WebCore/ChangeLog
Source/WebCore/rendering/RenderMenuList.cpp
Source/WebKit/ChangeLog
Source/WebKit/UIProcess/mac/WebPopupMenuProxyMac.mm
Source/WebKit/WebProcess/WebCoreSupport/WebPopupMenu.cpp
Source/WebKitLegacy/mac/ChangeLog
Source/WebKitLegacy/mac/WebCoreSupport/PopupMenuMac.mm

index 6afc609..cd65382 100644 (file)
@@ -1,3 +1,21 @@
+2019-03-27  Ryosuke Niwa  <rniwa@webkit.org>
+
+        [macOS] Select element doesn't show popup if select element had lost focus while popup was previosuly shown
+        https://bugs.webkit.org/show_bug.cgi?id=196336
+
+        Reviewed by Tim Horton.
+
+        Added regression tests for dismissing the select element's popup menu by bluring the element then re-opening the popup.
+        Unfortunately these tests are only enabled in WebKit2 since intenals.isSelectPopupVisible would always return false in WebKit1.
+
+        * TestExpectations:
+        * fast/forms/select/mac-wk2: Added.
+        * fast/forms/select/mac-wk2/blur-dismisses-select-popup-expected.html: Added.
+        * fast/forms/select/mac-wk2/blur-dismisses-select-popup.html: Added.
+        * fast/forms/select/mac-wk2/open-select-popup-after-dismissing-by-blur-expected.txt: Added.
+        * fast/forms/select/mac-wk2/open-select-popup-after-dismissing-by-blur.html: Added.
+        * platform/mac-wk2/TestExpectations:
+
 2019-03-27  Alicia Boya GarcĂ­a  <aboya@igalia.com>
 
         [GTK] Unreviewed test gardening
index 3f7c4d8..7641823 100644 (file)
@@ -23,6 +23,7 @@ editing/undo-manager [ Skip ]
 tiled-drawing [ Skip ]
 fast/css/watchos [ Skip ]
 fast/dom/Window/watchos [ Skip ]
+fast/forms/select/mac-wk2 [ Skip ]
 fast/forms/textarea/ios [ Skip ]
 fast/forms/watchos [ Skip ]
 fast/viewport/watchos [ Skip ]
diff --git a/LayoutTests/fast/forms/select/mac-wk2/blur-dismisses-select-popup-expected.html b/LayoutTests/fast/forms/select/mac-wk2/blur-dismisses-select-popup-expected.html
new file mode 100644 (file)
index 0000000..838e932
--- /dev/null
@@ -0,0 +1,12 @@
+<!DOCTYPE html>
+<html>
+<body>
+<p tabindex="1">This tests moving the focus from a select element while its popup menu is open. WebKit should dismiss the popup.</p>
+<select onmousedown="moveFocus()">
+  <option>Click here</option>
+  <option>You should see this</option>
+</select>
+<div id="result">PASS</div>
+<script> document.querySelector('p').focus(); </script>
+</body>
+</html>
diff --git a/LayoutTests/fast/forms/select/mac-wk2/blur-dismisses-select-popup.html b/LayoutTests/fast/forms/select/mac-wk2/blur-dismisses-select-popup.html
new file mode 100644 (file)
index 0000000..bcb0f17
--- /dev/null
@@ -0,0 +1,39 @@
+<!DOCTYPE html>
+<html>
+<body>
+<p tabindex="1">This tests moving the focus from a select element while its popup menu is open. WebKit should dismiss the popup.</p>
+<select>
+  <option>Click here</option>
+  <option>You should see this</option>
+</select>
+<div id="result"></div>
+<script>
+
+const select = document.querySelector('select');
+
+if (window.testRunner)
+    testRunner.waitUntilDone();
+else {
+    select.addEventListener('mousedown', () => {
+        setTimeout(() => document.querySelector('p').focus(), 0);
+    });
+}
+
+window.onload = () => {
+    const event = document.createEvent("MouseEvent");
+    event.initMouseEvent("mousedown", true, true, document.defaultView, 1, select.offsetLeft + 5, select.offsetTop + 5, select.offsetLeft + 5, select.offsetTop + 5, false, false, false, false, 0, document);
+    select.dispatchEvent(event);
+    
+    if (!window.testRunner)
+        return;
+
+    testRunner.runUIScript(`uiController.uiScriptComplete()`, () => {
+        document.querySelector('p').focus();
+        document.getElementById('result').textContent = internals.isSelectPopupVisible(select) ? 'FAIL' : 'PASS';
+        testRunner.notifyDone();
+    });
+}
+
+</script>
+</body>
+</html>
diff --git a/LayoutTests/fast/forms/select/mac-wk2/open-select-popup-after-dismissing-by-blur-expected.txt b/LayoutTests/fast/forms/select/mac-wk2/open-select-popup-after-dismissing-by-blur-expected.txt
new file mode 100644 (file)
index 0000000..6e45718
--- /dev/null
@@ -0,0 +1,7 @@
+This tests re-opening the select element popup after closing it by bluring the focus. WebKit should re-open the popup intead of hitting a debug assert.
+To manually test, after the popup which opened as this test is dismissed (click elsewhere to dismiss it if not). Clicking on the select element should then open the popup menu.
+
+
+PASS - popup closed by blur
+PASS - popup opened after closed by blur
+
diff --git a/LayoutTests/fast/forms/select/mac-wk2/open-select-popup-after-dismissing-by-blur.html b/LayoutTests/fast/forms/select/mac-wk2/open-select-popup-after-dismissing-by-blur.html
new file mode 100644 (file)
index 0000000..5db525b
--- /dev/null
@@ -0,0 +1,66 @@
+<!DOCTYPE html>
+<html>
+<body>
+<p tabindex="1">This tests re-opening the select element popup after closing it by bluring the focus.
+WebKit should re-open the popup intead of hitting a debug assert.<br>
+To manually test, after the popup which opened as this test is dismissed (click elsewhere to dismiss it if not).
+Clicking on the select element should then open the popup menu.</p>
+<select>
+  <option>Click here</option>
+  <option>You should see this</option>
+</select>
+<pre id="result"></pre>
+<script>
+
+const select = document.querySelector('select');
+
+if (window.testRunner) {
+    testRunner.waitUntilDone();
+    testRunner.dumpAsText();
+} else {
+    let count = 0;
+    select.addEventListener('mousedown', () => {
+        if (count++)
+            return;
+        setTimeout(() => document.querySelector('p').focus(), 0);
+    });
+}
+
+function log(text) {
+    document.getElementById('result').textContent += text + '\n';
+}
+
+function clickOnSelectElement() {
+    const event = document.createEvent("MouseEvent");
+    event.initMouseEvent("mousedown", true, true, document.defaultView, 1, select.offsetLeft + 5, select.offsetTop + 5, select.offsetLeft + 5, select.offsetTop + 5, false, false, false, false, 0, document);
+    select.dispatchEvent(event);
+}
+
+function roundTripToUIProcess() {
+    return new Promise((resolve) => {
+        testRunner.runUIScript(`uiController.uiScriptComplete()`, resolve);
+    });
+}
+
+window.onload = async () => {
+    clickOnSelectElement();
+
+    if (!window.testRunner)
+        return;
+
+    await roundTripToUIProcess();
+    document.querySelector('p').focus();
+    log(internals.isSelectPopupVisible(select) ? 'FAIL - popup was open after moving the focus' : 'PASS - popup closed by blur');
+
+    clickOnSelectElement();
+    log(internals.isSelectPopupVisible(select) ? 'PASS - popup opened after closed by blur' : 'FAIL - popup failed to open for the second time');
+
+    await roundTripToUIProcess();
+    document.querySelector('p').focus();
+
+    testRunner.notifyDone();
+}
+
+</script>
+</body>
+</html>
index d315bc5..e1d24db 100644 (file)
@@ -7,6 +7,7 @@
 
 editing/find [ Pass ]
 editing/undo-manager [ Pass ]
+fast/forms/select/mac-wk2 [ Pass ]
 fast/visual-viewport/tiled-drawing [ Pass ]
 fast/web-share [ Pass ]
 scrollingcoordinator [ Pass ]
index e190b88..804523b 100644 (file)
@@ -1,3 +1,13 @@
+2019-03-27  Ryosuke Niwa  <rniwa@webkit.org>
+
+        [macOS] Select element doesn't show popup if select element had lost focus while popup was previosuly shown
+        https://bugs.webkit.org/show_bug.cgi?id=196336
+
+        Reviewed by Tim Horton.
+
+        * rendering/RenderMenuList.cpp:
+        (RenderMenuList::popupDidHide): Added a comment.
+
 2019-03-27  Justin Fan  <justin_fan@apple.com>
 
         [Web GPU] Standardize Web GPU object reference counting and creation logic
index 89312de..02f1cbf 100644 (file)
@@ -612,6 +612,7 @@ int RenderMenuList::selectedIndex() const
 void RenderMenuList::popupDidHide()
 {
 #if !PLATFORM(IOS_FAMILY)
+    // PopupMenuMac::show in WebKitLegacy can call this callback even when popup had already been dismissed.
     m_popupIsVisible = false;
 #endif
 }
index 01878e8..498f3eb 100644 (file)
@@ -1,3 +1,25 @@
+2019-03-27  Ryosuke Niwa  <rniwa@webkit.org>
+
+        [macOS] Select element doesn't show popup if select element had lost focus while popup was previosuly shown
+        https://bugs.webkit.org/show_bug.cgi?id=196336
+
+        Reviewed by Tim Horton.
+
+        The bug was caused by WebPopupMenu::hide never notifying PopupClient that the popup had been dismissed.
+        This resulted in RenderMenuList::m_popupIsVisible to be never reset.
+
+        Also fixed a bug in WebPopupMenuProxyMac::hidePopupMenu that this function was never dismissing
+        the popup as the selector "dismissPopUp", on the contrary to its name, does not dimiss the popup.
+        Send cancelTracking to NSMenu instead, which DOES dismiss the popup.
+
+        Tests: fast/forms/select/mac-wk2/blur-dismisses-select-popup.html
+               fast/forms/select/mac-wk2/open-select-popup-after-dismissing-by-blur.html
+
+        * UIProcess/mac/WebPopupMenuProxyMac.mm:
+        (WebKit::WebPopupMenuProxyMac::hidePopupMenu):
+        * WebProcess/WebCoreSupport/WebPopupMenu.cpp:
+        (WebKit::WebPopupMenu::hide):
+
 2019-03-27  Dean Jackson  <dino@apple.com>
 
         [ARKit] Black view when opening a 3D model usdz file in new tab
index 684b136..e0911b8 100644 (file)
@@ -203,7 +203,7 @@ ALLOW_DEPRECATED_DECLARATIONS_END
 
 void WebPopupMenuProxyMac::hidePopupMenu()
 {
-    [m_popup dismissPopUp];
+    [[m_popup menu] cancelTracking];
 }
 
 void WebPopupMenuProxyMac::cancelTracking()
index 97fe97a..d16fe85 100644 (file)
@@ -120,7 +120,8 @@ void WebPopupMenu::hide()
         return;
 
     WebProcess::singleton().parentProcessConnection()->send(Messages::WebPageProxy::HidePopupMenu(), m_page->pageID());
-    m_page->setActivePopupMenu(0);
+    m_page->setActivePopupMenu(nullptr);
+    m_popupClient->popupDidHide();
 }
 
 void WebPopupMenu::updateFromElement()
index 98b02c8..ded8cc5 100644 (file)
@@ -1,3 +1,17 @@
+2019-03-27  Ryosuke Niwa  <rniwa@webkit.org>
+
+        [macOS] Select element doesn't show popup if select element had lost focus while popup was previosuly shown
+        https://bugs.webkit.org/show_bug.cgi?id=196336
+
+        Reviewed by Tim Horton.
+
+        Fixed the bug that we were not actually dismissing the popup in PopupMenuMac::hide as done in WebKit2.
+
+        Unfortunately no new tests since intenals.isSelectPopupVisible would always return false in WebKit1.
+
+        * WebCoreSupport/PopupMenuMac.mm:
+        (PopupMenuMac::hide):
+
 2019-03-25  Andy Estes  <aestes@apple.com>
 
         [iOS] Break a reference cycle between PreviewLoader and ResourceLoader
index 7366c73..bb84101 100644 (file)
@@ -234,9 +234,11 @@ void PopupMenuMac::show(const IntRect& r, FrameView* v, int index)
 
 void PopupMenuMac::hide()
 {
-    [m_popup dismissPopUp];
+    [[m_popup menu] cancelTracking];
+    if (m_client)
+        m_client->popupDidHide();
 }
-    
+
 void PopupMenuMac::updateFromElement()
 {
 }