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.
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: http://svn.webkit.org/repository/webkit/trunk@222114 268f45cc-cd09-0410-ab3c-d52691b4dbfc
diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
index ac78348..7179fe3 100644
--- a/LayoutTests/ChangeLog
+++ b/LayoutTests/ChangeLog
@@ -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
index 0000000..7ef22e9
--- /dev/null
+++ b/LayoutTests/fast/forms/change-input-type-in-focus-handler-expected.txt
@@ -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
index 0000000..b7a1d8c
--- /dev/null
+++ b/LayoutTests/fast/forms/change-input-type-in-focus-handler.html
@@ -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
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
index 0a073a1..218144e 100644
--- a/Source/WebCore/ChangeLog
+++ b/Source/WebCore/ChangeLog
@@ -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
diff --git a/Source/WebCore/dom/Document.cpp b/Source/WebCore/dom/Document.cpp
index f2c65a8..fc96f1e 100644
--- a/Source/WebCore/dom/Document.cpp
+++ b/Source/WebCore/dom/Document.cpp
@@ -3839,6 +3839,9 @@
else
view()->setFocus(false);
}
+
+ if (is<HTMLInputElement>(oldFocusedElement.get()))
+ downcast<HTMLInputElement>(*oldFocusedElement).didBlur();
}
if (newFocusedElement && newFocusedElement->isFocusable()) {
diff --git a/Source/WebCore/html/HTMLInputElement.cpp b/Source/WebCore/html/HTMLInputElement.cpp
index aacfcda..40bf52f 100644
--- a/Source/WebCore/html/HTMLInputElement.cpp
+++ b/Source/WebCore/html/HTMLInputElement.cpp
@@ -1120,6 +1120,14 @@
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) {
diff --git a/Source/WebCore/html/HTMLInputElement.h b/Source/WebCore/html/HTMLInputElement.h
index 8c8921a..928991e 100644
--- a/Source/WebCore/html/HTMLInputElement.h
+++ b/Source/WebCore/html/HTMLInputElement.h
@@ -213,6 +213,8 @@
void willDispatchEvent(Event&, InputElementClickState&);
void didDispatchClickEvent(Event&, const InputElementClickState&);
+ void didBlur();
+
int maxResults() const { return m_maxResults; }
WEBCORE_EXPORT String defaultValue() const;
diff --git a/Source/WebCore/html/InputType.h b/Source/WebCore/html/InputType.h
index d7e29c7..c1aa37b 100644
--- a/Source/WebCore/html/InputType.h
+++ b/Source/WebCore/html/InputType.h
@@ -203,6 +203,8 @@
virtual void subtreeHasChanged();
virtual void blur();
+ virtual void elementDidBlur() { }
+
#if ENABLE(TOUCH_EVENTS)
virtual bool hasTouchEventHandler() const;
#endif
diff --git a/Source/WebCore/html/TextFieldInputType.cpp b/Source/WebCore/html/TextFieldInputType.cpp
index b8752a8..5ca4667 100644
--- a/Source/WebCore/html/TextFieldInputType.cpp
+++ b/Source/WebCore/html/TextFieldInputType.cpp
@@ -188,29 +188,31 @@
return;
}
- if (event.isMouseEvent()
- || event.type() == eventNames().blurEvent
- || event.type() == eventNames().focusEvent)
- {
- element().document().updateStyleIfNeeded();
+ bool isFocusEvent = event.type() == eventNames().focusEvent;
+ bool isBlurEvent = event.type() == eventNames().blurEvent;
+ if (isFocusEvent || isBlurEvent)
+ capsLockStateMayHaveChanged();
+ if (event.isMouseEvent() || isFocusEvent || isBlurEvent)
+ element().forwardEvent(event);
+}
- 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();
+void TextFieldInputType::elementDidBlur()
+{
+ auto* renderer = element().renderer();
+ if (!renderer)
+ return;
- element().forwardEvent(event);
- }
- }
+ 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)
diff --git a/Source/WebCore/html/TextFieldInputType.h b/Source/WebCore/html/TextFieldInputType.h
index 7fe6f4c..ff06141 100644
--- a/Source/WebCore/html/TextFieldInputType.h
+++ b/Source/WebCore/html/TextFieldInputType.h
@@ -90,6 +90,7 @@
void subtreeHasChanged() final;
void capsLockStateMayHaveChanged() final;
void updateAutoFillButton() final;
+ void elementDidBlur() final;
// SpinButtonElement::SpinButtonOwner functions.
void focusAndSelectSpinButtonOwner() final;
