⚠ Archived content — this site is no longer maintained.   Current WebKit documentation is at docs.webkit.org.

Changeset 257839 in webkit


Ignore:
Timestamp:
Mar 4, 2020, 4:11:13 AM (6 years ago)
Author:
Antti Koivisto
Message:

Avoid full style resolution on Element::focus()
​https://bugs.webkit.org/show_bug.cgi?id=208504

Reviewed by Zalan Bujtas.

Source/WebCore:

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.

LayoutTests:

  • 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.

Location:
trunk
Files:
2 added
15 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r257836 r257839  
     12020-03-04  Antti Koivisto  <antti@apple.com>
     2
     3        Avoid full style resolution on Element::focus()
     4        https://bugs.webkit.org/show_bug.cgi?id=208504
     5
     6        Reviewed by Zalan Bujtas.
     7
     8        * fast/events/keypress-removed-node-expected.txt:
     9        * fast/events/keypress-removed-node.html:
     10
     11        Modify the test so it is not sensitive to non-rendered whitespace changes
     12        (caused by timing of render tree updates).
     13
     14        * fast/forms/autofocus-input-css-style-change.html:
     15
     16        Read the <input autofocus> :focus style in rAF as focusing happens asynchronously. This matches other browsers.
     17
     18        * fast/forms/focus-after-visibility-change-expected.txt: Added.
     19        * fast/forms/focus-after-visibility-change.html: Added.
     20
     21        Add a simple test for visibility style change after renderer has already be created.
     22
    1232020-03-04  Carlos Garcia Campos  <cgarcia@igalia.com>
    224

  • trunk/LayoutTests/fast/events/keypress-removed-node-expected.txt

    r21687 r257839  
    11This test verifies that a node does not retain keyboard focus after it has been removed from the DOM.
    22
    3  PASS: did not get keyboard event.
     3PASS: did not get keyboard event.

  • trunk/LayoutTests/fast/events/keypress-removed-node.html

    r120792 r257839  
    11<p>This test verifies that a node does not retain keyboard focus after it has
    22been removed from the DOM.</p>
    3 <hr>
    4 <input type="text">
    5 <pre id="console">PASS: did not get keyboard event.</pre>
     3<hr><input type="text"><pre id="console">PASS: did not get keyboard event.</pre>
    64
    75<script>

  • trunk/LayoutTests/fast/forms/autofocus-input-css-style-change.html

    r121008 r257839  
    1414}
    1515
    16 var test = document.getElementById("test");
    17 if (document.defaultView.getComputedStyle(test, null).getPropertyValue('background-color') == "rgb(0, 128, 0)")
    18     result.innerHTML = "PASS";
     16requestAnimationFrame(()=>{
     17    if (getComputedStyle(test, null).getPropertyValue('background-color') == "rgb(0, 128, 0)")
     18        result.innerHTML = "PASS";
     19});
     20
    1921</script>
    2022</body>

  • trunk/Source/WebCore/ChangeLog

    r257837 r257839  
     12020-03-04  Antti Koivisto  <antti@apple.com>
     2
     3        Avoid full style resolution on Element::focus()
     4        https://bugs.webkit.org/show_bug.cgi?id=208504
     5
     6        Reviewed by Zalan Bujtas.
     7
     8        Element::focus() currently triggers full style resolution both before (to compute element visibility)
     9        and after (for no particular reason).
     10
     11        Resolving style can be costly if there are further DOM mutations that end up invalidating it again.
     12        This patch adds a cheaper single-element way to computing visibility and uses it for focus().
     13
     14        This appears to be 3-4% Speedometer progression.
     15
     16        Test: fast/forms/focus-after-visibility-change.html
     17
     18        * accessibility/AXObjectCache.cpp:
     19        (WebCore::AXObjectCache::focusedUIElementForPage):
     20
     21        AX code assumes renderers have exist for focused element so ensure style is up to date.
     22
     23        * dom/Document.cpp:
     24        (WebCore::Document::setFocusedElement):
     25
     26        Remove style resolution.
     27
     28        * dom/Element.cpp:
     29        (WebCore::Element::isFocusable const):
     30
     31        Use isVisibleWithoutResolvingFullStyle helper.
     32
     33        (WebCore::Element::focus):
     34
     35        Avoid style resolution if the element is in a subtree that doesn't have renderers yet.
     36
     37        (WebCore::Element::resolveComputedStyle):
     38
     39        Add a mode where we bail out when we figure out we are in display:none subtree.
     40
     41        (WebCore::Element::hasValidStyle const):
     42
     43        See if we already have valid style.
     44
     45        (WebCore::Element::isVisibleWithoutResolvingFullStyle const):
     46
     47        Use computed style mechanism for subtrees that have no renderers yet.
     48
     49        (WebCore::Element::computedStyle):
     50        * dom/Element.h:
     51        * html/HTMLAreaElement.cpp:
     52        (WebCore::HTMLAreaElement::isFocusable const):
     53
     54        Use isVisibleWithoutResolvingFullStyle here too.
     55
     56        * html/HTMLSelectElement.cpp:
     57        (WebCore::HTMLSelectElement::platformHandleKeydownEvent):
     58        (WebCore::HTMLSelectElement::menuListDefaultEventHandler):
     59
     60        Update style after explicit focus() calls to keep the existing behavior.
     61
     62        * html/HTMLTextFormControlElement.cpp:
     63        (WebCore::HTMLTextFormControlElement::setRangeText):
     64
     65        Ensure the renderer is created.
     66
     67        * html/shadow/SpinButtonElement.cpp:
     68        (WebCore::SpinButtonElement::forwardEvent):
     69
     70        Remove unneeded renderer test.
     71
    1722020-03-04  Carlos Garcia Campos  <cgarcia@igalia.com>
    273

  • trunk/Source/WebCore/accessibility/AXObjectCache.cpp

    r257760 r257839  
    427427    if (!focusedDocument)
    428428        return nullptr;
     429
     430    focusedDocument->updateStyleIfNeeded();
    429431
    430432#if ENABLE(ACCESSIBILITY_ISOLATED_TREE)

  • trunk/Source/WebCore/dom/Document.cpp

    r257394 r257839  
    42764276        return false;
    42774277
    4278     bool focusChangeBlocked = false;
    42794278    RefPtr<Element> oldFocusedElement = WTFMove(m_focusedElement);
    42804279
    42814280    // Remove focus from the existing focus node (if any)
    42824281    if (oldFocusedElement) {
     4282        bool focusChangeBlocked = false;
     4283
    42834284        oldFocusedElement->setFocus(false);
    42844285        setFocusNavigationStartingNode(nullptr);
    … …  
    43334334            // HTMLInputElement::didBlur just scrolls text fields back to the beginning.
    43344335            // FIXME: This could be done asynchronusly.
    4335             // Updating style may dispatch events due to PostResolutionCallback
    4336             if (eventsMode == FocusRemovalEventsMode::Dispatch)
    4337                 updateStyleIfNeeded();
    43384336            downcast<HTMLInputElement>(*oldFocusedElement).didBlur();
    43394337        }
     4338
     4339        if (focusChangeBlocked)
     4340            return false;
    43404341    }
    43414342
    … …  
    43434344        if (&newFocusedElement->document() != this) {
    43444345            // Bluring oldFocusedElement may have moved newFocusedElement across documents.
    4345             focusChangeBlocked = true;
    4346             goto SetFocusedNodeDone;
     4346            return false;
    43474347        }
    43484348        if (newFocusedElement->isRootEditableElement() && !acceptsEditingFocus(*newFocusedElement)) {
    43494349            // delegate blocks focus change
    4350             focusChangeBlocked = true;
    4351             goto SetFocusedNodeDone;
     4350            return false;
    43524351        }
    43534352        // Set focus on the new node
    … …  
    43604359        if (m_focusedElement != newFocusedElement) {
    43614360            // handler shifted focus
    4362             focusChangeBlocked = true;
    4363             goto SetFocusedNodeDone;
     4361            return false;
    43644362        }
    43654363
    … …  
    43684366        if (m_focusedElement != newFocusedElement) {
    43694367            // handler shifted focus
    4370             focusChangeBlocked = true;
    4371             goto SetFocusedNodeDone;
     4368            return false;
    43724369        }
    43734370
    … …  
    43784375        if (m_focusedElement != newFocusedElement) {
    43794376            // handler shifted focus
    4380             focusChangeBlocked = true;
    4381             goto SetFocusedNodeDone;
     4377            return false;
    43824378        }
    43834379
    … …  
    43874383        if (m_focusedElement != newFocusedElement) {
    43884384            // handler shifted focus
    4389             focusChangeBlocked = true;
    4390             goto SetFocusedNodeDone;
     4385            return false;
    43914386        }
    43924387
    … …  
    44134408    }
    44144409
    4415     if (!focusChangeBlocked && m_focusedElement) {
     4410    if (m_focusedElement) {
    44164411        // Create the AXObject cache in a focus change because GTK relies on it.
    44174412        if (AXObjectCache* cache = axObjectCache())
    … …  
    44194414    }
    44204415
    4421     if (!focusChangeBlocked && page())
     4416    if (page())
    44224417        page()->chrome().focusedElementChanged(m_focusedElement.get());
    44234418
    4424 SetFocusedNodeDone:
    4425     // Updating style may dispatch events due to PostResolutionCallback
    4426     // FIXME: Why is synchronous style update needed here at all?
    4427     if (eventsMode == FocusRemovalEventsMode::Dispatch)
    4428         updateStyleIfNeeded();
    4429     return !focusChangeBlocked;
     4419    return true;
    44304420}
    44314421

  • trunk/Source/WebCore/dom/Element.cpp

    r257188 r257839  
    608608
    609609    if (!renderer()) {
    610         // If the node is in a display:none tree it might say it needs style recalc but
    611         // the whole document is actually up to date.
    612         // FIXME: We should be able to assert !needsStyleRecalc() || !document().childNeedsStyleRecalc()
    613         // but it hits too frequently on websites like Gmail and Microsoft Exchange.
    614 
    615610        // Elements in canvas fallback content are not rendered, but they are allowed to be
    616611        // focusable as long as their canvas is displayed and visible.
    617612        if (auto* canvas = ancestorsOfType<HTMLCanvasElement>(*this).first())
    618             return canvas->renderer() && canvas->renderer()->style().visibility() == Visibility::Visible;
    619     }
    620 
    621     // FIXME: Even if we are not visible, we might have a child that is visible.
    622     // Hyatt wants to fix that some day with a "has visible content" flag or the like.
    623     if (!renderer() || renderer()->style().visibility() != Visibility::Visible)
    624         return false;
    625 
    626     return true;
     613            return canvas->isVisibleWithoutResolvingFullStyle();
     614    }
     615
     616    return isVisibleWithoutResolvingFullStyle();
    627617}
    628618
    … …  
    29862976
    29872977    RefPtr<Element> newTarget = this;
    2988     if (document->haveStylesheetsLoaded())
     2978
     2979    // If we don't have renderer yet, isFocusable will compute it without style update.
     2980    // FIXME: Expand it to avoid style update in all cases.
     2981    if (renderer() && document->haveStylesheetsLoaded())
    29892982        document->updateStyleIfNeeded();
    29902983
    … …  
    32913284}
    32923285
    3293 const RenderStyle& Element::resolveComputedStyle()
     3286const RenderStyle* Element::resolveComputedStyle(ResolveComputedStyleMode mode)
    32943287{
    32953288    ASSERT(isConnected());
    … …  
    33153308        ElementRareData& rareData = element->ensureElementRareData();
    33163309        rareData.setComputedStyle(WTFMove(style));
    3317     }
    3318 
    3319     return *computedStyle;
     3310
     3311        if (mode == ResolveComputedStyleMode::RenderedOnly && computedStyle->display() == DisplayType::None)
     3312            return nullptr;
     3313    }
     3314
     3315    return computedStyle;
     3316}
     3317
     3318bool Element::hasValidStyle() const
     3319{
     3320    if (!document().needsStyleRecalc())
     3321        return true;
     3322
     3323    if (document().hasPendingFullStyleRebuild())
     3324        return false;
     3325   
     3326    for (auto& element : lineageOfType<Element>(*this)) {
     3327        if (element.styleValidity() != Style::Validity::Valid)
     3328            return false;
     3329    }
     3330    return true;
     3331}
     3332
     3333bool Element::isVisibleWithoutResolvingFullStyle() const
     3334{
     3335    if (renderStyle() || hasValidStyle())
     3336        return renderStyle() && renderStyle()->visibility() == Visibility::Visible;
     3337
     3338    // Compute style in yet unstyled subtree.
     3339    auto* style = existingComputedStyle();
     3340    if (!style)
     3341        style = const_cast<Element&>(*this).resolveComputedStyle(ResolveComputedStyleMode::RenderedOnly);
     3342
     3343    if (!style)
     3344        return false;
     3345
     3346    if (style->display() == DisplayType::None || style->display() == DisplayType::Contents)
     3347        return false;
     3348
     3349    if (style->visibility() != Visibility::Visible)
     3350        return false;
     3351
     3352    for (auto& element : ancestorsOfType<Element>(*this)) {
     3353        if (element.existingComputedStyle()->display() == DisplayType::None)
     3354            return false;
     3355    }
     3356
     3357    return true;
    33203358}
    33213359
    … …  
    33503388    auto* style = existingComputedStyle();
    33513389    if (!style)
    3352         style = &resolveComputedStyle();
     3390        style = resolveComputedStyle();
    33533391
    33543392    if (pseudoElementSpecifier != PseudoId::None) {

  • trunk/Source/WebCore/dom/Element.h

    r255383 r257839  
    344344    bool needsStyleInvalidation() const;
    345345
     346    bool hasValidStyle() const;
     347    bool isVisibleWithoutResolvingFullStyle() const;
     348
    346349    // Methods for indicating the style is affected by dynamic updates (e.g., children changing, our position changing in our sibling list, etc.)
    347350    bool styleAffectedByActive() const { return hasStyleFlag(ElementStyleFlag::StyleAffectedByActive); }
    … …  
    693696    void removeShadowRoot();
    694697
    695     const RenderStyle& resolveComputedStyle();
     698    enum class ResolveComputedStyleMode { Normal, RenderedOnly };
     699    const RenderStyle* resolveComputedStyle(ResolveComputedStyleMode = ResolveComputedStyleMode::Normal);
    696700    const RenderStyle& resolvePseudoElementStyle(PseudoId);
    697701

  • trunk/Source/WebCore/editing/FrameSelection.cpp

    r257188 r257839  
    370370    setCaretRectNeedsUpdate();
    371371
    372     if (!newSelection.isNone() && !(options & DoNotSetFocus))
     372    if (!newSelection.isNone() && !(options & DoNotSetFocus)) {
     373        auto* oldFocusedElement = m_frame->document()->focusedElement();
    373374        setFocusedElementIfNeeded();
     375        // FIXME: Should not be needed.
     376        if (m_frame->document()->focusedElement() != oldFocusedElement)
     377            m_frame->document()->updateStyleIfNeeded();
     378    }
    374379
    375380    // Always clear the x position used for vertical arrow navigation.

  • trunk/Source/WebCore/html/HTMLAreaElement.cpp

    r248846 r257839  
    211211{
    212212    RefPtr<HTMLImageElement> image = imageElement();
    213     if (!image || !image->renderer() || image->renderer()->style().visibility() != Visibility::Visible)
     213    if (!image || !image->isVisibleWithoutResolvingFullStyle())
    214214        return false;
    215215

  • trunk/Source/WebCore/html/HTMLSelectElement.cpp

    r254087 r257839  
    11011101        if (event->keyIdentifier() == "Down" || event->keyIdentifier() == "Up") {
    11021102            focus();
     1103            document().updateStyleIfNeeded();
    11031104            // Calling focus() may cause us to lose our renderer. Return true so
    11041105            // that our caller doesn't process the event further, but don't set
    … …  
    11991200            if (keyCode == ' ' || keyCode == '\r') {
    12001201                focus();
     1202                document().updateStyleIfNeeded();
    12011203
    12021204                // Calling focus() may remove the renderer or change the renderer type.
    … …  
    12161218            if (keyCode == ' ') {
    12171219                focus();
     1220                document().updateStyleIfNeeded();
    12181221
    12191222                // Calling focus() may remove the renderer or change the renderer type.
    … …  
    12441247        focus();
    12451248#if !PLATFORM(IOS_FAMILY)
     1249        document().updateStyleIfNeeded();
     1250
    12461251        auto* renderer = this->renderer();
    12471252        if (is<RenderMenuList>(renderer)) {
    … …  
    13271332    if (event.type() == eventNames().mousedownEvent && is<MouseEvent>(event) && downcast<MouseEvent>(event).button() == LeftButton) {
    13281333        focus();
     1334        document().updateStyleIfNeeded();
    13291335
    13301336        // Calling focus() may remove or change our renderer, in which case we don't want to handle the event further.

  • trunk/Source/WebCore/html/HTMLTextFormControlElement.cpp

    r257199 r257839  
    243243    setInnerTextValue(text);
    244244
    245     // FIXME: What should happen to the value (as in value()) if there's no renderer?
     245    // FIXME: This shouldn't need synchronous style update, or renderer at all.
     246    if (!renderer())
     247        document().updateStyleIfNeeded();
     248
    246249    if (!renderer())
    247250        return { };

  • trunk/Source/WebCore/html/shadow/SpinButtonElement.cpp

    r246490 r257839  
    158158void SpinButtonElement::forwardEvent(Event& event)
    159159{
    160     if (!renderBox())
    161         return;
    162 
    163160    if (!is<WheelEvent>(event))
    164161        return;

  • trunk/Source/WebCore/html/shadow/TextControlInnerElements.cpp

    r256725 r257839  
    226226        input->select();
    227227#if !PLATFORM(IOS_FAMILY)
     228        document().updateStyleIfNeeded();
     229
    228230        if (auto* renderer = input->renderer()) {
    229231            auto& searchFieldRenderer = downcast<RenderSearchField>(*renderer);
Note: See TracChangeset for help on using the changeset viewer.