1929891 - Issue tree sometimes show non-problematic node that loses their badge on hover and disappear on click

Status: RESOLVED FIXED

(DevTools :: Accessibility Tools, defect, P2)

Description

[Nicolas Chevobbe [:nchevobbe] [PTO - back January 24]]

Attachment: issue

Steps to reproduce

1. Go to https://nchevobbe.github.io/demo/console-test-app.html
2. Select the accessibility panel
3. Reload the page
4. Click the "Check for issues" button and select the "All issues" item

Expected results

only one item is displayed (canvas)

Actual results

Sometimes, the list will contain all the items on the page. When hovered, their "text label" badge disappear. If clicked, the item is removed

---

Looks like some re-rendering issue but I'm still not sure what is happening

Comment 1

[Nicolas Chevobbe [:nchevobbe] [PTO - back January 24]]

I was able to reproduce on ESR 128

Comment 2

[Nicolas Chevobbe [:nchevobbe] [PTO - back January 24]]

[Tracking Requested - why for this release]:

Comment 3

[Nicolas Chevobbe [:nchevobbe] [PTO - back January 24]]

Attachment: Bug 1929891 - [devtools] Run accessible checks sequentially to avoid them impacting each others. r=#devtools.

Comment 4

[Nicolas Chevobbe [:nchevobbe] [PTO - back January 24]]

Note that for some reason, I can only reproduce the issue on pages with @font-face rules in external stylesheets

The issue seems to be triggered from that function call https://searchfox.org/mozilla-release/rev/d8f30fc43d8a411937b53bb95eb9555356e9bcb8/devtools/server/actors/accessibility/audit/keyboard.js#239-244

// Check if an element node has distinct :focus styling.
const hasStylesForFocus = hasStylesForFocusRelatedPseudoClass(
  focusableNode,
  currentNode,
  FOCUS_PSEUDO_CLASS
);

So it looks like there's an issue of InspectorUtils.addPseudoClassLock + focus style + external font-face making accessible getting a null name property, but that's a bit out of my wheelhouse
Emilio, any idea what might be happening here?

I'm still going to land the devtools fix to run things sequentially to be safe, but I guess it would be nice to get to the bottom of this

Comment 5

[Emilio Cobos Álvarez (:emilio)]

So it looks like there's an issue of InspectorUtils.addPseudoClassLock + focus style + external font-face making accessible getting a null name property, but that's a bit out of my wheelhouse

The external font-face must be a red herring. It might be a timing issue or somesuch tho. Do you know what's going on on that check / why does it return false? Are we not returning a style? Something else?

Comment 6

[Nicolas Chevobbe [:nchevobbe] [PTO - back January 24]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #5)

So it looks like there's an issue of InspectorUtils.addPseudoClassLock + focus style + external font-face making accessible getting a null name property, but that's a bit out of my wheelhouse

The external font-face must be a red herring. It might be a timing issue or somesuch tho.

Yes, it could be that, but also not that the STR reproduces quite well even when waiting a long time after the page was loaded, so I guess there might be something triggered when adding the pseudo class lock?

Do you know what's going on on that check / why does it return false? Are we not returning a style? Something else?

So the check doesn't return false itself, it just makes another function (which checks the element accessible name) fail when they are running in parallel https://searchfox.org/mozilla-release/rev/d8f30fc43d8a411937b53bb95eb9555356e9bcb8/devtools/server/actors/accessibility/audit/text-label.js#87-90

const mustHaveNonEmptyNameRule = function (issue, accessible) {
  const name = getAccessibleName(accessible);
  return name ? null : { score: FAIL, issue };
};

calling

https://searchfox.org/mozilla-release/rev/d8f30fc43d8a411937b53bb95eb9555356e9bcb8/devtools/server/actors/accessibility/audit/text-label.js#75-77

function getAccessibleName(accessible) {
  return accessible.name && accessible.name.trim();
}

Here's the definition for accessible.name https://searchfox.org/mozilla-central/rev/94c62970ba2f9c40efd5a4f83a538595425820d9/accessible/interfaces/nsIAccessible.idl#114-124

/**
 * Accessible name -- the main text equivalent for this node. The name is
 * specified by ARIA or by native markup. Example of ARIA markup is
 * aria-labelledby attribute placed on element of this accessible. Example
 * of native markup is HTML label linked with HTML element of this accessible.
 *
 * Value can be string or null. A null value indicates that AT may attempt to
 * compute the name. Any string value, including the empty string, should be
 * considered author-intentional, and respected.
 */
readonly attribute AString name;

The following:

A null value indicates that AT may attempt to compute the name

seems to indicate we have some kind of specific things going on? I don't know what AT means here though (Accessibility Tree ?)

Comment 7

[Emilio Cobos Álvarez (:emilio)]

If you call element.getBoundingClientRect() before accessible.name, does it work? The accessibility tree generally relies on having clean layout and calls into things like GetRenderedText which bails out if dirty: https://searchfox.org/mozilla-central/rev/94c62970ba2f9c40efd5a4f83a538595425820d9/layout/generic/nsTextFrame.cpp#10348-10351

So that'd explain why you're dirtying a frame subtree with the pseudo class locks and that causes the a11y name computation to fail... It seems it'd be more reliable to flush layout before querying / messing with the a11y name, since layout can change the a11y tree more generally.

Comment 8

[Nicolas Chevobbe [:nchevobbe] [PTO - back January 24]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #7)

If you call element.getBoundingClientRect() before accessible.name, does it work?

it does indeed!

The accessibility tree generally relies on having clean layout and calls into things like GetRenderedText which bails out if dirty: https://searchfox.org/mozilla-central/rev/94c62970ba2f9c40efd5a4f83a538595425820d9/layout/generic/nsTextFrame.cpp#10348-10351

So that'd explain why you're dirtying a frame subtree with the pseudo class locks and that causes the a11y name computation to fail... It seems it'd be more reliable to flush layout before querying / messing with the a11y name, since layout can change the a11y tree more generally.

okay, I may do this in a follow up
Thanks a lot for helping me figuring it out Emilio

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/89995f2da983
[devtools] Run accessible checks sequentially to avoid them impacting each others. r=bomsy.

Comment 10

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/89995f2da983