1520779 - NVDA falls back to “line” text unit resolution as Firefox doesn’t properly support the “paragraph” resolution

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect, P2)

Description

[Emil Ghitta, QA [:emilghitta]]

Affected versions

• Firefox 66.0a1 (BuildId:20190116170105)
• Firefox 65.0b11 (BuildId:20190114172331)
• Firefox 64.0.2 (BuildId:20190108160530)
• Firefox 60.4.0esr (BuildId:20181203164059)

Affected platforms

• Windows 10 64bit.
• Windows 7 32bit.

Preconditions

• Have NVDA screen reader enabled with the “Text unit resolution” set to “paragraph”.

Steps to reproduce

1. Launch Firefox.
2. Access the following webpage: https://www.lipsum.com/
3. Hover over the first paragraph.

Expected result

• NVDA successfully reads the entire paragraph.

Actual result

• NVDA reads only the first line of the paragraph and the user has to hover over the next line in order to be read.

Regression range

• I don’t think that this is a regression.

Additional notes

• This is not reproducible on Google Chrome.

Comment 3

[James Teh [:Jamie]]

This is also super annoying for braille users who have "Read by paragraph" enabled in NVDA braille settings. Instead of reading by paragraph (which often means less pointless panning given that braille lines generally don't correspond with screen lines), you can only read by line.

Comment 4

[James Teh [:Jamie]]

I was just pinged about this by Vispero. JAWS wants this too.

Comment 5

[Marco Zehe (:MarcoZ)]

Attachment: Bug 1520779 Part 1: Add the paragraph boundary to nsIAccessibleText, r?jamie!

Comment 6

[Marco Zehe (:MarcoZ)]

Attachment: Bug 1520779 Part 2: Implement nsIAccessibleText::BOUNDARY_PARAGRAPH in HyperTextAccessible::GetTextAtOffset, r?jamie!

Depends on D90114

Comment 7

[Marco Zehe (:MarcoZ)]

Attachment: Bug 1520779 Part 3: Hook up the IA2_TEXT_BOUNDARY_PARAGRAPH to the internal nsIAccessibleText::BOUNDARY_PARAGRAPH support, r?jamie!

Depends on D90115

Comment 8

[Marco Zehe (:MarcoZ)]

Attachment: Bug 1520779 Part 4: Add tests to exercise paragraph retrieval, r?jamie!

Depends on D90116

Comment 9

[Marco Zehe (:MarcoZ)]

Try run is here. Now waiting for Jamie to throw edge cases at me. ;-) And we need to decide whether we want to also have GetTextBefore and GetTextAfterOffset versions of this. NVDA doesn't call this ever, I've also reached out to Vispero to find out if they need it. Linux doesn't seem to use this granularity type at all, at least we have no reference in our code anywhere about any ATK granularities.

Comment 10

[James Teh [:Jamie]]

(In reply to Marco Zehe (:MarcoZ) from comment #9)

And we need to decide whether we want to also have GetTextBefore and GetTextAfterOffset versions of this.

Ug. I always found those methods so confusing, as well as the separate start and end boundaries.

Eitan, do we need to support GetTextBeforeOffset and GetTextAfterOffset for paragraph? And do we need separate paragraph start and paragraph end boundaries? I honestly don't understand why we need these at all for any boundary type, but I know you have a bit more experience with that and needed them for some things on Android and mac.

It seems to me that paragraphs should always be directly adjacent; i.e. there shouldn't be white space we would ever exclude, whereas there could be for lines and words.

Linux doesn't seem to use this granularity type at all, at least we have no reference in our code anywhere about any ATK granularities.

It looks like we just pass the boundary type from ATK to Gecko without any checking or normalisation, just assuming the constants match. And this isn't documented anywhere, even in a code comment. So that's fun.
https://searchfox.org/mozilla-central/rev/0c97a6410ff018c22e65a0cbe4e5f2ca4581b22e/accessible/atk/nsMaiInterfaceText.cpp#224

It looks like the AtkTextBoundary enum we use is deprecated. It doesn't have a paragraph boundary. It seems there is a newer ATKTextGranularity enum which does have a paragraph boundary and also gets rid of the separate START and END boundaries and just has LINE, WORD, etc. As a result, it uses a new atk_text_get_string_at_offset method. So, it's more like the way NVDA does things with IA2: no start or end boundaries, no before or after offset.

With a quick grep, it doesn't look like Orca uses this new granularity thing, though I could be missing something. I also couldn't find any reference to any paragraph boundary or granularity.

Long story short: I don't think we need to do anything here for ATK at this stage, but it might be something we need to look into in future.

Comment 11

[Marco Zehe (:MarcoZ)]

Attachment: Bug 1520779 Part 5: Expose both text and list bullet when requesting the paragraph from a list item, r?jamie!

Tagging onto what we already do for getting line offsets, also doing the same when the paragraph selection is requested.

Comment 12

[Eitan Isaacson [:eeejay]]

This could be useful in Android!

Whether we need the before/after methods. I'm not sure yet, but I think its fine if it isn't in the scope of this patch.

If we revisit and add android support for this, we will see.

Comment 13

[Marco Zehe (:MarcoZ)]

Attachment: Bug 1520779 Part 6: Return consistent results for paragraphs that contain forced line breaks, r?jamie!

1. Make sure the paragraph text and start and end ofsets returned for offsets on either the text or its adjacent br always match.
2. For two consecutive line breaks, treat the second one as its own paragraph without text, and its start offset corresponding to its offset, and end offset being 1 greater.
3. Add a test case exercising all cases.

Comment 14

[Marco Zehe (:MarcoZ)]

Newest try build after a round of changes and hopefully all current edge cases fixed.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4cf65b0cd304
Part 1: Add the paragraph boundary to nsIAccessibleText, r=Jamie
https://hg.mozilla.org/integration/autoland/rev/3ca759ecb3d7
Part 2: Implement nsIAccessibleText::BOUNDARY_PARAGRAPH in HyperTextAccessible::GetTextAtOffset, r=Jamie,eeejay
https://hg.mozilla.org/integration/autoland/rev/6ddb6ee83467
Part 3: Hook up the IA2_TEXT_BOUNDARY_PARAGRAPH to the internal nsIAccessibleText::BOUNDARY_PARAGRAPH support, r=Jamie
https://hg.mozilla.org/integration/autoland/rev/2e3924874d25
Part 4: Add tests to exercise paragraph retrieval, r=Jamie
https://hg.mozilla.org/integration/autoland/rev/3e53ec41f414
Part 5: Expose both text and list bullet when requesting the paragraph from a list item, r=Jamie
https://hg.mozilla.org/integration/autoland/rev/1d5de85e5c45
Part 6: Return consistent results for paragraphs that contain forced line breaks, r=Jamie

Comment 16

[Dorel Luca [:dluca]]

Backed out 6 changesets (bug 1520779) for Mochitest failures in accessible/tests/mochitest/tree/test_img.html. CLOSED TREE

Log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=315890410&repo=autoland&lineNumber=5010

https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=315891705&repo=autoland&lineNumber=1584

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&revision=1d5de85e5c45c8ea4a1b786a488aa0a590fddb47

Backout:
https://hg.mozilla.org/integration/autoland/rev/26e08aca9d23be58347481adb17343201e17af9e

Comment 17

[James Teh [:Jamie]]

That's bizarre. I don't see how HyperTextAccessible changes or tests could have caused those failures. I wonder if there were any other a11y patches landed around the same time that might have caused this?

Comment 18

[Marco Zehe (:MarcoZ)]

Both try runs showed no indication of problems. This failure definitely seems unrelated to this patch series.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6b04d7de4980
Part 1: Add the paragraph boundary to nsIAccessibleText, r=Jamie
https://hg.mozilla.org/integration/autoland/rev/b4e833b459b2
Part 2: Implement nsIAccessibleText::BOUNDARY_PARAGRAPH in HyperTextAccessible::GetTextAtOffset, r=Jamie,eeejay
https://hg.mozilla.org/integration/autoland/rev/949cda3f4a12
Part 3: Hook up the IA2_TEXT_BOUNDARY_PARAGRAPH to the internal nsIAccessibleText::BOUNDARY_PARAGRAPH support, r=Jamie
https://hg.mozilla.org/integration/autoland/rev/c972513d9991
Part 4: Add tests to exercise paragraph retrieval, r=Jamie
https://hg.mozilla.org/integration/autoland/rev/d9dd1a2cf2cb
Part 5: Expose both text and list bullet when requesting the paragraph from a list item, r=Jamie
https://hg.mozilla.org/integration/autoland/rev/338bbaf179ae
Part 6: Return consistent results for paragraphs that contain forced line breaks, r=Jamie

Comment 20

[Marco Zehe (:MarcoZ)]

This push apparently failed again. However, a try push that turned on everything Linux that could be turned on via try chooser, came back green. So whatever it is that' failing is a) a difference between try and autoland, and b) not related to this patch series. The two failing tests test image maps by moving the mouse over the image map area, waiting for an accessible Reorder event to fire, and then test the resulting tree. They time out, and usually if that happens, it means that the event we were waiting for never fired.

Comment 21

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/6b04d7de4980
https://hg.mozilla.org/mozilla-central/rev/b4e833b459b2
https://hg.mozilla.org/mozilla-central/rev/949cda3f4a12
https://hg.mozilla.org/mozilla-central/rev/c972513d9991
https://hg.mozilla.org/mozilla-central/rev/d9dd1a2cf2cb
https://hg.mozilla.org/mozilla-central/rev/338bbaf179ae