From 9db905efb49da9417cf11f00e4107bf602ee5755 Mon Sep 17 00:00:00 2001 From: Eitan Isaacson <eitan@monotonous.org> Date: Fri, 24 Apr 2020 14:34:09 +0000 Subject: [PATCH] Bug 1625864 - Fire state change event on LINKED change. r=Jamie,morgan Also stop recreating any accessible that has href modified. Differential Revision: https://phabricator.services.mozilla.com/D71258 --- accessible/base/AccTypes.h | 1 + accessible/base/nsAccessibilityService.cpp | 21 ++++++++---- accessible/generic/Accessible.h | 4 +++ accessible/generic/DocAccessible.cpp | 34 +++++++++++-------- accessible/generic/DocAccessible.h | 3 +- accessible/html/HTMLLinkAccessible.cpp | 6 ++-- accessible/html/HTMLLinkAccessible.h | 14 +++++--- .../events/test_focusable_statechange.html | 6 ++++ .../mochitest/events/test_statechange.html | 20 +++++++++++ .../mochitest/relations/test_update.html | 2 +- .../mochitest/treeupdate/test_recreation.html | 6 ---- .../mochitest/treeupdate/test_select.html | 12 +++---- 12 files changed, 87 insertions(+), 42 deletions(-) diff --git a/accessible/base/AccTypes.h b/accessible/base/AccTypes.h index 7606b715e91c7d..aa5dabab9c6931 100644 --- a/accessible/base/AccTypes.h +++ b/accessible/base/AccTypes.h @@ -50,6 +50,7 @@ enum AccType { * Other accessible types. */ eApplicationType, + eHTMLLinkType, eHTMLOptGroupType, eImageMapType, eMenuPopupType, diff --git a/accessible/base/nsAccessibilityService.cpp b/accessible/base/nsAccessibilityService.cpp index 642e64c5106eca..8ce0c8fdbdc7d6 100644 --- a/accessible/base/nsAccessibilityService.cpp +++ b/accessible/base/nsAccessibilityService.cpp @@ -285,16 +285,23 @@ nsAccessibilityService::ListenersChanged(nsIArray* aEventChanges) { change->GetCountOfEventListenerChangesAffectingAccessibility(&changeCount); NS_ENSURE_SUCCESS(rv, rv); - for (uint32_t i = 0; i < changeCount; i++) { + if (changeCount) { Document* ownerDoc = node->OwnerDoc(); DocAccessible* document = GetExistingDocAccessible(ownerDoc); - // Create an accessible for a inaccessible element having click event - // handler. - if (document && !document->HasAccessible(node) && - nsCoreUtils::HasClickListener(node)) { - document->ContentInserted(node, node->GetNextSibling()); - break; + if (document) { + Accessible* acc = document->GetAccessible(node); + if (!acc && nsCoreUtils::HasClickListener(node)) { + // Create an accessible for a inaccessible element having click event + // handler. + document->ContentInserted(node, node->GetNextSibling()); + } else if (acc && acc->IsHTMLLink() && !acc->AsHTMLLink()->IsLinked()) { + // Notify of a LINKED state change if an HTML link gets a click + // listener but does not have an href attribute. + RefPtr<AccEvent> linkedChangeEvent = + new AccStateChangeEvent(acc, states::LINKED); + document->FireDelayedEvent(linkedChangeEvent); + } } } } diff --git a/accessible/generic/Accessible.h b/accessible/generic/Accessible.h index 4eb404d831a68f..351447eaa35bd9 100644 --- a/accessible/generic/Accessible.h +++ b/accessible/generic/Accessible.h @@ -39,6 +39,7 @@ class EmbeddedObjCollector; class EventTree; class HTMLImageMapAccessible; class HTMLLIAccessible; +class HTMLLinkAccessible; class HyperTextAccessible; class ImageAccessible; class KeyBinding; @@ -576,6 +577,9 @@ class Accessible : public nsISupports { bool IsHTMLListItem() const { return mType == eHTMLLiType; } HTMLLIAccessible* AsHTMLListItem(); + bool IsHTMLLink() const { return mType == eHTMLLinkType; } + HTMLLinkAccessible* AsHTMLLink(); + bool IsHTMLOptGroup() const { return mType == eHTMLOptGroupType; } bool IsHTMLTable() const { return mType == eHTMLTableType; } diff --git a/accessible/generic/DocAccessible.cpp b/accessible/generic/DocAccessible.cpp index 2eb5c3f9e894d8..a82431d1bfef1c 100644 --- a/accessible/generic/DocAccessible.cpp +++ b/accessible/generic/DocAccessible.cpp @@ -671,7 +671,7 @@ void DocAccessible::AttributeWillChange(dom::Element* aElement, return; } - if (aAttribute == nsGkAtoms::aria_disabled || + if (aAttribute == nsGkAtoms::aria_disabled || aAttribute == nsGkAtoms::href || aAttribute == nsGkAtoms::disabled || aAttribute == nsGkAtoms::tabindex || aAttribute == nsGkAtoms::contenteditable) { mPrevStateBits = accessible->State(); @@ -732,7 +732,7 @@ void DocAccessible::AttributeChanged(dom::Element* aElement, // Fire accessible events iff there's an accessible, otherwise we consider // the accessible state wasn't changed, i.e. its state is initial state. - AttributeChangedImpl(accessible, aNameSpaceID, aAttribute); + AttributeChangedImpl(accessible, aNameSpaceID, aAttribute, aModType); // Update dependent IDs cache. Take care of accessible elements because no // accessible element means either the element is not accessible at all or @@ -748,7 +748,7 @@ void DocAccessible::AttributeChanged(dom::Element* aElement, // DocAccessible protected member void DocAccessible::AttributeChangedImpl(Accessible* aAccessible, int32_t aNameSpaceID, - nsAtom* aAttribute) { + nsAtom* aAttribute, int32_t aModType) { // Fire accessible event after short timer, because we need to wait for // DOM attribute & resulting layout to actually change. Otherwise, // assistive technology will retrieve the wrong state/value/selection info. @@ -921,6 +921,23 @@ void DocAccessible::AttributeChangedImpl(Accessible* aAccessible, if (aAttribute == nsGkAtoms::value) { if (aAccessible->IsProgress()) FireDelayedEvent(nsIAccessibleEvent::EVENT_VALUE_CHANGE, aAccessible); + return; + } + + if (aModType == dom::MutationEvent_Binding::REMOVAL || + aModType == dom::MutationEvent_Binding::ADDITION) { + if (aAttribute == nsGkAtoms::href) { + if (aAccessible->IsHTMLLink() && + !nsCoreUtils::HasClickListener(aAccessible->GetContent())) { + RefPtr<AccEvent> linkedChangeEvent = + new AccStateChangeEvent(aAccessible, states::LINKED); + FireDelayedEvent(linkedChangeEvent); + // Fire a focusable state change event if the previous state was + // different. It may be the same if there is tabindex on this link. + aAccessible->MaybeFireFocusableStateChange( + (mPrevStateBits & states::FOCUSABLE)); + } + } } } @@ -1789,17 +1806,6 @@ bool DocAccessible::UpdateAccessibleOnAttrChange(dom::Element* aElement, return true; } - if (aAttribute == nsGkAtoms::href) { - // Not worth the expense to ensure which namespace these are in. It doesn't - // kill use to recreate the accessible even if the attribute was used in - // the wrong namespace or an element that doesn't support it. - - // Make sure the accessible is recreated asynchronously to allow the content - // to handle the attribute change. - RecreateAccessible(aElement); - return true; - } - if (aAttribute == nsGkAtoms::aria_multiselectable && aElement->HasAttr(kNameSpaceID_None, nsGkAtoms::role)) { // This affects whether the accessible supports SelectAccessible. diff --git a/accessible/generic/DocAccessible.h b/accessible/generic/DocAccessible.h index 16339ff7a0eb0b..4aa4314c5b4453 100644 --- a/accessible/generic/DocAccessible.h +++ b/accessible/generic/DocAccessible.h @@ -472,9 +472,10 @@ class DocAccessible : public HyperTextAccessibleWrap, * @param aAccessible [in] accessible the DOM attribute is changed for * @param aNameSpaceID [in] namespace of changed attribute * @param aAttribute [in] changed attribute + * @param aModType [in] modification type (changed/added/removed) */ void AttributeChangedImpl(Accessible* aAccessible, int32_t aNameSpaceID, - nsAtom* aAttribute); + nsAtom* aAttribute, int32_t aModType); /** * Fire accessible events when ARIA attribute is changed. diff --git a/accessible/html/HTMLLinkAccessible.cpp b/accessible/html/HTMLLinkAccessible.cpp index 5cf34b5480f910..a6b952dcf3f2e3 100644 --- a/accessible/html/HTMLLinkAccessible.cpp +++ b/accessible/html/HTMLLinkAccessible.cpp @@ -23,7 +23,9 @@ using namespace mozilla::a11y; HTMLLinkAccessible::HTMLLinkAccessible(nsIContent* aContent, DocAccessible* aDoc) - : HyperTextAccessibleWrap(aContent, aDoc) {} + : HyperTextAccessibleWrap(aContent, aDoc) { + mType = eHTMLLinkType; +} //////////////////////////////////////////////////////////////////////////////// // nsIAccessible @@ -107,7 +109,7 @@ already_AddRefed<nsIURI> HTMLLinkAccessible::AnchorURIAt( } //////////////////////////////////////////////////////////////////////////////// -// Protected members +// HTMLLinkAccessible bool HTMLLinkAccessible::IsLinked() const { EventStates state = mContent->AsElement()->State(); diff --git a/accessible/html/HTMLLinkAccessible.h b/accessible/html/HTMLLinkAccessible.h index b07d649094e999..01c8a3f62039f6 100644 --- a/accessible/html/HTMLLinkAccessible.h +++ b/accessible/html/HTMLLinkAccessible.h @@ -35,17 +35,21 @@ class HTMLLinkAccessible : public HyperTextAccessibleWrap { virtual already_AddRefed<nsIURI> AnchorURIAt( uint32_t aAnchorIndex) const override; - protected: - virtual ~HTMLLinkAccessible() {} - - enum { eAction_Jump = 0 }; - /** * Returns true if the link has href attribute. */ bool IsLinked() const; + + protected: + virtual ~HTMLLinkAccessible() {} + + enum { eAction_Jump = 0 }; }; +inline HTMLLinkAccessible* Accessible::AsHTMLLink() { + return IsHTMLLink() ? static_cast<HTMLLinkAccessible*>(this) : nullptr; +} + } // namespace a11y } // namespace mozilla diff --git a/accessible/tests/mochitest/events/test_focusable_statechange.html b/accessible/tests/mochitest/events/test_focusable_statechange.html index 79e528c28846db..530dbea2202f53 100644 --- a/accessible/tests/mochitest/events/test_focusable_statechange.html +++ b/accessible/tests/mochitest/events/test_focusable_statechange.html @@ -122,6 +122,10 @@ getNode("scrollable2").setAttribute("disabled", "true"); await p; + p = waitForEvent(...focusableStateChange("link", false)); + getNode("link").removeAttribute("href"); + await p; + SimpleTest.finish(); } @@ -152,5 +156,7 @@ <textarea id="textarea" rows="3" cols="30"></textarea> + <a id="link" href="#">A link</a> + </body> </html> diff --git a/accessible/tests/mochitest/events/test_statechange.html b/accessible/tests/mochitest/events/test_statechange.html index 06a471fb38e408..2d712b90b6f0ea 100644 --- a/accessible/tests/mochitest/events/test_statechange.html +++ b/accessible/tests/mochitest/events/test_statechange.html @@ -117,6 +117,20 @@ await p; } + async function testLinked() { + let p = waitForEvent(...stateChange(STATE_LINKED, false, false, "link1")); + getNode("link1").removeAttribute("href"); + await p; + + p = waitForEvent(...stateChange(STATE_LINKED, false, false, "link2")); + getNode("link2").removeAttribute("onclick"); + await p; + + p = waitForEvent(...stateChange(STATE_LINKED, false, true, "link3")); + getNode("link3").setAttribute("href", "http://example.com"); + await p; + } + async function doTests() { // Test opening details objects await openNode("detailsOpen", "summaryOpen", true); @@ -159,6 +173,8 @@ await echoingStateChange("text1", "aria-disabled", "disabled", null, EXT_STATE_ENABLED, true, true); + await testLinked(); + SimpleTest.finish(); } @@ -245,6 +261,10 @@ <input id="text1"> + <a id="link1" href="#">I am a link link</a> + <a id="link2" onclick="console.log('hi')">I am a link-ish link</a> + <a id="link3">I am a non-link link</a> + <div id="eventdump"></div> </body> </html> diff --git a/accessible/tests/mochitest/relations/test_update.html b/accessible/tests/mochitest/relations/test_update.html index f4873316f1cce4..581d592bece032 100644 --- a/accessible/tests/mochitest/relations/test_update.html +++ b/accessible/tests/mochitest/relations/test_update.html @@ -100,7 +100,7 @@ testRelation(aLabelID, RELATION_LABEL_FOR, aElmID); testRelation(aElmID, RELATION_LABELLED_BY, aLabelID); - this.containerNode.setAttribute('href', 'foo'); + this.containerNode.setAttribute('role', 'group'); }; this.finalCheck = function recreateRelatives_finalCheck() { diff --git a/accessible/tests/mochitest/treeupdate/test_recreation.html b/accessible/tests/mochitest/treeupdate/test_recreation.html index ee4b2d471cb45e..92cf89256947d8 100644 --- a/accessible/tests/mochitest/treeupdate/test_recreation.html +++ b/accessible/tests/mochitest/treeupdate/test_recreation.html @@ -39,12 +39,6 @@ document.getElementById("div1").setAttribute("role", "button"); await events; - msg = "Add an href to an anchor tag"; - events = waitForOrderedEvents( - [[EVENT_HIDE, "anchor"], [EVENT_SHOW, "anchor"], [EVENT_REORDER, "container"]], msg); - document.getElementById("anchor").setAttribute("href", "www"); - await events; - msg = "Set a listbox as multiselectable"; events = waitForOrderedEvents( [[EVENT_HIDE, "div3"], [EVENT_SHOW, "div3"], [EVENT_REORDER, "container"]], msg); diff --git a/accessible/tests/mochitest/treeupdate/test_select.html b/accessible/tests/mochitest/treeupdate/test_select.html index 773fbcb8fe7e5e..c4c8c8b8f63206 100644 --- a/accessible/tests/mochitest/treeupdate/test_select.html +++ b/accessible/tests/mochitest/treeupdate/test_select.html @@ -83,16 +83,16 @@ } /** - * Setting @href on option makes the accessible to recreate. + * Setting role=option on option makes the accessible recreate. */ - function setHrefOnOption() { + function setRoleOnOption() { this.eventSeq = [ new invokerChecker(EVENT_HIDE, "s2_o"), new invokerChecker(EVENT_SHOW, "s2_o"), ]; - this.invoke = function setHrefOnOption_setHref() { - getNode("s2_o").setAttribute("href", "1"); + this.invoke = function setRoleOnOption_setRole() { + getNode("s2_o").setAttribute("role", "option"); }; this.finalCheck = function() { @@ -106,7 +106,7 @@ }; this.getID = function removeptions_getID() { - return "setting @href on select option"; + return "setting role=option on select option"; }; } @@ -115,7 +115,7 @@ gQueue.push(new addOptions("select")); gQueue.push(new removeOptions("select")); - gQueue.push(new setHrefOnOption()); + gQueue.push(new setRoleOnOption()); gQueue.invoke(); // Will call SimpleTest.finish(); }