Bug 256950

<rdar://problem/109498423>

Created attachment 466397 [details] Patch

(In reply to Tyler Wilcock from comment #2) > Created attachment 466397 [details] > Patch --- a/Source/WebCore/accessibility/AXObjectCache.cpp +++ b/Source/WebCore/accessibility/AXObjectCache.cpp +static bool shouldCreateAccessibilityList(Node* node) I would call it simply isAXList(...). +{ + // If the node is aria role="list" or the aria role is empty and its a Typo: its -> it's + // ul/ol/dl type (it shouldn't be a list if aria says otherwise). + return (node && ((nodeHasRole(node, "list"_s) || nodeHasRole(node, "directory"_s)) + || (nodeHasRole(node, nullAtom()) && (node->hasTagName(ulTag) || node->hasTagName(olTag) || node->hasTagName(dlTag) || node->hasTagName(menuTag))))); +} The role attribute should not override <ul>, <ol>, <dl> tag names. A <ul> element should be a list. Although I see that this was there in createObjectFromRenderer before this change, it is incorrect. Can we add a FIXME for this? --- a/Source/WebCore/accessibility/AccessibilityList.h +++ b/Source/WebCore/accessibility/AccessibilityList.h + AccessibilityRole determineAccessibilityRoleInner(); Don't see where this is defined or used. Missed to remove? --- a/Source/WebCore/accessibility/AccessibilityNodeObject.cpp +++ b/Source/WebCore/accessibility/AccessibilityNodeObject.cpp bool AccessibilityNodeObject::canHaveChildren() const { - // If this is an AccessibilityRenderObject, then it's okay if this object - // doesn't have a node - there are some renderers that don't have associated - // nodes, like scroll areas and css-generated text. - if (!node() && !isAccessibilityRenderObject()) + if (!renderer() && !node()) return false; I find this comment somewhat instructive if it holds true, that there can be RenderObjects with no Node. Can we keep it, or a revision of it? Furthermore, how can there be an object without eigher renderer or node? There is the AX mocked objects, but I don't think they derive from AXNodeObject, do they? @@ -572,6 +588,8 @@ bool AccessibilityNodeObject::computeAccessibilityIsIgnored() const // it's been initialized. ASSERT(m_initialized); #endif + if (!node()) + return true; Why do we need this? --- a/Source/WebCore/accessibility/AccessibilityRenderObject.cpp +++ b/Source/WebCore/accessibility/AccessibilityRenderObject.cpp +AccessibilityRenderObject::AccessibilityRenderObject(Node& node) + : AccessibilityNodeObject(&node) + , m_renderer(node.renderer()) +{ + // We should only ever create an instance of this class with a node if that node has no renderer (i.e. because of display:contents). + ASSERT(!node.renderer()); +} Then we don't need + , m_renderer(node.renderer()) Node* AccessibilityRenderObject::node() const do we need this override at all? { if (!m_renderer) - return nullptr; - if (m_renderer->isRenderView()) + return AccessibilityNodeObject::node(); + if (UNLIKELY(m_renderer->isRenderView())) is this possible? Or a RenderView will always create an AXScrollView instead. return &m_renderer->document(); return m_renderer->node(); } AccessibilityObjectInclusion AccessibilityRenderObject::defaultObjectInclusion() const { - if (!m_renderer) + if (!m_renderer && !node()) return AccessibilityObjectInclusion::IgnoreObject; Is it possible an AXRenderObject with neigher renderer nor Node? --- a/LayoutTests/platform/mac-wk2/accessibility/aria-visible-element-roles-expected.txt +++ b/LayoutTests/platform/mac-wk2/accessibility/aria-visible-element-roles-expected.txt - AXRole: AXList - computedRoleString: list + AXRole: AXGroup + AXSubrole: AXContentList is this expected, the role changing from list to group? <ul hidden="" aria-hidden="false" id="ul"></ul> - AXRole: AXList - computedRoleString: list + AXRole: AXGroup + AXSubrole: AXContentList Dito. <menu hidden="" aria-hidden="false" type="toolbar" id="menu-toolbar"></menu> - AXRole: AXList - computedRoleString: list + AXRole: AXGroup + AXSubrole: AXContentList Dito.

Created attachment 466438 [details] Patch

Created attachment 466439 [details] Patch

> + // ul/ol/dl type (it shouldn't be a list if aria says otherwise). > + return (node && ((nodeHasRole(node, "list"_s) || nodeHasRole(node, > "directory"_s)) > + || (nodeHasRole(node, nullAtom()) && (node->hasTagName(ulTag) || > node->hasTagName(olTag) || node->hasTagName(dlTag) || > node->hasTagName(menuTag))))); > +} > > The role attribute should not override <ul>, <ol>, <dl> tag names. A <ul> > element should be a list. > Although I see that this was there in createObjectFromRenderer before this > change, it is incorrect. Can we add a FIXME for this? I think we should hold off adding a FIXME for this until we get agreement from standards folks that "role attribute should not override <ul>, <ol>, <dl> tag names" is a change that should be made. > @@ -572,6 +588,8 @@ bool > AccessibilityNodeObject::computeAccessibilityIsIgnored() const > // it's been initialized. > ASSERT(m_initialized); > #endif > + if (!node()) > + return true; > > Why do we need this? The intention is to catch the case where the WeakPtr<Node> m_node has been destroyed but the AX object hasn't been cleaned up yet (similar to the check in AccessibilityRenderObject::computeAccessibilityIsIgnored). > Node* AccessibilityRenderObject::node() const > > do we need this override at all? I did try removing it, but that causes many tests to fail. Maybe for cases where RenderObject::node() initially is nullptr when creating the object, but becomes not nullptr later? I have to imagine that would mess lots of other things up, though...so something to investigate for the future. > { > if (!m_renderer) > - return nullptr; > - if (m_renderer->isRenderView()) > + return AccessibilityNodeObject::node(); > + if (UNLIKELY(m_renderer->isRenderView())) > > is this possible? Or a RenderView will always create an AXScrollView instead. RenderViews are created as AccessibilityRenderObjects, not AccessibilityScrollView (that is ScrollView). > AccessibilityRenderObject::defaultObjectInclusion() const > { > - if (!m_renderer) > + if (!m_renderer && !node()) > return AccessibilityObjectInclusion::IgnoreObject; > > Is it possible an AXRenderObject with neigher renderer nor Node? The intention is to catch the case where the WeakPtr<RenderObject> m_renderer and WeakPtr<Node> m_node have been destroyed but that AX object hasn't been cleaned up yet. > a/LayoutTests/platform/mac-wk2/accessibility/aria-visible-element-roles- > expected.txt > +++ > b/LayoutTests/platform/mac-wk2/accessibility/aria-visible-element-roles- > expected.txt > > - AXRole: AXList > - computedRoleString: list > + AXRole: AXGroup > + AXSubrole: AXContentList > > is this expected, the role changing from list to group? > > > <ul hidden="" aria-hidden="false" id="ul"></ul> > - AXRole: AXList > - computedRoleString: list > + AXRole: AXGroup > + AXSubrole: AXContentList > > Dito. > > <menu hidden="" aria-hidden="false" type="toolbar" id="menu-toolbar"></menu> > - AXRole: AXList > - computedRoleString: list > + AXRole: AXGroup > + AXSubrole: AXContentList > > Dito. Yes, all intentional. Because we now properly create display:contents lists as AccessibilityList rather than AccessibilityNodeObject, the more stringent "is this a list" heuristics are applied, and "ARIA visible" lists do not pass that test (because nothing is visibly rendered).

Created attachment 466440 [details] Patch

(In reply to Tyler Wilcock from comment #7) > Created attachment 466440 [details] > Patch @@ -516,8 +535,9 @@ bool AccessibilityNodeObject::canHaveChildren() const { // If this is an AccessibilityRenderObject, then it's okay if this object // doesn't have a node - there are some renderers that don't have associated - // nodes, like scroll areas and css-generated text. - if (!node() && !isAccessibilityRenderObject()) + // nodes, like scroll areas and css-generated text. However, if there also + // no renderer, we should bail. + if (!renderer() && !node()) return false; Can we really have this scenario? An AccessibilityNodeObject that has neither renderer nor node associated with it? If this is not a possible, expected state, maybe we should add an ASSERT here. @@ -1064,7 +1049,7 @@ static AccessibilityObjectInclusion objectInclusionFromAltText(const String& alt AccessibilityObjectInclusion AccessibilityRenderObject::defaultObjectInclusion() const { - if (!m_renderer) + if (!m_renderer && !node()) return AccessibilityObjectInclusion::IgnoreObject; Same question here. --- a/LayoutTests/platform/glib/accessibility/aria-visible-element-roles-expected.txt +++ b/LayoutTests/platform/glib/accessibility/aria-visible-element-roles-expected.txt @@ -105,12 +105,10 @@ This test ensures ARIA visible elements (e.g. those with both `hidden` and `aria AXRole: AXDescriptionList <ol hidden="" aria-hidden="false" id="ol"></ol> - AXRole: AXList - computedRoleString: list + AXRole: AXGroup <ul hidden="" aria-hidden="false" id="ul"></ul> - AXRole: AXList - computedRoleString: list + AXRole: AXGroup Doesn't this mean we are breaking something for ATSPI? What it was a list before, now is a group. @@ -180,8 +178,7 @@ This test ensures ARIA visible elements (e.g. those with both `hidden` and `aria computedRoleString: status <menu hidden="" aria-hidden="false" type="toolbar" id="menu-toolbar"></menu> - AXRole: AXList - computedRoleString: list + AXRole: AXGroup Dito. --- a/LayoutTests/platform/mac-wk1/accessibility/aria-visible-element-roles-expected.txt +++ b/LayoutTests/platform/mac-wk1/accessibility/aria-visible-element-roles-expected.txt <ol hidden="" aria-hidden="false" id="ol"></ol> - AXRole: AXList - computedRoleString: list + AXRole: AXGroup + AXSubrole: AXContentList <ul hidden="" aria-hidden="false" id="ul"></ul> - AXRole: AXList - computedRoleString: list + AXRole: AXGroup + AXSubrole: AXContentList Dito. @@ -203,8 +204,8 @@ This test ensures ARIA visible elements (e.g. those with both `hidden` and `aria AXSubrole: AXApplicationStatus <menu hidden="" aria-hidden="false" type="toolbar" id="menu-toolbar"></menu> - AXRole: AXList - computedRoleString: list + AXRole: AXGroup + AXSubrole: AXContentList Dito. --- a/LayoutTests/platform/mac-wk2/accessibility/aria-visible-element-roles-expected.txt +++ b/LayoutTests/platform/mac-wk2/accessibility/aria-visible-element-roles-expected.txt <ol hidden="" aria-hidden="false" id="ol"></ol> - AXRole: AXList - computedRoleString: list + AXRole: AXGroup + AXSubrole: AXContentList <ul hidden="" aria-hidden="false" id="ul"></ul> - AXRole: AXList - computedRoleString: list + AXRole: AXGroup + AXSubrole: AXContentList Dito. <menu hidden="" aria-hidden="false" type="toolbar" id="menu-toolbar"></menu> - AXRole: AXList - computedRoleString: list + AXRole: AXGroup + AXSubrole: AXContentList Dito. --- a/LayoutTests/platform/wpe/accessibility/aria-visible-element-roles-expected.txt +++ b/LayoutTests/platform/wpe/accessibility/aria-visible-element-roles-expected.txt Same here, don't understand why these elements were role list before and now they are groups.

(In reply to Andres Gonzalez from comment #8) > (In reply to Tyler Wilcock from comment #7) > > Created attachment 466440 [details] > > Patch > > @@ -516,8 +535,9 @@ bool AccessibilityNodeObject::canHaveChildren() const > { > // If this is an AccessibilityRenderObject, then it's okay if this > object > // doesn't have a node - there are some renderers that don't have > associated > - // nodes, like scroll areas and css-generated text. > - if (!node() && !isAccessibilityRenderObject()) > + // nodes, like scroll areas and css-generated text. However, if there > also > + // no renderer, we should bail. > + if (!renderer() && !node()) > return false; > > Can we really have this scenario? An AccessibilityNodeObject that has > neither renderer nor node associated with it? If this is not a possible, > expected state, maybe we should add an ASSERT here.' These are both WeakPtrs, so the intention is to catch the case where the renderer and node in those WeakPtrs have been destroyed but the AX object has not been cleaned up yet (and thus it should not be allowed to have children, and should compute AccessibilityObjectInclusion::IgnoreObject). > --- > a/LayoutTests/platform/glib/accessibility/aria-visible-element-roles- > expected.txt > +++ > b/LayoutTests/platform/glib/accessibility/aria-visible-element-roles- > expected.txt > @@ -105,12 +105,10 @@ This test ensures ARIA visible elements (e.g. those > with both `hidden` and `aria > AXRole: AXDescriptionList > > <ol hidden="" aria-hidden="false" id="ol"></ol> > - AXRole: AXList > - computedRoleString: list > + AXRole: AXGroup > > <ul hidden="" aria-hidden="false" id="ul"></ul> > - AXRole: AXList > - computedRoleString: list > + AXRole: AXGroup > > Doesn't this mean we are breaking something for ATSPI? What it was a list > before, now is a group. > > Same here, don't understand why these elements were role list before and now > they are groups. Addressed this in my last reply: Yes, all intentional. Because we now properly create display:contents lists as AccessibilityList rather than AccessibilityNodeObject, the more stringent "is this a list" heuristics are applied, and "ARIA visible" lists do not pass that test (because nothing is visibly rendered).

Created attachment 466484 [details] Patch

> > Can we really have this scenario? An AccessibilityNodeObject that has > > neither renderer nor node associated with it? If this is not a possible, > > expected state, maybe we should add an ASSERT here.' > These are both WeakPtrs, so the intention is to catch the case where the > renderer and node in those WeakPtrs have been destroyed but the AX object > has not been cleaned up yet (and thus it should not be allowed to have > children, and should compute AccessibilityObjectInclusion::IgnoreObject). I decided to just remove these checks in the latest patch. These objects should be cleaned up before either of these methods are called.

(In reply to Tyler Wilcock from comment #9) > (In reply to Andres Gonzalez from comment #8) > > (In reply to Tyler Wilcock from comment #7) > > > Created attachment 466440 [details] > > > Patch > > > > @@ -516,8 +535,9 @@ bool AccessibilityNodeObject::canHaveChildren() const > > { > > // If this is an AccessibilityRenderObject, then it's okay if this > > object > > // doesn't have a node - there are some renderers that don't have > > associated > > - // nodes, like scroll areas and css-generated text. > > - if (!node() && !isAccessibilityRenderObject()) > > + // nodes, like scroll areas and css-generated text. However, if there > > also > > + // no renderer, we should bail. > > + if (!renderer() && !node()) > > return false; > > > > Can we really have this scenario? An AccessibilityNodeObject that has > > neither renderer nor node associated with it? If this is not a possible, > > expected state, maybe we should add an ASSERT here.' > These are both WeakPtrs, so the intention is to catch the case where the > renderer and node in those WeakPtrs have been destroyed but the AX object > has not been cleaned up yet (and thus it should not be allowed to have > children, and should compute AccessibilityObjectInclusion::IgnoreObject). > > > --- > > a/LayoutTests/platform/glib/accessibility/aria-visible-element-roles- > > expected.txt > > +++ > > b/LayoutTests/platform/glib/accessibility/aria-visible-element-roles- > > expected.txt > > @@ -105,12 +105,10 @@ This test ensures ARIA visible elements (e.g. those > > with both `hidden` and `aria > > AXRole: AXDescriptionList > > > > <ol hidden="" aria-hidden="false" id="ol"></ol> > > - AXRole: AXList > > - computedRoleString: list > > + AXRole: AXGroup > > > > <ul hidden="" aria-hidden="false" id="ul"></ul> > > - AXRole: AXList > > - computedRoleString: list > > + AXRole: AXGroup > > > > Doesn't this mean we are breaking something for ATSPI? What it was a list > > before, now is a group. > > > > Same here, don't understand why these elements were role list before and now > > they are groups. > Addressed this in my last reply: > > Yes, all intentional. Because we now properly create display:contents lists > as AccessibilityList rather than AccessibilityNodeObject, the more stringent > "is this a list" heuristics are applied, and "ARIA visible" lists do not > pass that test (because nothing is visibly rendered). Sorry that I missed your previous comment and repeated the question. This is not a display:contents list : <ul hidden="" aria-hidden="false" id="ul"></ul> The role of this was list before and now it is group. I understand why the change causes this, but the question is whether this is change in behavior is desirable. IMO, If we are exposing this <ul> at all, it should be a list (empty list), not a generic group.

> Sorry that I missed your previous comment and repeated the question. This is > not a display:contents list : > > <ul hidden="" aria-hidden="false" id="ul"></ul> > > The role of this was list before and now it is group. I understand why the > change causes this, but the question is whether this is change in behavior > is desirable. IMO, If we are exposing this <ul> at all, it should be a list > (empty list), not a generic group. That one is hidden (not visibly rendered, same for its descendants) but "ARIA visible" (aria-hidden="false"). Because nothing is rendered, it also fails the heuristics for "is this a list" (as it should). It's likely that we need to remove this entire notion of exposing ARIA visible (non-rendered) things anyways -- no other browser does.

Created attachment 466511 [details] Patch

Created attachment 466526 [details] Patch

Committed 264644@main (ac7dd8d670b1): <https://commits.webkit.org/264644@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 466526 [details].