Closed Bug 1162434 Opened 9 years ago Closed 9 years ago

Make ISimpleDOM work on X64

Categories

(Core :: Disability Access APIs, defect)

x86_64
Windows
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox38 --- unaffected
firefox38.0.5 --- wontfix
firefox39 --- wontfix
firefox40 --- fixed
firefox41 --- fixed

People

(Reporter: MarcoZ, Assigned: surkov)

References

Details

Attachments

(2 files, 1 obsolete file)

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.
Attached patch WIP (obsolete) — Splinter Review
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)
(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)
(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.
Attached patch patchSplinter Review
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.
Keywords: leave-open
Attachment #8605247 - Flags: review?(surkov.alexander)
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+
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/eb2201af1e2f
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
[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.
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+
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.
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.