- Revision
- 266887
- Author
- wenson_hs...@apple.com
- Date
- 2020-09-10 12:56:07 -0700 (Thu, 10 Sep 2020)
Log Message
REGRESSION (r257839): clickpay.com - password placeholder text cannot be replaced
https://bugs.webkit.org/show_bug.cgi?id=216257
<rdar://problem/68150686>
Reviewed by Antti Koivisto.
Source/WebCore:
On clickpay.com, the field in the login form that contains the text "Password" is actually a plain text input,
referred to by the site's script as the "null text input". The page adds a focus event listener to this null
text input, and inside of this focus event listener, it reveals a hidden password field by removing an inline
`display: none;` style rule on the real password input element, programmatically focuses it, and then hides the
null text input by setting it to `display: none;` via inline style.
However, after the changes in r257839, we no longer attempt to do a style update upon programmatic focus in the
case where the programmatically focused element does not have a renderer yet (this applies to the password field
in this scenario, because it previously had `display: none;`). When we determine whether the newly displayed
password field is focusable using `Element::isVisibleWithoutResolvingFullStyle`, we then attempt to use either
the existing computed `RenderStyle` on the element, or perform a partial computed style resolution using the
`ResolveComputedStyleMode::RenderedOnly` flag.
But in the case where `ElementRareData`'s computed style exists, it is not guaranteed to be up to date if the
inline style changed since the computed style was last set. In the context of this bug, it's actually Safari's
AutoFill logic (embedded in the injected bundle) that ends up asking for the computed style of the password
input, forcing it to be created and set (though, as demonstrated in the layout test, simply grabbing the
computed style is sufficient to replicate the bug outside of Safari).
The end result is that we'll use this stale computed style, which still believes that the password input is not
displayed, and we end up not focusing the element due to believing that the password input is hidden. To fix
this, we would need to either check whether the element has an invalid style (i.e. `needsStyleRecalc()`) before
attempting to use the existing computed style, or clear out the `ElementRareData` computed style anytime the
element's style is invalidated. However, both of these approaches will cause us to perform partial style
resolution much more aggressively, leading to a 2-3% regression in Speedometer.
To address the bug without hampering our performance wins from r257839, we add a new node flag so that we can
remember when computed styles are no longer valid due to style invalidation, and consult this flag in
`Element::isVisibleWithoutResolvingFullStyle` to avoid using the existing computed style.
Test: fast/forms/programmatic-focus-after-display.html
* dom/Element.cpp:
(WebCore::Element::invalidateStyle):
(WebCore::Element::resolveComputedStyle):
(WebCore::Element::isVisibleWithoutResolvingFullStyle const):
* dom/Node.h:
(WebCore::Node::setHasValidStyle):
LayoutTests:
Add a new layout test to verify that the bug does not occur. See WebCore/ChangeLog for more details.
* fast/forms/programmatic-focus-after-display-expected.txt: Added.
* fast/forms/programmatic-focus-after-display.html: Added.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (266886 => 266887)
--- trunk/LayoutTests/ChangeLog 2020-09-10 19:42:41 UTC (rev 266886)
+++ trunk/LayoutTests/ChangeLog 2020-09-10 19:56:07 UTC (rev 266887)
@@ -1,3 +1,16 @@
+2020-09-10 Wenson Hsieh <wenson_hs...@apple.com>
+
+ REGRESSION (r257839): clickpay.com - password placeholder text cannot be replaced
+ https://bugs.webkit.org/show_bug.cgi?id=216257
+ <rdar://problem/68150686>
+
+ Reviewed by Antti Koivisto.
+
+ Add a new layout test to verify that the bug does not occur. See WebCore/ChangeLog for more details.
+
+ * fast/forms/programmatic-focus-after-display-expected.txt: Added.
+ * fast/forms/programmatic-focus-after-display.html: Added.
+
2020-09-10 Hector Lopez <hector_i_lo...@apple.com>
[ macOS iOS ] media/modern-media-controls/playback-support/playback-support-autoplay.html is a flaky failure/timeout
Added: trunk/LayoutTests/fast/forms/programmatic-focus-after-display-expected.txt (0 => 266887)
--- trunk/LayoutTests/fast/forms/programmatic-focus-after-display-expected.txt (rev 0)
+++ trunk/LayoutTests/fast/forms/programmatic-focus-after-display-expected.txt 2020-09-10 19:56:07 UTC (rev 266887)
@@ -0,0 +1,11 @@
+This test verifies that programmatically focusing a newly displayed input field changes the active element. To test manually, tap or click the input field below.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS document.activeElement is passwordInput
+PASS didFinishActivatingElement became true
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
Added: trunk/LayoutTests/fast/forms/programmatic-focus-after-display.html (0 => 266887)
--- trunk/LayoutTests/fast/forms/programmatic-focus-after-display.html (rev 0)
+++ trunk/LayoutTests/fast/forms/programmatic-focus-after-display.html 2020-09-10 19:56:07 UTC (rev 266887)
@@ -0,0 +1,47 @@
+<!DOCTYPE html>
+<html>
+<meta name="viewport" content="width=device-width, initial-scale=1">
+<head>
+ <script src=""
+ <script src=""
+ <style>
+ input {
+ font-size: 20px;
+ }
+
+ input[type="password"] {
+ display: none;
+ }
+ </style>
+</head>
+<body>
+ <input type="text" value="Click me">
+ <input type="password">
+ <script>
+ jsTestIsAsync = true;
+ didFinishActivatingElement = false;
+ textInput = document.querySelector("input[type='text']");
+ passwordInput = document.querySelector("input[type='password']");
+
+ textInput.addEventListener("focus", async () => {
+ getComputedStyle(passwordInput).display; // This is necessary in order to reproduce the bug.
+ passwordInput.style.display = "block";
+ passwordInput.focus();
+
+ shouldBe("document.activeElement", "passwordInput");
+ passwordInput.remove();
+ textInput.remove();
+
+ await new Promise(resolve => shouldBecomeEqual("didFinishActivatingElement", "true", resolve));
+ finishJSTest();
+ });
+
+ addEventListener("load", async () => {
+ description("This test verifies that programmatically focusing a newly displayed input field changes the active element. To test manually, tap or click the input field below.");
+ if (window.testRunner)
+ await UIHelper.activateElement(textInput);
+ didFinishActivatingElement = true;
+ });
+ </script>
+</body>
+</html>
Modified: trunk/Source/WebCore/ChangeLog (266886 => 266887)
--- trunk/Source/WebCore/ChangeLog 2020-09-10 19:42:41 UTC (rev 266886)
+++ trunk/Source/WebCore/ChangeLog 2020-09-10 19:56:07 UTC (rev 266887)
@@ -1,3 +1,50 @@
+2020-09-10 Wenson Hsieh <wenson_hs...@apple.com>
+
+ REGRESSION (r257839): clickpay.com - password placeholder text cannot be replaced
+ https://bugs.webkit.org/show_bug.cgi?id=216257
+ <rdar://problem/68150686>
+
+ Reviewed by Antti Koivisto.
+
+ On clickpay.com, the field in the login form that contains the text "Password" is actually a plain text input,
+ referred to by the site's script as the "null text input". The page adds a focus event listener to this null
+ text input, and inside of this focus event listener, it reveals a hidden password field by removing an inline
+ `display: none;` style rule on the real password input element, programmatically focuses it, and then hides the
+ null text input by setting it to `display: none;` via inline style.
+
+ However, after the changes in r257839, we no longer attempt to do a style update upon programmatic focus in the
+ case where the programmatically focused element does not have a renderer yet (this applies to the password field
+ in this scenario, because it previously had `display: none;`). When we determine whether the newly displayed
+ password field is focusable using `Element::isVisibleWithoutResolvingFullStyle`, we then attempt to use either
+ the existing computed `RenderStyle` on the element, or perform a partial computed style resolution using the
+ `ResolveComputedStyleMode::RenderedOnly` flag.
+
+ But in the case where `ElementRareData`'s computed style exists, it is not guaranteed to be up to date if the
+ inline style changed since the computed style was last set. In the context of this bug, it's actually Safari's
+ AutoFill logic (embedded in the injected bundle) that ends up asking for the computed style of the password
+ input, forcing it to be created and set (though, as demonstrated in the layout test, simply grabbing the
+ computed style is sufficient to replicate the bug outside of Safari).
+
+ The end result is that we'll use this stale computed style, which still believes that the password input is not
+ displayed, and we end up not focusing the element due to believing that the password input is hidden. To fix
+ this, we would need to either check whether the element has an invalid style (i.e. `needsStyleRecalc()`) before
+ attempting to use the existing computed style, or clear out the `ElementRareData` computed style anytime the
+ element's style is invalidated. However, both of these approaches will cause us to perform partial style
+ resolution much more aggressively, leading to a 2-3% regression in Speedometer.
+
+ To address the bug without hampering our performance wins from r257839, we add a new node flag so that we can
+ remember when computed styles are no longer valid due to style invalidation, and consult this flag in
+ `Element::isVisibleWithoutResolvingFullStyle` to avoid using the existing computed style.
+
+ Test: fast/forms/programmatic-focus-after-display.html
+
+ * dom/Element.cpp:
+ (WebCore::Element::invalidateStyle):
+ (WebCore::Element::resolveComputedStyle):
+ (WebCore::Element::isVisibleWithoutResolvingFullStyle const):
+ * dom/Node.h:
+ (WebCore::Node::setHasValidStyle):
+
2020-09-10 Devin Rousso <drou...@apple.com>
Web Inspector: modernize generated backend protocol code
Modified: trunk/Source/WebCore/dom/Element.cpp (266886 => 266887)
--- trunk/Source/WebCore/dom/Element.cpp 2020-09-10 19:42:41 UTC (rev 266886)
+++ trunk/Source/WebCore/dom/Element.cpp 2020-09-10 19:56:07 UTC (rev 266887)
@@ -1969,6 +1969,11 @@
{
Node::invalidateStyle(Style::Validity::ElementInvalid);
invalidateSiblingsIfNeeded(*this);
+
+ // FIXME: This flag should be set whenever styles are invalidated while computed styles are present,
+ // not just in this codepath.
+ if (hasRareData() && elementRareData()->computedStyle())
+ setNodeFlag(NodeFlag::IsComputedStyleInvalidFlag);
}
void Element::invalidateStyleAndLayerComposition()
@@ -3363,7 +3368,7 @@
const RenderStyle* Element::resolveComputedStyle(ResolveComputedStyleMode mode)
{
ASSERT(isConnected());
- ASSERT(!existingComputedStyle());
+ ASSERT(!existingComputedStyle() || hasNodeFlag(NodeFlag::IsComputedStyleInvalidFlag));
Deque<RefPtr<Element>, 32> elementsRequiringComputedStyle({ this });
const RenderStyle* computedStyle = nullptr;
@@ -3383,6 +3388,7 @@
computedStyle = style.get();
ElementRareData& rareData = element->ensureElementRareData();
rareData.setComputedStyle(WTFMove(style));
+ element->clearNodeFlag(NodeFlag::IsComputedStyleInvalidFlag);
if (mode == ResolveComputedStyleMode::RenderedOnly && computedStyle->display() == DisplayType::None)
return nullptr;
@@ -3412,7 +3418,7 @@
return renderStyle() && renderStyle()->visibility() == Visibility::Visible;
// Compute style in yet unstyled subtree.
- auto* style = existingComputedStyle();
+ auto* style = hasNodeFlag(NodeFlag::IsComputedStyleInvalidFlag) ? nullptr : existingComputedStyle();
if (!style)
style = const_cast<Element&>(*this).resolveComputedStyle(ResolveComputedStyleMode::RenderedOnly);
Modified: trunk/Source/WebCore/dom/Node.h (266886 => 266887)
--- trunk/Source/WebCore/dom/Node.h 2020-09-10 19:42:41 UTC (rev 266886)
+++ trunk/Source/WebCore/dom/Node.h 2020-09-10 19:56:07 UTC (rev 266887)
@@ -552,8 +552,9 @@
#if ENABLE(FULLSCREEN_API)
ContainsFullScreenElement = 1 << 26,
#endif
+ IsComputedStyleInvalidFlag = 1 << 27,
- // Bits 27-31 are free.
+ // Bits 28-31 are free.
};
enum class TabIndexState : uint8_t {
@@ -873,6 +874,7 @@
bitfields.setStyleValidity(Style::Validity::Valid);
bitfields.clearFlag(NodeStyleFlag::StyleResolutionShouldRecompositeLayer);
setStyleBitfields(bitfields);
+ clearNodeFlag(NodeFlag::IsComputedStyleInvalidFlag);
}
inline void Node::setTreeScopeRecursively(TreeScope& newTreeScope)
