Diff
Modified: trunk/LayoutTests/ChangeLog (257838 => 257839)
--- trunk/LayoutTests/ChangeLog 2020-03-04 11:47:32 UTC (rev 257838)
+++ trunk/LayoutTests/ChangeLog 2020-03-04 12:11:13 UTC (rev 257839)
@@ -1,3 +1,25 @@
+2020-03-04 Antti Koivisto <an...@apple.com>
+
+ Avoid full style resolution on Element::focus()
+ https://bugs.webkit.org/show_bug.cgi?id=208504
+
+ Reviewed by Zalan Bujtas.
+
+ * fast/events/keypress-removed-node-expected.txt:
+ * fast/events/keypress-removed-node.html:
+
+ Modify the test so it is not sensitive to non-rendered whitespace changes
+ (caused by timing of render tree updates).
+
+ * fast/forms/autofocus-input-css-style-change.html:
+
+ Read the <input autofocus> :focus style in rAF as focusing happens asynchronously. This matches other browsers.
+
+ * fast/forms/focus-after-visibility-change-expected.txt: Added.
+ * fast/forms/focus-after-visibility-change.html: Added.
+
+ Add a simple test for visibility style change after renderer has already be created.
+
2020-03-04 Carlos Garcia Campos <cgar...@igalia.com>
[GTK][WPE] Stop adding volume-box class to volume box element
Modified: trunk/LayoutTests/fast/events/keypress-removed-node-expected.txt (257838 => 257839)
--- trunk/LayoutTests/fast/events/keypress-removed-node-expected.txt 2020-03-04 11:47:32 UTC (rev 257838)
+++ trunk/LayoutTests/fast/events/keypress-removed-node-expected.txt 2020-03-04 12:11:13 UTC (rev 257839)
@@ -1,3 +1,3 @@
This test verifies that a node does not retain keyboard focus after it has been removed from the DOM.
- PASS: did not get keyboard event.
+PASS: did not get keyboard event.
Modified: trunk/LayoutTests/fast/events/keypress-removed-node.html (257838 => 257839)
--- trunk/LayoutTests/fast/events/keypress-removed-node.html 2020-03-04 11:47:32 UTC (rev 257838)
+++ trunk/LayoutTests/fast/events/keypress-removed-node.html 2020-03-04 12:11:13 UTC (rev 257839)
@@ -1,8 +1,6 @@
<p>This test verifies that a node does not retain keyboard focus after it has
been removed from the DOM.</p>
-<hr>
-<input type="text">
-<pre id="console">PASS: did not get keyboard event.</pre>
+<hr><input type="text"><pre id="console">PASS: did not get keyboard event.</pre>
<script>
if (window.testRunner)
Modified: trunk/LayoutTests/fast/forms/autofocus-input-css-style-change.html (257838 => 257839)
--- trunk/LayoutTests/fast/forms/autofocus-input-css-style-change.html 2020-03-04 11:47:32 UTC (rev 257838)
+++ trunk/LayoutTests/fast/forms/autofocus-input-css-style-change.html 2020-03-04 12:11:13 UTC (rev 257839)
@@ -13,9 +13,11 @@
testRunner.dumpAsText();
}
-var test = document.getElementById("test");
-if (document.defaultView.getComputedStyle(test, null).getPropertyValue('background-color') == "rgb(0, 128, 0)")
- result.innerHTML = "PASS";
+requestAnimationFrame(()=>{
+ if (getComputedStyle(test, null).getPropertyValue('background-color') == "rgb(0, 128, 0)")
+ result.innerHTML = "PASS";
+});
+
</script>
</body>
</html>
Added: trunk/LayoutTests/fast/forms/focus-after-visibility-change-expected.txt (0 => 257839)
--- trunk/LayoutTests/fast/forms/focus-after-visibility-change-expected.txt (rev 0)
+++ trunk/LayoutTests/fast/forms/focus-after-visibility-change-expected.txt 2020-03-04 12:11:13 UTC (rev 257839)
@@ -0,0 +1,2 @@
+PASS
+
Added: trunk/LayoutTests/fast/forms/focus-after-visibility-change.html (0 => 257839)
--- trunk/LayoutTests/fast/forms/focus-after-visibility-change.html (rev 0)
+++ trunk/LayoutTests/fast/forms/focus-after-visibility-change.html 2020-03-04 12:11:13 UTC (rev 257839)
@@ -0,0 +1,9 @@
+<div id=log>PASS</div>
+<input id=input _onfocus_="log.textContent = 'FAIL';">
+<script>
+if (window.testRunner)
+ testRunner.dumpAsText();
+document.body.offsetLeft;
+input.style.visibility="hidden";
+input.focus();
+</script>
Modified: trunk/Source/WebCore/ChangeLog (257838 => 257839)
--- trunk/Source/WebCore/ChangeLog 2020-03-04 11:47:32 UTC (rev 257838)
+++ trunk/Source/WebCore/ChangeLog 2020-03-04 12:11:13 UTC (rev 257839)
@@ -1,3 +1,74 @@
+2020-03-04 Antti Koivisto <an...@apple.com>
+
+ Avoid full style resolution on Element::focus()
+ https://bugs.webkit.org/show_bug.cgi?id=208504
+
+ Reviewed by Zalan Bujtas.
+
+ Element::focus() currently triggers full style resolution both before (to compute element visibility)
+ and after (for no particular reason).
+
+ Resolving style can be costly if there are further DOM mutations that end up invalidating it again.
+ This patch adds a cheaper single-element way to computing visibility and uses it for focus().
+
+ This appears to be 3-4% Speedometer progression.
+
+ Test: fast/forms/focus-after-visibility-change.html
+
+ * accessibility/AXObjectCache.cpp:
+ (WebCore::AXObjectCache::focusedUIElementForPage):
+
+ AX code assumes renderers have exist for focused element so ensure style is up to date.
+
+ * dom/Document.cpp:
+ (WebCore::Document::setFocusedElement):
+
+ Remove style resolution.
+
+ * dom/Element.cpp:
+ (WebCore::Element::isFocusable const):
+
+ Use isVisibleWithoutResolvingFullStyle helper.
+
+ (WebCore::Element::focus):
+
+ Avoid style resolution if the element is in a subtree that doesn't have renderers yet.
+
+ (WebCore::Element::resolveComputedStyle):
+
+ Add a mode where we bail out when we figure out we are in display:none subtree.
+
+ (WebCore::Element::hasValidStyle const):
+
+ See if we already have valid style.
+
+ (WebCore::Element::isVisibleWithoutResolvingFullStyle const):
+
+ Use computed style mechanism for subtrees that have no renderers yet.
+
+ (WebCore::Element::computedStyle):
+ * dom/Element.h:
+ * html/HTMLAreaElement.cpp:
+ (WebCore::HTMLAreaElement::isFocusable const):
+
+ Use isVisibleWithoutResolvingFullStyle here too.
+
+ * html/HTMLSelectElement.cpp:
+ (WebCore::HTMLSelectElement::platformHandleKeydownEvent):
+ (WebCore::HTMLSelectElement::menuListDefaultEventHandler):
+
+ Update style after explicit focus() calls to keep the existing behavior.
+
+ * html/HTMLTextFormControlElement.cpp:
+ (WebCore::HTMLTextFormControlElement::setRangeText):
+
+ Ensure the renderer is created.
+
+ * html/shadow/SpinButtonElement.cpp:
+ (WebCore::SpinButtonElement::forwardEvent):
+
+ Remove unneeded renderer test.
+
2020-03-04 Carlos Garcia Campos <cgar...@igalia.com>
[GTK][WPE] Use restore view icon for exit fullscreen button in media controls
Modified: trunk/Source/WebCore/accessibility/AXObjectCache.cpp (257838 => 257839)
--- trunk/Source/WebCore/accessibility/AXObjectCache.cpp 2020-03-04 11:47:32 UTC (rev 257838)
+++ trunk/Source/WebCore/accessibility/AXObjectCache.cpp 2020-03-04 12:11:13 UTC (rev 257839)
@@ -427,6 +427,8 @@
if (!focusedDocument)
return nullptr;
+ focusedDocument->updateStyleIfNeeded();
+
#if ENABLE(ACCESSIBILITY_ISOLATED_TREE)
if (clientSupportsIsolatedTree())
return isolatedTreeFocusedObject(*focusedDocument);
Modified: trunk/Source/WebCore/dom/Document.cpp (257838 => 257839)
--- trunk/Source/WebCore/dom/Document.cpp 2020-03-04 11:47:32 UTC (rev 257838)
+++ trunk/Source/WebCore/dom/Document.cpp 2020-03-04 12:11:13 UTC (rev 257839)
@@ -4275,11 +4275,12 @@
if (backForwardCacheState() != NotInBackForwardCache)
return false;
- bool focusChangeBlocked = false;
RefPtr<Element> oldFocusedElement = WTFMove(m_focusedElement);
// Remove focus from the existing focus node (if any)
if (oldFocusedElement) {
+ bool focusChangeBlocked = false;
+
oldFocusedElement->setFocus(false);
setFocusNavigationStartingNode(nullptr);
@@ -4332,23 +4333,21 @@
if (is<HTMLInputElement>(oldFocusedElement)) {
// HTMLInputElement::didBlur just scrolls text fields back to the beginning.
// FIXME: This could be done asynchronusly.
- // Updating style may dispatch events due to PostResolutionCallback
- if (eventsMode == FocusRemovalEventsMode::Dispatch)
- updateStyleIfNeeded();
downcast<HTMLInputElement>(*oldFocusedElement).didBlur();
}
+
+ if (focusChangeBlocked)
+ return false;
}
if (newFocusedElement && newFocusedElement->isFocusable()) {
if (&newFocusedElement->document() != this) {
// Bluring oldFocusedElement may have moved newFocusedElement across documents.
- focusChangeBlocked = true;
- goto SetFocusedNodeDone;
+ return false;
}
if (newFocusedElement->isRootEditableElement() && !acceptsEditingFocus(*newFocusedElement)) {
// delegate blocks focus change
- focusChangeBlocked = true;
- goto SetFocusedNodeDone;
+ return false;
}
// Set focus on the new node
m_focusedElement = newFocusedElement;
@@ -4359,8 +4358,7 @@
if (m_focusedElement != newFocusedElement) {
// handler shifted focus
- focusChangeBlocked = true;
- goto SetFocusedNodeDone;
+ return false;
}
m_focusedElement->dispatchFocusInEvent(eventNames().focusinEvent, oldFocusedElement.copyRef()); // DOM level 3 bubbling focus event.
@@ -4367,8 +4365,7 @@
if (m_focusedElement != newFocusedElement) {
// handler shifted focus
- focusChangeBlocked = true;
- goto SetFocusedNodeDone;
+ return false;
}
// FIXME: We should remove firing DOMFocusInEvent event when we are sure no content depends
@@ -4377,8 +4374,7 @@
if (m_focusedElement != newFocusedElement) {
// handler shifted focus
- focusChangeBlocked = true;
- goto SetFocusedNodeDone;
+ return false;
}
m_focusedElement->setFocus(true);
@@ -4386,8 +4382,7 @@
// The setFocus call triggers a blur and a focus event. Event handlers could cause the focused element to be cleared.
if (m_focusedElement != newFocusedElement) {
// handler shifted focus
- focusChangeBlocked = true;
- goto SetFocusedNodeDone;
+ return false;
}
if (m_focusedElement->isRootEditableElement())
@@ -4412,21 +4407,16 @@
}
}
- if (!focusChangeBlocked && m_focusedElement) {
+ if (m_focusedElement) {
// Create the AXObject cache in a focus change because GTK relies on it.
if (AXObjectCache* cache = axObjectCache())
cache->deferFocusedUIElementChangeIfNeeded(oldFocusedElement.get(), newFocusedElement.get());
}
- if (!focusChangeBlocked && page())
+ if (page())
page()->chrome().focusedElementChanged(m_focusedElement.get());
-SetFocusedNodeDone:
- // Updating style may dispatch events due to PostResolutionCallback
- // FIXME: Why is synchronous style update needed here at all?
- if (eventsMode == FocusRemovalEventsMode::Dispatch)
- updateStyleIfNeeded();
- return !focusChangeBlocked;
+ return true;
}
static bool shouldResetFocusNavigationStartingNode(Node& node)
Modified: trunk/Source/WebCore/dom/Element.cpp (257838 => 257839)
--- trunk/Source/WebCore/dom/Element.cpp 2020-03-04 11:47:32 UTC (rev 257838)
+++ trunk/Source/WebCore/dom/Element.cpp 2020-03-04 12:11:13 UTC (rev 257839)
@@ -607,23 +607,13 @@
return false;
if (!renderer()) {
- // If the node is in a display:none tree it might say it needs style recalc but
- // the whole document is actually up to date.
- // FIXME: We should be able to assert !needsStyleRecalc() || !document().childNeedsStyleRecalc()
- // but it hits too frequently on websites like Gmail and Microsoft Exchange.
-
// Elements in canvas fallback content are not rendered, but they are allowed to be
// focusable as long as their canvas is displayed and visible.
if (auto* canvas = ancestorsOfType<HTMLCanvasElement>(*this).first())
- return canvas->renderer() && canvas->renderer()->style().visibility() == Visibility::Visible;
+ return canvas->isVisibleWithoutResolvingFullStyle();
}
- // FIXME: Even if we are not visible, we might have a child that is visible.
- // Hyatt wants to fix that some day with a "has visible content" flag or the like.
- if (!renderer() || renderer()->style().visibility() != Visibility::Visible)
- return false;
-
- return true;
+ return isVisibleWithoutResolvingFullStyle();
}
bool Element::isUserActionElementInActiveChain() const
@@ -2985,7 +2975,10 @@
}
RefPtr<Element> newTarget = this;
- if (document->haveStylesheetsLoaded())
+
+ // If we don't have renderer yet, isFocusable will compute it without style update.
+ // FIXME: Expand it to avoid style update in all cases.
+ if (renderer() && document->haveStylesheetsLoaded())
document->updateStyleIfNeeded();
if (&newTarget->document() != document.ptr())
@@ -3290,7 +3283,7 @@
return nullptr;
}
-const RenderStyle& Element::resolveComputedStyle()
+const RenderStyle* Element::resolveComputedStyle(ResolveComputedStyleMode mode)
{
ASSERT(isConnected());
ASSERT(!existingComputedStyle());
@@ -3314,11 +3307,56 @@
computedStyle = style.get();
ElementRareData& rareData = element->ensureElementRareData();
rareData.setComputedStyle(WTFMove(style));
+
+ if (mode == ResolveComputedStyleMode::RenderedOnly && computedStyle->display() == DisplayType::None)
+ return nullptr;
}
- return *computedStyle;
+ return computedStyle;
}
+bool Element::hasValidStyle() const
+{
+ if (!document().needsStyleRecalc())
+ return true;
+
+ if (document().hasPendingFullStyleRebuild())
+ return false;
+
+ for (auto& element : lineageOfType<Element>(*this)) {
+ if (element.styleValidity() != Style::Validity::Valid)
+ return false;
+ }
+ return true;
+}
+
+bool Element::isVisibleWithoutResolvingFullStyle() const
+{
+ if (renderStyle() || hasValidStyle())
+ return renderStyle() && renderStyle()->visibility() == Visibility::Visible;
+
+ // Compute style in yet unstyled subtree.
+ auto* style = existingComputedStyle();
+ if (!style)
+ style = const_cast<Element&>(*this).resolveComputedStyle(ResolveComputedStyleMode::RenderedOnly);
+
+ if (!style)
+ return false;
+
+ if (style->display() == DisplayType::None || style->display() == DisplayType::Contents)
+ return false;
+
+ if (style->visibility() != Visibility::Visible)
+ return false;
+
+ for (auto& element : ancestorsOfType<Element>(*this)) {
+ if (element.existingComputedStyle()->display() == DisplayType::None)
+ return false;
+ }
+
+ return true;
+}
+
const RenderStyle& Element::resolvePseudoElementStyle(PseudoId pseudoElementSpecifier)
{
ASSERT(!isPseudoElement());
@@ -3349,7 +3387,7 @@
auto* style = existingComputedStyle();
if (!style)
- style = &resolveComputedStyle();
+ style = resolveComputedStyle();
if (pseudoElementSpecifier != PseudoId::None) {
if (auto* cachedPseudoStyle = style->getCachedPseudoStyle(pseudoElementSpecifier))
Modified: trunk/Source/WebCore/dom/Element.h (257838 => 257839)
--- trunk/Source/WebCore/dom/Element.h 2020-03-04 11:47:32 UTC (rev 257838)
+++ trunk/Source/WebCore/dom/Element.h 2020-03-04 12:11:13 UTC (rev 257839)
@@ -343,6 +343,9 @@
bool needsStyleInvalidation() const;
+ bool hasValidStyle() const;
+ bool isVisibleWithoutResolvingFullStyle() const;
+
// Methods for indicating the style is affected by dynamic updates (e.g., children changing, our position changing in our sibling list, etc.)
bool styleAffectedByActive() const { return hasStyleFlag(ElementStyleFlag::StyleAffectedByActive); }
bool styleAffectedByEmpty() const { return hasStyleFlag(ElementStyleFlag::StyleAffectedByEmpty); }
@@ -692,7 +695,8 @@
void removeShadowRoot();
- const RenderStyle& resolveComputedStyle();
+ enum class ResolveComputedStyleMode { Normal, RenderedOnly };
+ const RenderStyle* resolveComputedStyle(ResolveComputedStyleMode = ResolveComputedStyleMode::Normal);
const RenderStyle& resolvePseudoElementStyle(PseudoId);
unsigned rareDataChildIndex() const;
Modified: trunk/Source/WebCore/editing/FrameSelection.cpp (257838 => 257839)
--- trunk/Source/WebCore/editing/FrameSelection.cpp 2020-03-04 11:47:32 UTC (rev 257838)
+++ trunk/Source/WebCore/editing/FrameSelection.cpp 2020-03-04 12:11:13 UTC (rev 257839)
@@ -369,8 +369,13 @@
setCaretRectNeedsUpdate();
- if (!newSelection.isNone() && !(options & DoNotSetFocus))
+ if (!newSelection.isNone() && !(options & DoNotSetFocus)) {
+ auto* oldFocusedElement = m_frame->document()->focusedElement();
setFocusedElementIfNeeded();
+ // FIXME: Should not be needed.
+ if (m_frame->document()->focusedElement() != oldFocusedElement)
+ m_frame->document()->updateStyleIfNeeded();
+ }
// Always clear the x position used for vertical arrow navigation.
// It will be restored by the vertical arrow navigation code if necessary.
Modified: trunk/Source/WebCore/html/HTMLAreaElement.cpp (257838 => 257839)
--- trunk/Source/WebCore/html/HTMLAreaElement.cpp 2020-03-04 11:47:32 UTC (rev 257838)
+++ trunk/Source/WebCore/html/HTMLAreaElement.cpp 2020-03-04 12:11:13 UTC (rev 257839)
@@ -210,7 +210,7 @@
bool HTMLAreaElement::isFocusable() const
{
RefPtr<HTMLImageElement> image = imageElement();
- if (!image || !image->renderer() || image->renderer()->style().visibility() != Visibility::Visible)
+ if (!image || !image->isVisibleWithoutResolvingFullStyle())
return false;
return supportsFocus() && tabIndexSetExplicitly().valueOr(0) >= 0;
Modified: trunk/Source/WebCore/html/HTMLSelectElement.cpp (257838 => 257839)
--- trunk/Source/WebCore/html/HTMLSelectElement.cpp 2020-03-04 11:47:32 UTC (rev 257838)
+++ trunk/Source/WebCore/html/HTMLSelectElement.cpp 2020-03-04 12:11:13 UTC (rev 257839)
@@ -1100,6 +1100,7 @@
if (!isSpatialNavigationEnabled(document().frame())) {
if (event->keyIdentifier() == "Down" || event->keyIdentifier() == "Up") {
focus();
+ document().updateStyleIfNeeded();
// Calling focus() may cause us to lose our renderer. Return true so
// that our caller doesn't process the event further, but don't set
// the event as handled.
@@ -1198,6 +1199,7 @@
if (RenderTheme::singleton().popsMenuBySpaceOrReturn()) {
if (keyCode == ' ' || keyCode == '\r') {
focus();
+ document().updateStyleIfNeeded();
// Calling focus() may remove the renderer or change the renderer type.
auto* renderer = this->renderer();
@@ -1215,6 +1217,7 @@
} else if (RenderTheme::singleton().popsMenuByArrowKeys()) {
if (keyCode == ' ') {
focus();
+ document().updateStyleIfNeeded();
// Calling focus() may remove the renderer or change the renderer type.
auto* renderer = this->renderer();
@@ -1243,6 +1246,8 @@
if (event.type() == eventNames().mousedownEvent && is<MouseEvent>(event) && downcast<MouseEvent>(event).button() == LeftButton) {
focus();
#if !PLATFORM(IOS_FAMILY)
+ document().updateStyleIfNeeded();
+
auto* renderer = this->renderer();
if (is<RenderMenuList>(renderer)) {
auto& menuList = downcast<RenderMenuList>(*renderer);
@@ -1326,6 +1331,7 @@
if (event.type() == eventNames().mousedownEvent && is<MouseEvent>(event) && downcast<MouseEvent>(event).button() == LeftButton) {
focus();
+ document().updateStyleIfNeeded();
// Calling focus() may remove or change our renderer, in which case we don't want to handle the event further.
auto* renderer = this->renderer();
Modified: trunk/Source/WebCore/html/HTMLTextFormControlElement.cpp (257838 => 257839)
--- trunk/Source/WebCore/html/HTMLTextFormControlElement.cpp 2020-03-04 11:47:32 UTC (rev 257838)
+++ trunk/Source/WebCore/html/HTMLTextFormControlElement.cpp 2020-03-04 12:11:13 UTC (rev 257839)
@@ -242,8 +242,11 @@
setInnerTextValue(text);
- // FIXME: What should happen to the value (as in value()) if there's no renderer?
+ // FIXME: This shouldn't need synchronous style update, or renderer at all.
if (!renderer())
+ document().updateStyleIfNeeded();
+
+ if (!renderer())
return { };
subtreeHasChanged();
Modified: trunk/Source/WebCore/html/shadow/SpinButtonElement.cpp (257838 => 257839)
--- trunk/Source/WebCore/html/shadow/SpinButtonElement.cpp 2020-03-04 11:47:32 UTC (rev 257838)
+++ trunk/Source/WebCore/html/shadow/SpinButtonElement.cpp 2020-03-04 12:11:13 UTC (rev 257839)
@@ -157,9 +157,6 @@
void SpinButtonElement::forwardEvent(Event& event)
{
- if (!renderBox())
- return;
-
if (!is<WheelEvent>(event))
return;
Modified: trunk/Source/WebCore/html/shadow/TextControlInnerElements.cpp (257838 => 257839)
--- trunk/Source/WebCore/html/shadow/TextControlInnerElements.cpp 2020-03-04 11:47:32 UTC (rev 257838)
+++ trunk/Source/WebCore/html/shadow/TextControlInnerElements.cpp 2020-03-04 12:11:13 UTC (rev 257839)
@@ -225,6 +225,8 @@
input->focus();
input->select();
#if !PLATFORM(IOS_FAMILY)
+ document().updateStyleIfNeeded();
+
if (auto* renderer = input->renderer()) {
auto& searchFieldRenderer = downcast<RenderSearchField>(*renderer);
if (searchFieldRenderer.popupIsVisible())
