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

Changeset 287038 in webkit


Ignore:
Timestamp:
Dec 14, 2021, 11:59:26 AM (5 years ago)
Author:
Tyler Wilcock
Message:

Web Inspector: test webpage keeps reloading when Inspector is open
​https://bugs.webkit.org/show_bug.cgi?id=234076

Reviewed by Chris Fleizach.

Source/WebCore:

​https://trac.webkit.org/changeset/284335/webkit introduced the
potential for infinite recursion in AccessibilityObjectMac::shouldIgnoreGroup:

From accessibilityIsIgnored to computeAccessibilityIsIgnored to
defaultObjectInclusion to accessibilityPlatformIncludesObject to
shouldIgnoreGroup to accessibilityText to titleElementText to
exposesTitleUIElement to accessibilityIsIgnored to ...

shouldIgnoreGroup returns true if the group has one static text
child with the same content as the group's AX text. We fix the
recursion by making this check more conservative.

Rather than checking all of the AX text (which includes title text, alt text, help text,
visible text, placeholder text), only check the group's alt text and help text. This
accomplishes the original problem statement laid out in
​https://bugs.webkit.org/show_bug.cgi?id=169924#c0 while avoiding
infinite recursion, as nothing in AccessibilityNodeObject::alternativeText or
AccessibilityNodeObject::helpText calls accessibilityIsIgnored.

Test: accessibility/mac/ignore-redundant-groups-crash.html

  • accessibility/AccessibilityNodeObject.h:

Move alternativeText and helpText method definitions from private to public
so they can be called by the static shouldIgnoreGroup method in AccessibilityObjectMac.

  • accessibility/mac/AccessibilityObjectMac.mm:

(WebCore::shouldIgnoreGroup):
Check only alt text and help text rather than all
AX text when deciding if a group should be ignored.

LayoutTests:

This patch fixes the potential for infinite recursion introduced by
​https://trac.webkit.org/changeset/284335/webkit. See corresponding
Source/WebCore/ChangeLog for full details.

  • accessibility/mac/ignore-redundant-accessibility-text-groups-expected.txt:
  • accessibility/mac/ignore-redundant-accessibility-text-groups.html:

Add an aria-describedby testcase.

  • accessibility/mac/ignore-redundant-groups-crash-expected.txt: Added.
  • accessibility/mac/ignore-redundant-groups-crash.html:

Added. Exercises the crash testcase raised in
​https://bugs.webkit.org/show_bug.cgi?id=234076 along with other
branches in AccessibilityNodeObject::alternativeText to prevent future
problems.

Location:
trunk
Files:
2 added
6 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r287029 r287038  
     12021-12-14  Tyler Wilcock  <tyler_w@apple.com>
     2
     3        Web Inspector: test webpage keeps reloading when Inspector is open
     4        https://bugs.webkit.org/show_bug.cgi?id=234076
     5
     6        Reviewed by Chris Fleizach.
     7
     8        This patch fixes the potential for infinite recursion introduced by
     9        https://trac.webkit.org/changeset/284335/webkit. See corresponding
     10        Source/WebCore/ChangeLog for full details.
     11
     12        * accessibility/mac/ignore-redundant-accessibility-text-groups-expected.txt:
     13        * accessibility/mac/ignore-redundant-accessibility-text-groups.html:
     14        Add an aria-describedby testcase.
     15
     16        * accessibility/mac/ignore-redundant-groups-crash-expected.txt: Added.
     17        * accessibility/mac/ignore-redundant-groups-crash.html:
     18        Added. Exercises the crash testcase raised in
     19        https://bugs.webkit.org/show_bug.cgi?id=234076 along with other
     20        branches in AccessibilityNodeObject::alternativeText to prevent future
     21        problems.
     22
    1232021-12-14  Brandon Stewart  <brandonstewart@apple.com>
    224

  • trunk/LayoutTests/accessibility/mac/ignore-redundant-accessibility-text-groups-expected.txt

    r284335 r287038  
    88PASS !ariaLabelDiv is true
    99PASS !titleDiv is true
     10PASS !ariaDescribedByDiv is true
    1011PASS typeof clickHandlerGroup is 'object'
    1112PASS typeof clickHandlerDiv is 'object'
    … …  
    1415PASS resultElement.role is 'AXRole: AXStaticText'
    1516PASS resultElement.stringValue is 'AXValue: Oranges'
    16 PASS contentContainer.childrenCount is 6
     17PASS resultElement.role is 'AXRole: AXStaticText'
     18PASS resultElement.stringValue is 'AXValue: Jello'
     19PASS resultElement.role is 'AXRole: AXStaticText'
     20PASS resultElement.stringValue is 'AXValue: Broccoli'
     21PASS contentContainer.childrenCount is 8
    1722PASS contentContainer.childAtIndex(0).stringValue is 'AXValue: Blue cheese'
    1823PASS contentContainer.childAtIndex(0).role is 'AXRole: AXStaticText'
    1924PASS contentContainer.childAtIndex(1).stringValue is 'AXValue: Oranges'
    2025PASS contentContainer.childAtIndex(1).role is 'AXRole: AXStaticText'
    21 PASS contentContainer.childAtIndex(2).role is 'AXRole: AXGroup'
    22 PASS contentContainer.childAtIndex(3).stringValue is 'AXValue: Jello'
     26PASS contentContainer.childAtIndex(2).stringValue is 'AXValue: Jello'
     27PASS contentContainer.childAtIndex(2).role is 'AXRole: AXStaticText'
     28PASS contentContainer.childAtIndex(3).stringValue is 'AXValue: Broccoli'
    2329PASS contentContainer.childAtIndex(3).role is 'AXRole: AXStaticText'
    24 PASS contentContainer.childAtIndex(4).stringValue is 'AXValue: Broccoli'
    25 PASS contentContainer.childAtIndex(4).role is 'AXRole: AXStaticText'
    26 PASS contentContainer.childAtIndex(5).role is 'AXRole: AXGroup'
     30Verifying #describer
     31PASS contentContainer.childAtIndex(4).role is 'AXRole: AXGroup'
     32PASS contentContainer.childAtIndex(5).stringValue is 'AXValue: Cheesecake'
     33PASS contentContainer.childAtIndex(5).role is 'AXRole: AXStaticText'
     34Verifying #click-handler-div
     35PASS contentContainer.childAtIndex(6).role is 'AXRole: AXGroup'
     36Verifying #click-handler-group
     37PASS contentContainer.childAtIndex(7).role is 'AXRole: AXGroup'
    2738PASS successfullyParsed is true
    2839

  • trunk/LayoutTests/accessibility/mac/ignore-redundant-accessibility-text-groups.html

    r284335 r287038  
    1717    </div>
    1818
    19     <div id="click-handler-group" role="group" aria-label="Group click handler" onclick="emptyClickHandler()">
    20         Group click handler
    21     </div>
    22 
    2319    <!-- Also test role-less generic divs. -->
    2420    <div id="aria-label-div" aria-label="Jello">
    … …  
    3026    </div>
    3127
     28    <div id="describer">Cheesecake</div>
     29    <div id="aria-describedby-div" aria-describedby="describer">
     30        Cheesecake
     31    </div>
     32
    3233    <div id="click-handler-div" aria-label="Div click handler" onclick="emptyClickHandler()">
    3334        Div click handler
     35    </div>
     36
     37    <div id="click-handler-group" role="group" aria-label="Group click handler" onclick="emptyClickHandler()">
     38        Group click handler
    3439    </div>
    3540</div>
    … …  
    4045
    4146    if (window.accessibilityController) {
    42 
    4347        var contentContainer = accessibilityController.accessibleElementById("content");
    4448
    … …  
    5054        var clickHandlerDiv = accessibilityController.accessibleElementById("click-handler-div");
    5155        var titleDiv = accessibilityController.accessibleElementById("title-div");
     56        var ariaDescribedByDiv = accessibilityController.accessibleElementById("aria-describedby-div");
    5257
    5358        // We shouldn't be able to get an accessible element for these groups because they should be ignored.
    … …  
    5661        shouldBeTrue("!ariaLabelDiv");
    5762        shouldBeTrue("!titleDiv");
     63        shouldBeTrue("!ariaDescribedByDiv");
    5864        // But any group with an event handler should always be exposed.
    5965        shouldBe("typeof clickHandlerGroup", "'object'");
    … …  
    6975        shouldBe("resultElement.stringValue", "'AXValue: Oranges'");
    7076
     77        resultElement = contentContainer.uiElementForSearchPredicate(resultElement, true, "AXAnyTypeSearchKey", "", false);
     78        shouldBe("resultElement.role", "'AXRole: AXStaticText'");
     79        shouldBe("resultElement.stringValue", "'AXValue: Jello'");
     80
     81        resultElement = contentContainer.uiElementForSearchPredicate(resultElement, true, "AXAnyTypeSearchKey", "", false);
     82        shouldBe("resultElement.role", "'AXRole: AXStaticText'");
     83        shouldBe("resultElement.stringValue", "'AXValue: Broccoli'");
     84
    7185        // Ensure the only accessible content exposed via `children` is the text elements and event handler groups.
    72         shouldBe("contentContainer.childrenCount", "6");
     86        shouldBe("contentContainer.childrenCount", "8");
    7387        shouldBe("contentContainer.childAtIndex(0).stringValue", "'AXValue: Blue cheese'");
    7488        shouldBe("contentContainer.childAtIndex(0).role", "'AXRole: AXStaticText'");
    … …  
    7791        shouldBe("contentContainer.childAtIndex(1).role", "'AXRole: AXStaticText'");
    7892
    79         shouldBe("contentContainer.childAtIndex(2).role", "'AXRole: AXGroup'");
     93        shouldBe("contentContainer.childAtIndex(2).stringValue", "'AXValue: Jello'");
     94        shouldBe("contentContainer.childAtIndex(2).role", "'AXRole: AXStaticText'");
    8095
    81         shouldBe("contentContainer.childAtIndex(3).stringValue", "'AXValue: Jello'");
     96        shouldBe("contentContainer.childAtIndex(3).stringValue", "'AXValue: Broccoli'");
    8297        shouldBe("contentContainer.childAtIndex(3).role", "'AXRole: AXStaticText'");
    8398
    84         shouldBe("contentContainer.childAtIndex(4).stringValue", "'AXValue: Broccoli'");
    85         shouldBe("contentContainer.childAtIndex(4).role", "'AXRole: AXStaticText'");
     99        debug("Verifying #describer")
     100        shouldBe("contentContainer.childAtIndex(4).role", "'AXRole: AXGroup'");
    86101
    87         shouldBe("contentContainer.childAtIndex(5).role", "'AXRole: AXGroup'");
     102        shouldBe("contentContainer.childAtIndex(5).stringValue", "'AXValue: Cheesecake'");
     103        shouldBe("contentContainer.childAtIndex(5).role", "'AXRole: AXStaticText'");
     104
     105        debug("Verifying #click-handler-div")
     106        shouldBe("contentContainer.childAtIndex(6).role", "'AXRole: AXGroup'");
     107        debug("Verifying #click-handler-group")
     108        shouldBe("contentContainer.childAtIndex(7).role", "'AXRole: AXGroup'");
    88109
    89110        document.getElementById("content").style.visibility = "hidden";

  • trunk/Source/WebCore/ChangeLog

    r287036 r287038  
     12021-12-14  Tyler Wilcock  <tyler_w@apple.com>
     2
     3        Web Inspector: test webpage keeps reloading when Inspector is open
     4        https://bugs.webkit.org/show_bug.cgi?id=234076
     5
     6        Reviewed by Chris Fleizach.
     7
     8        https://trac.webkit.org/changeset/284335/webkit introduced the
     9        potential for infinite recursion in AccessibilityObjectMac::shouldIgnoreGroup:
     10
     11        From accessibilityIsIgnored to computeAccessibilityIsIgnored to
     12        defaultObjectInclusion to accessibilityPlatformIncludesObject to
     13        shouldIgnoreGroup to accessibilityText to titleElementText to
     14        exposesTitleUIElement to accessibilityIsIgnored to ...
     15
     16        shouldIgnoreGroup returns `true` if the group has one static text
     17        child with the same content as the group's AX text. We fix the
     18        recursion by making this check more conservative.
     19
     20        Rather than checking all of the AX text (which includes title text, alt text, help text,
     21        visible text, placeholder text), only check the group's alt text and help text. This
     22        accomplishes the original problem statement laid out in
     23        https://bugs.webkit.org/show_bug.cgi?id=169924#c0 while avoiding
     24        infinite recursion, as nothing in AccessibilityNodeObject::alternativeText or
     25        AccessibilityNodeObject::helpText calls accessibilityIsIgnored.
     26
     27        Test: accessibility/mac/ignore-redundant-groups-crash.html
     28
     29        * accessibility/AccessibilityNodeObject.h:
     30        Move alternativeText and helpText method definitions from private to public
     31        so they can be called by the static shouldIgnoreGroup method in AccessibilityObjectMac.
     32        * accessibility/mac/AccessibilityObjectMac.mm:
     33        (WebCore::shouldIgnoreGroup):
     34        Check only alt text and help text rather than all
     35        AX text when deciding if a group should be ignored.
     36
    1372021-12-14  Alan Bujtas  <zalan@apple.com>
    238

  • trunk/Source/WebCore/accessibility/AccessibilityNodeObject.h

    r285092 r287038  
    112112    String title() const override;
    113113    String text() const override;
     114    void alternativeText(Vector<AccessibilityText>&) const;
     115    void helpText(Vector<AccessibilityText>&) const;
    114116    String stringValue() const override;
    115117    SRGBA<uint8_t> colorValue() const override;
    … …  
    181183    bool isAccessibilityNodeObject() const final { return true; }
    182184    void accessibilityText(Vector<AccessibilityText>&) const override;
    183     void alternativeText(Vector<AccessibilityText>&) const;
    184185    void visibleText(Vector<AccessibilityText>&) const;
    185     void helpText(Vector<AccessibilityText>&) const;
    186186    String alternativeTextForWebArea() const;
    187187    void ariaLabeledByText(Vector<AccessibilityText>&) const;

  • trunk/Source/WebCore/accessibility/mac/AccessibilityObjectMac.mm

    r286406 r287038  
    120120        return false;
    121121
    122     // Never ignore a <div> with event listeners attached to it (e.g. onclick).
     122    // Never ignore a group with event listeners attached to it (e.g. onclick).
    123123    if (axObject.node() && axObject.node()->hasEventListeners())
    124124        return false;
    … …  
    128128        auto childString = first->stringValue();
    129129        // stringValue() can be null if the underlying document needs style recalculation.
    130         if (!childString.isNull()) {
     130        if (!childString.isNull() && is<AccessibilityNodeObject>(axObject)) {
    131131            Vector<AccessibilityText> axText;
    132             axObject.accessibilityText(axText);
    133             // Don't expose <div>s whose only child is text that has the same content as the <div>s accessibility text.
    134             // Instead, we should expose the text element directly.
     132            auto& axNodeObject = downcast<AccessibilityNodeObject>(axObject);
     133            axNodeObject.alternativeText(axText);
     134            axNodeObject.helpText(axText);
     135            // Ignore groups whose accessibility text is the same as their child's static-text content.
    135136            auto firstText = axText.size() ? axText[0].text : String();
    136137            if (firstText == childString)
Note: See TracChangeset for help on using the changeset viewer.