Make ISimpleDOM work on X64

RESOLVED FIXED in Firefox 40

Status

()

Core
Disability Access APIs
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: MarcoZ, Assigned: surkov)

Tracking

Trunk
mozilla41
x86_64
Windows
Points:
---

Firefox Tracking Flags

(firefox38 unaffected, firefox38.0.5 wontfix, firefox39 wontfix, firefox40 fixed, firefox41 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

The SDN part also needs to use the new 32 bit IDs introduced in bug 606080. NVDA now works in 64 bit Firefox, JAWS doesn't. JAWS still uses ISimpleDOM heavily and thus needs to get the correct IDs.
Try push with a WIP patch:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=b38b22a215ef
Created attachment 8602651 [details] [diff] [review]
WIP

Alex, any ideas? After the patch for bug 606080 landed, NVDA now works fine with 64 bit builds of nightly (or try server), but JAWS doesn't. It invokes its virtual buffer, but the only content it gets is the NetscapeHWND or whatever it calls it. I checked, and we seem to be firing the window emulation JAWS still relies on.

So my hunch was to do the same as for the Unique IDs in IAccessible, for SDNAccessible. I worked up this patch, which compiles, but the result is the same. The virtual buffer seems to be invoked, but no content is being added. And debugging doesn't help me because the screen reader and browser run into a deadlock as soon as I try to run a local debug build. Same debug problems as usual.
Attachment #8602651 - Flags: feedback?(surkov.alexander)
Opt Release build for the try run can be found here: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mzehe@mozilla.com-b38b22a215ef/try-win64/
(Assignee)

Comment 4

3 years ago
(In reply to Marco Zehe (:MarcoZ) from comment #2)

> So my hunch was to do the same as for the Unique IDs in IAccessible, for
> SDNAccessible. I worked up this patch, which compiles, but the result is the
> same. 

perhaps you're right and they rely on uniqueID for DOM node (your patch makes it 0 when DOM node is not accessible). I'll check it out with them.
FYI, Window-Eyes is also not able to access the document.

(In reply to alexander :surkov from comment #4)
> perhaps you're right and they rely on uniqueID for DOM node (your patch
> makes it 0 when DOM node is not accessible). I'll check it out with them.

But if the DOM node is accessible, it should be correct, right? Or is this the totally wrong approach to the problem of getting the Unique ID for SDNAccessible?
Flags: needinfo?(surkov.alexander)
(Assignee)

Comment 6

3 years ago
(In reply to Marco Zehe (:MarcoZ) from comment #5)
> FYI, Window-Eyes is also not able to access the document.
> 
> (In reply to alexander :surkov from comment #4)
> > perhaps you're right and they rely on uniqueID for DOM node (your patch
> > makes it 0 when DOM node is not accessible). I'll check it out with them.
> 
> But if the DOM node is accessible, it should be correct, right?

yes

> Or is this
> the totally wrong approach to the problem of getting the Unique ID for
> SDNAccessible?

I don't their internals, I asked them, let's see.
Flags: needinfo?(surkov.alexander)
(In reply to Marco Zehe (:MarcoZ) from comment #5)
> FYI, Window-Eyes is also not able to access the document.
> 
> (In reply to alexander :surkov from comment #4)
> > perhaps you're right and they rely on uniqueID for DOM node (your patch
> > makes it 0 when DOM node is not accessible). I'll check it out with them.
> 
> But if the DOM node is accessible, it should be correct, right? Or is this
> the totally wrong approach to the problem of getting the Unique ID for
> SDNAccessible?

they might also need a unique id from the UIA stuff which also needs to be fixed, but there I think you can just give them a 64 bit unique id.
(Assignee)

Comment 8

3 years ago
Created attachment 8604238 [details] [diff] [review]
patch
Assignee: nobody → surkov.alexander
Attachment #8602651 - Attachment is obsolete: true
Attachment #8602651 - Flags: feedback?(surkov.alexander)
Attachment #8604238 - Flags: review?(mzehe)
Comment on attachment 8604238 [details] [diff] [review]
patch

Yes, this fixes the problem. Thank you! (BTW: This is written with a local build that contains this patch, using JAWS.)
Attachment #8604238 - Flags: review?(mzehe) → review+
Confirming that this also works with Window-Eyes.

Comment 11

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb2201af1e2f
Keywords: leave-open
Created attachment 8605247 [details] [diff] [review]
Part 2 - Fix ISimpleDOM Unique ID retrieval.
Attachment #8605247 - Flags: review?(surkov.alexander)
(Assignee)

Comment 13

3 years ago
Comment on attachment 8605247 [details] [diff] [review]
Part 2 - Fix ISimpleDOM Unique ID retrieval.

Review of attachment 8605247 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/windows/sdn/sdnAccessible.cpp
@@ +106,5 @@
>    // focus events, to correlate back to data nodes in their internal object
>    // model.
> +  AccessibleWrap* accessible = static_cast<AccessibleWrap*>(GetAccessible());
> +  if (accessible) {
> +    *aUniqueID = AccessibleWrap::GetChildIDFor(accessible);

GetChildIdFor takes Accessible*, you don't need to cast it to wrap
Attachment #8605247 - Flags: review?(surkov.alexander) → review+

Comment 14

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/7af7c273097d
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/eb2201af1e2f
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
https://hg.mozilla.org/mozilla-central/rev/7af7c273097d
[Tracking Requested - why for this release]:

If bug 606080 is permitted for 39, the patches in this bug need to land there, too. Setting "Affected" flag for that and requesting tracking.
status-firefox39: --- → affected
tracking-firefox39: --- → ?
Comment on attachment 8604238 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]: bug 606080
[User impact if declined]: Users of JAWS and Window-Eyes screen readers will not be able to use 64-bit editions of Firefox. JAWS is the market-leading screen reader.
[Describe test coverage new/current, TreeHerder]: Manually tested on local and mozilla-central builds.
[Risks and why]: Low ris,, this patch duplicated behavior from widget code needed for a call screen readers are making to a WndProc function.
[String/UUID change made/needed]: None.
Attachment #8604238 - Flags: approval-mozilla-aurora?
Comment on attachment 8605247 [details] [diff] [review]
Part 2 - Fix ISimpleDOM Unique ID retrieval.

Approval Request Comment
[Feature/regressing bug #]: bug 606080
[User impact if declined]: Some functionality in commercial screen readers JAWS and Window-Eyes may be impaired.
[Describe test coverage new/current, TreeHerder]: Tested on local build and mozilla-central build.
[Risks and why]: Low risk, this just duplicates some code introduced in bug 606080 and uses a function introduced there on 64 bit systems.
[String/UUID change made/needed]: None.
Attachment #8605247 - Flags: approval-mozilla-aurora?
Attachment #8605247 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8604238 [details] [diff] [review]
patch

Taking it as we care about accessibility
Attachment #8604238 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/b3034590e03f
https://hg.mozilla.org/releases/mozilla-aurora/rev/e8e924727dba
status-firefox40: affected → fixed
Marco, can you request uplift to beta also so we can get this fix into 39?
Flags: needinfo?(mzehe)
Wait, I just realized that is wrong, as we didn't take the fix for bug 606080 in 39. So I guess we have to wontfix this as well.   I am marking 38.0.5 as wontfix assuming the win64 builds for 38.0.5 are also affected.
status-firefox38: --- → unaffected
status-firefox38.0.5: --- → wontfix
status-firefox39: affected → wontfix
tracking-firefox39: ? → ---
Flags: needinfo?(mzehe)
Liz, yes, they are definitely affected. But as you said yourself, if we don't take bug 606080 for 39 after all, we won't need this one either. If that decision changes, however, we'll have to revisit this one, too.
You need to log in before you can comment on or make changes to this bug.