1822340 - Using Firefox Nightly with GitHub Blog I can no longer read the links on the page with NVDA using the mouse

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect)

Description

[4RJames]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:109.0) Gecko/20100101 Firefox/113.0

Steps to reproduce:

I'm noticing another detail with links in web page content

This web page is an example I have read with both Firefox browsers
https://github.blog/2023-03-09-how-github-accelerates-development-for-embedded-systems/

Actual results:

When you mouse over content that includes embedded links

With Firefox stable they are spoken with mouse over

However, in Firefox Nightly they are not

Expected results:

With Firefox stable they are spoken with mouse over

Comment 1

[4RJames]

BTW
When I received the email notification onf this bug
This line of the message could not be read when using the mouse in GMail with Firefox nightly:
Configure your email settings at https://bugzilla.mozilla.org/userprefs.cgi?tab=email.

I think it is because the link is at the end of the sentence
However, it can be read in this edit box...

Comment 2

[James Teh [:Jamie]]

Thanks for reporting. I can confirm this.

Distilled test case (using NVDA):

1. Open this test case:
   data:text/html,<p style="width: 3ch; font-family: monospace;"><a href="https://example.com/">a</a>b cd
2. Move the mouse to the link.
   • Expected: NVDA should report "a"
   • Actual: It doesn't report any text.

What's happening here is that " bcd" is a text node that wraps across the first and second lines. That means its rect actually covers the start of the first line, since the "b" is on the first line but the "c" is at the very left of the paragraph. Because the "bcd" text leaf is earlier in the hit testing order and the requested point is within it, we return it instead of continuing to the link.

Comment 3

[James Teh [:Jamie]]

To fix this, when we find a text leaf in ChildAtPoint, I think we're going to need to look at the character rects. Walking the character rects for the entire node will be way too expensive. Instead, for each line start, we can:

1. Use the line start cache to get the start and inclusive end offset of each line.
2. Get the character rects for the start and end offset.
3. Union them to get the line rect.
4. Check if the point is within that rect.

This is similar to what we do in TextLeafRange::Bounds, but because we only need this for RemoteAccessible, we only need it within a single text leaf (not a HyperText) and we are always walking from the first line start, I think it'll be easier to use the cache directly rather than TextLeafPoint.

Comment 4

[James Teh [:Jamie]]

It's a bit more complicated than this. Our character rects are also incorrect because we assume that the acc's bounds originate at the top left of the primary frame, but they don't in this case for the same reason. So, we also need to compensate for this when calculating character rects.

Comment 5

[James Teh [:Jamie]]

Morgan, I'm trying to write a text bounds test for this. However, even when I test this in the parent process (where we know this works), the y coordinate is wrong even for offset 0 which doesn't wrap. Here's the test (added to the bottom of browser_caching_text_bounds.js):

/**
 * Test wrapping text.
 */
addAccessibleTask(
  `
<p id="wrappingText" style="width: 3ch; font-family: monospace;">
  <a href="https://example.com/">a</a>b cd
</p>
  `,
  async function(browser, docAcc) {
    await testChar(docAcc, browser, "wrappingText", 0);
    await testChar(docAcc, browser, "wrappingText", 1);
    await testChar(docAcc, browser, "wrappingText", 2);
    await testChar(docAcc, browser, "wrappingText", 3);
  },
  { chrome: true, topLevel: false }
);

Any idea what's going on here? I think this might be a bug in testTextRange, but I can't figure out what's wrong at present.

I did notice this:

We use getBoundsInCSSPixels to avoid factoring in the DPR ourselves.

I'm confused by this. I think testTextPos and testTextBounds use CharBounds()/TextBounds(), which I don't think use CSS pixels? That said, I guess this probably doesn't make a difference except with DPI scaling, and it also doesn't explain why all the other tests work just fine.

Comment 6

[James Teh [:Jamie]]

Aha. This breaks because of the white space which doesn't get rendered. If I do this:

<p>abcd</p>

it works, but if I do this:

<p>
  abcd
</p>

it fails. We could tweak testTextRange to fix this, but it's probably not worth the effort.

Comment 7

[James Teh [:Jamie]]

Attachment: Bug 1822340 part 1: Fix cached text bounds for wrapped text which starts part way through a line or where the primary text frame is empty.

Comment 8

[James Teh [:Jamie]]

Attachment: Bug 1822340 part 2: When hit testing using the cache, only match a TextLeafAccessible if one of its lines includes the requested point.

Previously, we always used the Accessible's rect.
However, if the text wraps across lines, its rect might cover a wider area than the actual text.
That meant we were matching a wrapped text leaf when we shouldn't in some cases.
Now, we restrict text leaf matches to the rects occupied by text.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/56df1407576e
part 1: Fix cached text bounds for wrapped text which starts part way through a line or where the primary text frame is empty. r=morgan
https://hg.mozilla.org/integration/autoland/rev/9ee28f249e2d
part 2: When hit testing using the cache, only match a TextLeafAccessible if one of its lines includes the requested point. r=morgan

Comment 10

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/56df1407576e
https://hg.mozilla.org/mozilla-central/rev/9ee28f249e2d

Comment 11

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:Jamie, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox112 to wontfix.

For more information, please visit auto_nag documentation.

Comment 12

[James Teh [:Jamie]]

Comment on attachment 9324104 [details]
Bug 1822340 part 1: Fix cached text bounds for wrapped text which starts part way through a line or where the primary text frame is empty.

Beta/Release Uplift Approval Request

• User impact if declined: Incorrect screen reader mouse/touch tracking in some cases.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Covered by automated tests. Only impacts a11y hit testing and only while the a11y cache is disabled (so we could pref off the cache in an absolute worst case scenario).
• String changes made/needed:
• Is Android affected?: No

Comment 13

[Dianna Smith [:diannaS]]

Comment on attachment 9324104 [details]
Bug 1822340 part 1: Fix cached text bounds for wrapped text which starts part way through a line or where the primary text frame is empty.

Approved for 112.0b7

Comment 14

[Dianna Smith [:diannaS]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a7d4b87b6f99
https://hg.mozilla.org/releases/mozilla-beta/rev/8784a6f2bb89

Comment 15

[Timea Zsoldos [:zstimi/tzsoldos], Desktop QA]

I have reproduced this issue using Firefox 113.0a1 (2023.03.14) on Windows 10 x64.
I can confirm this issue is fixed, I verified using Firefox 113.0a1 latest nightly and Firefox 112.0b7 on Windows 10, Ubuntu 22 and on macOS 10.15, opening test case from comment 2 NVDA report "a".

Comment 16

[4RJames]

I went back to the web page used in the original report
When I mouse over the paragraph below that has embedded links
The reading stops without completing the paragraph

We know that DevOps is all about the continuous delivery of value to end-users. For these types of systems, quality and security are fundamental, non-negotiable aspects. Whether it’s ISO 26262 for automotive, IEC 62304 for medical devices, or any of the other myriad of functional safety standards, compliance to the standards adds significant complexity to the development process. When you add in ISO 27001 and the new UN regulatory requirements for cybersecurity on top of industry coding standards like AUTOSAR and Misra, you can see why finding ways to automate becomes even more critical in the world of embedded development.

However, here in this edit box it reads the entire paragraph...

In one of my comments earlier in this issue I mentioned another example line that has an embedded link at the end and it si not spoken.
When reviewing this issue that line is still silent.

Sorry, I don't know if these are different problems but they are still problems...

Comment 17

[James Teh [:Jamie]]

This one is due to a bug in NVDA. I've double checked that Firefox is reporting what it should be and also that the same bug occurs even with Firefox's new caching disabled.

Comment 18

[James Teh [:Jamie]]

Note to self: This line in NVDA's IA2TextTextInfo is causing the problem:
self._endOffset = text.index(textUtils.OBJ_REPLACEMENT_CHAR, oldEnd - self._startOffset)
_startOffset should probably be _endOffset.

Comment 19

[4RJames]

Thank you, do you know if this has been reported to NVDA?

I don't understand why that specific paragraph reads correctly with the mouse here in the bugzilla comments but not on the original web page...
Using NVDA in both cases...

Comment 20

[James Teh [:Jamie]]

I'm not sure if it's been reported to NVDA. I'll take a look and file a bug when I get a chance if it hasn't been filed already, as I can see pretty clearly which code is causing the problem.

It doesn't happen on Bugzilla because Bugzilla doesn't include the links from the paragraph. The links trigger the problem. Note that even once this is fixed, you'll still have to move to the link and then to the text after the link. NVDA won't ever read the paragraph in its entirety. That is currently expected behaviour, though it's arguably not ideal.

Comment 21

[James Teh [:Jamie]]

This issue was already filed with NVDA: https://github.com/nvaccess/nvda/issues/9235
I submitted a change to fix it: https://github.com/nvaccess/nvda/pull/14755