Avoid style recomputation when forwarding a focus event to an text field's input...
authorwenson_hsieh@apple.com <wenson_hsieh@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 15 Sep 2017 22:18:51 +0000 (22:18 +0000)
committerwenson_hsieh@apple.com <wenson_hsieh@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 15 Sep 2017 22:18:51 +0000 (22:18 +0000)
https://bugs.webkit.org/show_bug.cgi?id=176160
<rdar://problem/34184820>

Reviewed by Ryosuke Niwa.

Source/WebCore:

Currently, TextFieldInputType::forwardEvent synchronously triggers style recomputation, for the purpose of
scrolling to the origin upon handling a blur event, and also for updating caps lock state after a blur or focus.
In synchronously triggering style recomputation, we may end up running arbitrary JavaScript, which may change
the HTMLInputElement's type and cause the current TextFieldInputType to be destroyed.

To mitigate this, we only update caps lock state when forwarding a focus or blur event to the InputType, and
instead scroll blurred text fields to the origin later, in HTMLInputElement::didBlur (invoked from
Document::setFocusedElement after blur and focusout events have fired). Instead of having the InputType update
style, lift the call to Document::updateStyleIfNeeded up into HTMLInputElement so that we gracefully handle the
case where the page destroys and sets a new InputType within the scope of this style update.

Test: fast/forms/change-input-type-in-focus-handler.html

* dom/Document.cpp:
(WebCore::Document::setFocusedElement):
* html/HTMLInputElement.cpp:
(WebCore::HTMLInputElement::didBlur):
* html/HTMLInputElement.h:
* html/InputType.h:
(WebCore::InputType::elementDidBlur):
* html/TextFieldInputType.cpp:
(WebCore::TextFieldInputType::forwardEvent):
(WebCore::TextFieldInputType::elementDidBlur):
* html/TextFieldInputType.h:

LayoutTests:

Adds a new layout test verifying that we don't crash when changing the input type from within a focus event listener.

* fast/forms/change-input-type-in-focus-handler-expected.txt: Added.
* fast/forms/change-input-type-in-focus-handler.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/fast/forms/change-input-type-in-focus-handler-expected.txt [new file with mode: 0644]
LayoutTests/fast/forms/change-input-type-in-focus-handler.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/dom/Document.cpp
Source/WebCore/html/HTMLInputElement.cpp
Source/WebCore/html/HTMLInputElement.h
Source/WebCore/html/InputType.h
Source/WebCore/html/TextFieldInputType.cpp
Source/WebCore/html/TextFieldInputType.h

index ac78348d75ba810140aae3589bf937907e701a0b..7179fe344200293ea0341b16a232721f36646b45 100644 (file)
@@ -1,3 +1,16 @@
+2017-09-15  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        Avoid style recomputation when forwarding a focus event to an text field's input type
+        https://bugs.webkit.org/show_bug.cgi?id=176160
+        <rdar://problem/34184820>
+
+        Reviewed by Ryosuke Niwa.
+
+        Adds a new layout test verifying that we don't crash when changing the input type from within a focus event listener.
+
+        * fast/forms/change-input-type-in-focus-handler-expected.txt: Added.
+        * fast/forms/change-input-type-in-focus-handler.html: Added.
+
 2017-09-14  Ryan Haddad  <ryanhaddad@apple.com>
 
         Move test expectations from 'mac-highsierra' to 'mac' directory
diff --git a/LayoutTests/fast/forms/change-input-type-in-focus-handler-expected.txt b/LayoutTests/fast/forms/change-input-type-in-focus-handler-expected.txt
new file mode 100644 (file)
index 0000000..7ef22e9
--- /dev/null
@@ -0,0 +1 @@
+PASS
diff --git a/LayoutTests/fast/forms/change-input-type-in-focus-handler.html b/LayoutTests/fast/forms/change-input-type-in-focus-handler.html
new file mode 100644 (file)
index 0000000..b7a1d8c
--- /dev/null
@@ -0,0 +1,26 @@
+<!DOCTYPE html>
+<html>
+<div id="div">
+    <form id="form">
+        <input id="input" onfocus="focused()" type="tel"></input>
+    </form>
+</div>
+
+<script>
+function focused() {
+    if (input.autofocus) {
+        input.type = "checkbox";
+        document.body.innerHTML = "<code style='color: green'>PASS</code>";
+        return;
+    }
+
+    document.body.appendChild(input);
+    input.autofocus = true;
+}
+
+if (window.testRunner)
+    testRunner.dumpAsText();
+
+input.focus();
+</script>
+</html>
\ No newline at end of file
index 0a073a1ea5e266e8d6b41f1b3644198ea8584b59..218144edb6ea657e86d904b7cde12386e5f69d86 100644 (file)
@@ -1,3 +1,36 @@
+2017-09-15  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        Avoid style recomputation when forwarding a focus event to an text field's input type
+        https://bugs.webkit.org/show_bug.cgi?id=176160
+        <rdar://problem/34184820>
+
+        Reviewed by Ryosuke Niwa.
+
+        Currently, TextFieldInputType::forwardEvent synchronously triggers style recomputation, for the purpose of
+        scrolling to the origin upon handling a blur event, and also for updating caps lock state after a blur or focus.
+        In synchronously triggering style recomputation, we may end up running arbitrary JavaScript, which may change
+        the HTMLInputElement's type and cause the current TextFieldInputType to be destroyed.
+
+        To mitigate this, we only update caps lock state when forwarding a focus or blur event to the InputType, and
+        instead scroll blurred text fields to the origin later, in HTMLInputElement::didBlur (invoked from
+        Document::setFocusedElement after blur and focusout events have fired). Instead of having the InputType update
+        style, lift the call to Document::updateStyleIfNeeded up into HTMLInputElement so that we gracefully handle the
+        case where the page destroys and sets a new InputType within the scope of this style update.
+
+        Test: fast/forms/change-input-type-in-focus-handler.html
+
+        * dom/Document.cpp:
+        (WebCore::Document::setFocusedElement):
+        * html/HTMLInputElement.cpp:
+        (WebCore::HTMLInputElement::didBlur):
+        * html/HTMLInputElement.h:
+        * html/InputType.h:
+        (WebCore::InputType::elementDidBlur):
+        * html/TextFieldInputType.cpp:
+        (WebCore::TextFieldInputType::forwardEvent):
+        (WebCore::TextFieldInputType::elementDidBlur):
+        * html/TextFieldInputType.h:
+
 2017-09-15  JF Bastien  <jfbastien@apple.com>
 
         WTF: use Forward.h when appropriate instead of Vector.h
index f2c65a84ed5c36a0654e3ba0fa0c279e6bc0f073..fc96f1ea599ebd144422d02bb775b9eeb1fb9139 100644 (file)
@@ -3839,6 +3839,9 @@ bool Document::setFocusedElement(Element* element, FocusDirection direction, Foc
             else
                 view()->setFocus(false);
         }
+
+        if (is<HTMLInputElement>(oldFocusedElement.get()))
+            downcast<HTMLInputElement>(*oldFocusedElement).didBlur();
     }
 
     if (newFocusedElement && newFocusedElement->isFocusable()) {
index aacfcdab2d3a76d023a53e0b02d6e6805f9e0633..40bf52fc6e848c17a8f2e952acc956de7c1d60e5 100644 (file)
@@ -1120,6 +1120,14 @@ void HTMLInputElement::didDispatchClickEvent(Event& event, const InputElementCli
     m_inputType->didDispatchClick(&event, state);
 }
 
+void HTMLInputElement::didBlur()
+{
+    // We need to update style here, rather than in InputType itself, since style recomputation may fire events
+    // that could change the input's type.
+    document().updateStyleIfNeeded();
+    m_inputType->elementDidBlur();
+}
+
 void HTMLInputElement::defaultEventHandler(Event& event)
 {
     if (is<MouseEvent>(event) && event.type() == eventNames().clickEvent && downcast<MouseEvent>(event).button() == LeftButton) {
index 8c8921aa8c930f70e5d5f67773c1566e57f4469f..928991e3d744ab8fc334c7f797fa916a2281d817 100644 (file)
@@ -213,6 +213,8 @@ public:
     void willDispatchEvent(Event&, InputElementClickState&);
     void didDispatchClickEvent(Event&, const InputElementClickState&);
 
+    void didBlur();
+
     int maxResults() const { return m_maxResults; }
 
     WEBCORE_EXPORT String defaultValue() const;
index d7e29c73b490d9399b4b6c2bae766e9f12bfca97..c1aa37b441a2e5e4a9b355efc0e0dba13a30d410 100644 (file)
@@ -203,6 +203,8 @@ public:
     virtual void subtreeHasChanged();
     virtual void blur();
 
+    virtual void elementDidBlur() { }
+
 #if ENABLE(TOUCH_EVENTS)
     virtual bool hasTouchEventHandler() const;
 #endif
index b8752a8928db2d995b847a253f7099be88c7ccfc..5ca466711609bf8092f3344972b7087f2b50cfb7 100644 (file)
@@ -188,29 +188,31 @@ void TextFieldInputType::forwardEvent(Event& event)
             return;
     }
 
-    if (event.isMouseEvent()
-        || event.type() == eventNames().blurEvent
-        || event.type() == eventNames().focusEvent)
-    {
-        element().document().updateStyleIfNeeded();
-
-        auto* renderer = element().renderer();
-        if (element().renderer()) {
-            if (event.type() == eventNames().blurEvent) {
-                if (auto* innerTextRenderer = innerTextElement()->renderer()) {
-                    if (auto* innerLayer = innerTextRenderer->layer()) {
-                        bool isLeftToRightDirection = downcast<RenderTextControlSingleLine>(*renderer).style().isLeftToRightDirection();
-                        ScrollOffset scrollOffset(isLeftToRightDirection ? 0 : innerLayer->scrollWidth(), 0);
-                        innerLayer->scrollToOffset(scrollOffset, RenderLayer::ScrollOffsetClamped);
-                    }
-                }
-                capsLockStateMayHaveChanged();
-            } else if (event.type() == eventNames().focusEvent)
-                capsLockStateMayHaveChanged();
-
-            element().forwardEvent(event);
-        }
-    }
+    bool isFocusEvent = event.type() == eventNames().focusEvent;
+    bool isBlurEvent = event.type() == eventNames().blurEvent;
+    if (isFocusEvent || isBlurEvent)
+        capsLockStateMayHaveChanged();
+    if (event.isMouseEvent() || isFocusEvent || isBlurEvent)
+        element().forwardEvent(event);
+}
+
+void TextFieldInputType::elementDidBlur()
+{
+    auto* renderer = element().renderer();
+    if (!renderer)
+        return;
+
+    auto* innerTextRenderer = innerTextElement()->renderer();
+    if (!innerTextRenderer)
+        return;
+
+    auto* innerLayer = innerTextRenderer->layer();
+    if (!innerLayer)
+        return;
+
+    bool isLeftToRightDirection = downcast<RenderTextControlSingleLine>(*renderer).style().isLeftToRightDirection();
+    ScrollOffset scrollOffset(isLeftToRightDirection ? 0 : innerLayer->scrollWidth(), 0);
+    innerLayer->scrollToOffset(scrollOffset, RenderLayer::ScrollOffsetClamped);
 }
 
 void TextFieldInputType::handleFocusEvent(Node* oldFocusedNode, FocusDirection)
index 7fe6f4cd5fb23a10ffa5bef5ea880cd2b7264d6d..ff061410d6fd3e9b7adf993084dc22cd8fe19137 100644 (file)
@@ -90,6 +90,7 @@ private:
     void subtreeHasChanged() final;
     void capsLockStateMayHaveChanged() final;
     void updateAutoFillButton() final;
+    void elementDidBlur() final;
 
     // SpinButtonElement::SpinButtonOwner functions.
     void focusAndSelectSpinButtonOwner() final;