Closed Bug 1226473 Opened 6 years ago Closed 1 year ago

Firefox does not read selected text

Categories

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

x86
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla80
Tracking Status
e10s + ---
firefox80 --- fixed

People

(Reporter: marcosc, Assigned: eeejay)

References

Details

(Whiteboard: [mac2020_2])

Attachments

(2 files)

Screen reader support seems to have completely stopped working in Firefox in El Capitan. 

Steps:

1. select any text on a page. 
2. press Apple's key combo to read text (alt-esc) for me.

Expected:
Text to be read. 

Actual: 
Nothing is read. 


The only time it seems to work is in text areas. But doesn't work on any other text.
Actually, sometimes it just starts reading the whole tab instead of the selected text.
Component: Disability Access → Widget: Cocoa
Product: Firefox → Core
Version: unspecified → Trunk
Marcos, does this work with e10s disabled? In Nightly > Preferences > General > Startup > (uncheck) Enable multi-process Nightly
Flags: needinfo?(mcaceres)
(In reply to Tracy Walker [:tracy] from comment #2)
> Marcos, does this work with e10s disabled? In Nightly > Preferences >
> General > Startup > (uncheck) Enable multi-process Nightly

Yes. It seems to work fine without e10s enabled.
Flags: needinfo?(mcaceres)
Actually, it only works for a little while... then I just breaks again and goes back to reading the whole page.
e10s is the new standard so we really need to fix this there.
Blocks: e10sa11y2
tracking-e10s: --- → +
Priority: -- → P1
Whiteboard: tpi:+
Whiteboard: tpi:+ → aes-osx
Whiteboard: aes-osx → aes+
Whiteboard: aes+ → aes+, tpi:+
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1261299
This is not a duplicate of bug 1261299.
This problem occurs in non-e10s as well.

I'm not familiar with our osx speech synthesis code.

The first text selection when you press (alt-esc) works. But if you press it again or make a new selection and press (alt-esc). You got these logs
> AXGroup can't respond to attribute AXSelectedText
> AXGroup can't respond to attribute AXSelectedTextMarkerRange

This bug should depend on bug 1261299 because it caches the current selection as a transferable, which this can use.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Duplicate of this bug: 1221041
(In reply to Jimmy Wang (:jimicy) - works on e10s stuff from comment #7)
> This is not a duplicate of bug 1261299.
> This problem occurs in non-e10s as well.
> 
> I'm not familiar with our osx speech synthesis code.
> 
> The first text selection when you press (alt-esc) works. But if you press it
> again or make a new selection and press (alt-esc). You got these logs
> > AXGroup can't respond to attribute AXSelectedText
> > AXGroup can't respond to attribute AXSelectedTextMarkerRange

These are from the mac Accessible implementation ( http://searchfox.org/mozilla-central/rev/03b3c20a6ec60435feb995a2a23c5926188c85a1/accessible/mac/mozAccessible.mm#439 ). Yura, do you know more about that and/or do you know someone who does?
Depends on: 1261299
Flags: needinfo?(yzenevich)
I'm not familiar with this code, perhaps Alex would know?
Flags: needinfo?(yzenevich) → needinfo?(surkov.alexander)
(In reply to :Gijs Kruitbosch (PTO recovery mode) from comment #9)
> (In reply to Jimmy Wang (:jimicy) - works on e10s stuff from comment #7)
> > This is not a duplicate of bug 1261299.
> > This problem occurs in non-e10s as well.
> > 
> > I'm not familiar with our osx speech synthesis code.
> > 
> > The first text selection when you press (alt-esc) works. But if you press it
> > again or make a new selection and press (alt-esc). You got these logs
> > > AXGroup can't respond to attribute AXSelectedText
> > > AXGroup can't respond to attribute AXSelectedTextMarkerRange
> 
> These are from the mac Accessible implementation (
> http://searchfox.org/mozilla-central/rev/
> 03b3c20a6ec60435feb995a2a23c5926188c85a1/accessible/mac/mozAccessible.mm#439
> ). Yura, do you know more about that and/or do you know someone who does?

given that implementation of the function doesn't handle that attribute it seems like we are creating the wrong sub class of mozAccessible, but other than that somebody just needs to debug it.
It's interesting but it seems we implement neither AXSelectedText nor AXSelectedTextMarkerRange. Steven fixed a similar bug a while ago a different way, bug 674612, maybe VoiceOver expectations were changed.

Marco, do you have ideas?
Flags: needinfo?(surkov.alexander)
Unfortunately I do not. VoiceOver was never involved in the bug Steven fixed. That was the Edit/Speak Selected Text feature, which bypasses VoiceOver and takes text from the app and sends it to the speech system directly. That's a different OS X function that has nothing to do with VoiceOver.
Whiteboard: aes+, tpi:+ → tpi:+
I have a patch that fixes this, but I want to figure out why it works and if it has side-effects before requesting review.
Assignee: nobody → spohl.mozilla.bugs
Status: REOPENED → ASSIGNED
Attached patch PatchSplinter Review
The reason why VoiceOver is able to read the selected text the first time is because |accessibleParent| is NULL the first time and we call |GetNativeFromGeckoAccessible(accWrap->RootAccessible())| instead of |GetNativeFromGeckoAccessible(accessibleParent);|. Every subsequent attempt of making VoiceOver read the selected text fails because |accessibleParent| points to a valid object after the first time. We seem to be infinitely looping around when this is the case, which appears to be parent to child and back again (but I haven't been able to confirm for sure yet. Too much console spam).

This patch fixes the issue and VoiceOver is able to successfully read the selected text every time. We also don't have any infinite loops anymore.

Lorien, I don't know enough about this code to know if this is a valid fix, but I'm hoping that by looking at the patch, you could see what might be going on and how to fix it properly. Thanks!
Attachment #8798139 - Flags: feedback?(lorien)
(In reply to Stephen A Pohl [:spohl] (OOO 10/13-10/20) from comment #15)
> Created attachment 8798139 [details] [diff] [review]
> Patch
> 
> The reason why VoiceOver is able to read the selected text the first time is
> because |accessibleParent| is NULL the first time and we call
> |GetNativeFromGeckoAccessible(accWrap->RootAccessible())| instead of
> |GetNativeFromGeckoAccessible(accessibleParent);|. Every subsequent attempt
> of making VoiceOver read the selected text fails because |accessibleParent|
> points to a valid object after the first time. We seem to be infinitely
> looping around when this is the case, which appears to be parent to child
> and back again (but I haven't been able to confirm for sure yet. Too much
> console spam).

afaik the parent is supposed to return immediate parent, not the root object. I'm not sure I've caught your correctly about looping, do we ever hang? otherwise how the loop gets broken?

> This patch fixes the issue and VoiceOver is able to successfully read the
> selected text every time. We also don't have any infinite loops anymore.

how does selected text is related with root object as a parent?

> Lorien, I don't know enough about this code to know if this is a valid fix,
> but I'm hoping that by looking at the patch, you could see what might be
> going on and how to fix it properly. Thanks!

Lorien is not around these days, you can ni? me instead, I don't know much about mac a11y part, but I'll do my best.
Attachment #8798139 - Flags: feedback?(lorien)
Looks like this is an a11y related bug vs. widget so moving this over.
Assignee: spohl.mozilla.bugs → nobody
Status: ASSIGNED → NEW
Component: Widget: Cocoa → Disability Access APIs
Priority: P1 → P3
Whiteboard: tpi:+
(In reply to alexander :surkov from comment #16)
> (In reply to Stephen A Pohl [:spohl] (OOO 10/13-10/20) from comment #15)
> > Created attachment 8798139 [details] [diff] [review]
> > Patch
> > 
> > The reason why VoiceOver is able to read the selected text the first time is
> > because |accessibleParent| is NULL the first time and we call
> > |GetNativeFromGeckoAccessible(accWrap->RootAccessible())| instead of
> > |GetNativeFromGeckoAccessible(accessibleParent);|. Every subsequent attempt
> > of making VoiceOver read the selected text fails because |accessibleParent|
> > points to a valid object after the first time. We seem to be infinitely
> > looping around when this is the case, which appears to be parent to child
> > and back again (but I haven't been able to confirm for sure yet. Too much
> > console spam).
> 
> afaik the parent is supposed to return immediate parent, not the root
> object. I'm not sure I've caught your correctly about looping, do we ever
> hang? otherwise how the loop gets broken?

I'm not familiar with this code and I was just stating my observations.

> > This patch fixes the issue and VoiceOver is able to successfully read the
> > selected text every time. We also don't have any infinite loops anymore.
> 
> how does selected text is related with root object as a parent?

I was simply pointing out that this patch appears to fix the issue, which may help in diagnosing and fixing the problem correctly.

> > Lorien, I don't know enough about this code to know if this is a valid fix,
> > but I'm hoping that by looking at the patch, you could see what might be
> > going on and how to fix it properly. Thanks!
> 
> Lorien is not around these days, you can ni? me instead, I don't know much
> about mac a11y part, but I'll do my best.

It seems like someone with greater familiarity with a11y should take this over.
Duplicate of this bug: 1592649
Whiteboard: [mac2020_1]
Whiteboard: [mac2020_1] → [mac2020_2]
Severity: normal → S3
Priority: P3 → P1
Duplicate of this bug: 1408620
Assignee: nobody → eitan
Depends on: 1649217

To do this well we need to cache the text selection in the top level process.

Pushed by eisaacson@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/92d97e70eb6b
Support AXSelectedTextMarkerRange. r=morgan
Status: NEW → RESOLVED
Closed: 5 years ago1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla80
You need to log in before you can comment on or make changes to this bug.