Closed Bug 612872 Opened 9 years ago Closed 9 years ago

HAL/Supernova needs more sophisticated window emulation

Categories

(Core :: Disability Access APIs, defect)

x86
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0
Tracking Status
blocking2.0 --- final+

People

(Reporter: davidb, Assigned: surkov)

References

Details

(Keywords: regression, Whiteboard: [softblocker][has patch][f+ Dolphin])

Attachments

(1 file, 1 obsolete file)

"After investigating a bit further, it seems that it is the properties of the
emulated windows that prevents them being detected by Supernova, if I check
with spy++ they are indeed there. The emulated windows have a size of 0 x 0,
which causes them to be discarded by Supernova's window detector, is it
possible to have them set to the size of the document as in previous
versions? If not, some arbitrary nonzero size may be ok, but I haven't been
able to test this. Also, I noticed that when multiple document tabs are
open, they all have the WS_VISIBLE style set, even the background tabs. Is
it possible to only apply this style to the foreground tab as in earlier
versions? If we can solve this then I am confident that our previous
versions should work just fine."
Assignee: nobody → bolterbugz
blocking2.0: --- → final+
Waiting for a product key from Dolphin to make debugging easier.
Got the key, tried making the native windows 10x10 and SuperNova started working again so we are looking good here. Patch to come later.
Ignore comment #2, I launched FF3.6 by mistake for that test. The problem is more complex.
Further work is hindered by inability to debug the Dolphin products. I'm in contact with them regarding alternate solutions, as well as how to proceed here.
Whiteboard: [softblocker]
Whiteboard: [softblocker] → [softblocker][possible wontfix]
Further contact made; trying something.
Whiteboard: [softblocker][possible wontfix] → [softblocker][possible thirdparty fix]
Pursuing vendor fix. I'll close this when I can.
Vendor will/has fix in new product but backporting is uncertain.
Attached patch patch (obsolete) — Splinter Review
Assignee: bolterbugz → surkov.alexander
Status: NEW → ASSIGNED
Attachment #515308 - Flags: superreview?(roc)
Attachment #515308 - Flags: review?(bolterbugz)
Comment on attachment 515308 [details] [diff] [review]
patch

>+ *   Alexander Surkov <surkov.alexander@gmail.com> (origianl author)

Nit: "original"
Comment on attachment 515308 [details] [diff] [review]
patch

> void
>+nsAccessibilityService::PresShellActivated(nsIPresShell* aPresShell)
>+{
>+  nsIDocument* DOMDoc = aPresShell->GetDocument();
>+  if (DOMDoc) {
>+    nsDocAccessible* document = GetDocAccessibleFromCache(DOMDoc);
>+    if (document) {
>+      nsRootAccessible* rootDocument = document->RootAccessible();
>+      rootDocument->DocumentActivated(document);
>+    }
>+  }
>+}

The body is only relevant for windows right? Can you #ifdef it for now?
Does resizing the window work?
Comment on attachment 515308 [details] [diff] [review]
patch

This try-server build works fine with JAWS and WE.
(In reply to comment #12)
> Comment on attachment 515308 [details] [diff] [review]
> patch
> 
> > void
> >+nsAccessibilityService::PresShellActivated(nsIPresShell* aPresShell)
> >+{
> >+  nsIDocument* DOMDoc = aPresShell->GetDocument();
> >+  if (DOMDoc) {
> >+    nsDocAccessible* document = GetDocAccessibleFromCache(DOMDoc);
> >+    if (document) {
> >+      nsRootAccessible* rootDocument = document->RootAccessible();
> >+      rootDocument->DocumentActivated(document);
> >+    }
> >+  }
> >+}
> 
> The body is only relevant for windows right? Can you #ifdef it for now?

Body does something on windows only if emulation enabled. presshells don't get activated by thousands, so excess couple light methods won't harm us.
(In reply to comment #13)
> Does resizing the window work?

no, I didn't include resizing in the final patch since I didn't get confirmation they need it. Also I don't handle maximizing, minimizing which might be different from resizing.
Comment on attachment 515308 [details] [diff] [review]
patch

>+   * Notify that the sub document was activated.

What does it mean to say "document was activated"?  The pres shell?
Comment on attachment 515308 [details] [diff] [review]
patch

r+ for the accessibility changes with comments addressed. This would be a scary change to take this late. We'd want nightly coverage before RC.

(the layout change looks good to me)

> void
>+nsAccessibilityService::PresShellActivated(nsIPresShell* aPresShell)
>+{
>+  nsIDocument* DOMDoc = aPresShell->GetDocument();
>+  if (DOMDoc) {
>+    nsDocAccessible* document = GetDocAccessibleFromCache(DOMDoc);
>+    if (document) {
>+      nsRootAccessible* rootDocument = document->RootAccessible();
>+      rootDocument->DocumentActivated(document);
>+    }

We should null check rootDocument right?

>+++ b/accessible/src/base/nsRootAccessible.cpp
>+++ b/accessible/src/msaa/nsAccessNodeWrap.cpp

>+    case WM_NCHITTEST:
>+    {
>+      LRESULT lRet = ::DefWindowProc(hWnd, msg, wParam, lParam);
>+      if (HTCLIENT == lRet)
>+        lRet = HTTRANSPARENT;
>+      return lRet;
>+    }

Good trick! I forgot about this.

>+void
>+nsDocAccessibleWrap::NotifyOfInitialUpdate()

>+      nsRootAccessible* rootDocument = RootAccessible();
>+      PRInt32 rootX = 0, rootY = 0, rootWidth = 0, rootHeight = 0;
>+      rootDocument->GetBounds(&rootX, &rootY, &rootWidth, &rootHeight);

null check rootDocument first.

>+++ b/accessible/src/msaa/nsDocAccessibleWrap.h

> protected:
>+  // nsDocAccessible
>+  virtual void NotifyOfInitialUpdate();

Could add more comments.

>+++ b/accessible/src/msaa/nsRootAccessibleWrap.cpp

>+ * Contributor(s):
>+ *   Alexander Surkov <surkov.alexander@gmail.com> (origianl author)

Actually origami author would be neat :)

>+void
>+nsRootAccessibleWrap::DocumentActivated(nsDocAccessible* aDocument)
>+{

Ah ok I get this one, but comments in the header file would still be good.
Attachment #515308 - Flags: review?(bolterbugz) → review+
Let's get an f+ from David Carrington.
We have to make sure we don't break the current and future versions of Dolphin products if we take this patch.

Note, I filed bug 637326 for eventually removing our emulation hacks.
I don't understand how this works. You create fake HWNDs and place them over the main window content? Does it work because the windows are always hidden?
(In reply to comment #21)
> I don't understand how this works. You create fake HWNDs and place them over
> the main window content? Does it work because the windows are always hidden?

correct, they visible, the trick is to make them transparent and handle WM_NCHITTEST event
(In reply to comment #18)

> We should null check rootDocument right?
> null check rootDocument first.

they shouldn't be ever null, if you want to be 100% sure we don't introduce new crashes then I can add null check and assertion.
Attachment #515308 - Flags: approval2.0?
(In reply to comment #19)
> Let's get an f+ from David Carrington.

go ahead and ask, please make sure to give him details why you look for his f+.
Isn't there a massive performance penalty for doing this? For example, have you checked how scrolling performance is affected? This is really scary.
(In reply to comment #25)
> Isn't there a massive performance penalty for doing this? For example, have you
> checked how scrolling performance is affected? This is really scary.

Are there available perf testsuite for that? I didn't noticed anything by eyes.
You can test scrolling performance on most pages using this bookmarklet:

javascript:void((function(){var%20i=0;var%20start=Date.now();function%20f(){if(++i==100){alert((Date.now()-start)+"ms");return;}window.scrollTo(0,i*5);setTimeout(f,10);}f();})())
(In reply to comment #19)
> Let's get an f+ from David Carrington.

I didn't recognized his name and I though this is somebody from Mozilla :) Yes, David said this patch improves the things and they need small fix on their side to make Supernova working with Fx4 and hopefully they provide an internet update to their users if this patch is landed.
(In reply to comment #27)
> You can test scrolling performance on most pages using this bookmarklet:
> 
> javascript:void((function(){var%20i=0;var%20start=Date.now();function%20f(){if(++i==100){alert((Date.now()-start)+"ms");return;}window.scrollTo(0,i*5);setTimeout(f,10);}f();})())

Ok, thank you. I'll do testing. But on the another hand numbers don't look here so important since this code is activated iif we detected specific screen readers running and screen reader users don't use scrolling too much and hearing is much different than seeing in terms of time estimation.
After our risk/reward discussion on IRC and comment 28, I think we would strongly consider landing this. We'll need to get Robert's r+ next (before a+).
Summary: HAL/Supernova needs tweaks to our window emulation → HAL/Supernova needs more sophisticated window emulation
Whiteboard: [softblocker][possible thirdparty fix] → [softblocker][has patch][f+ Dolphin][needs superreview]
Comment on attachment 515308 [details] [diff] [review]
patch

Looks like the windows will be created if JAWS, WindowEyes or HAL are running.

The code is OK but this still really scares me. The overlaying transparent window could affect drag-and-drop, trackpad drivers, and who knows what else. Maybe even plugins.

If we only need this for HAL, can we make it not happen for JAWS/WindowEyes?
Alexander mentioned in channel that we could make this HAL only.
Comment on attachment 515308 [details] [diff] [review]
patch

Can't grant a before sr, since sr helps us discover the risk that we use to assess approval
Attachment #515308 - Flags: approval2.0?
(In reply to comment #32)
> Alexander mentioned in channel that we could make this HAL only.

Please do that, then I'll mark this sr+.
Attached patch patch2Splinter Review
with fixed David's and Robert's comments
Attachment #515308 - Attachment is obsolete: true
Attachment #516158 - Flags: superreview?(roc)
Attachment #515308 - Flags: superreview?(roc)
(In reply to comment #27)
> You can test scrolling performance on most pages using this bookmarklet:
> 
> javascript:void((function(){var%20i=0;var%20start=Date.now();function%20f(){if(++i==100){alert((Date.now()-start)+"ms");return;}window.scrollTo(0,i*5);setTimeout(f,10);}f();})())

I don't see any slowness, moreever due to some reason I get numbers higher on patched build: 3200 ms vs 3300 on debug builds.
Comment on attachment 516158 [details] [diff] [review]
patch2

Ugh.
Attachment #516158 - Flags: superreview?(roc) → superreview+
Attachment #516158 - Flags: approval2.0?
Whiteboard: [softblocker][has patch][f+ Dolphin][needs superreview] → [softblocker][has patch][f+ Dolphin][needs approval]
Comment on attachment 516158 [details] [diff] [review]
patch2

a=me. MarcoZ is building locally to test. Once he is happy he'll land this duckling.

Note: IsWindowEmulationEnabled is sort of a dual purpose method now which is a bit strange and we might want to change later, but the logic looks right.
Attachment #516158 - Flags: approval2.0? → approval2.0+
landed on 2.0 (fx4rc) - http://hg.mozilla.org/mozilla-central/rev/3a4967bd43e8
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [softblocker][has patch][f+ Dolphin][needs approval] → [softblocker][has patch][f+ Dolphin]
Target Milestone: --- → mozilla2.0
(In reply to comment #12)
> Comment on attachment 515308 [details] [diff] [review]
> patch
> 
> > void
> >+nsAccessibilityService::PresShellActivated(nsIPresShell* aPresShell)
> >+{
> >+  nsIDocument* DOMDoc = aPresShell->GetDocument();
> >+  if (DOMDoc) {
> >+    nsDocAccessible* document = GetDocAccessibleFromCache(DOMDoc);
> >+    if (document) {
> >+      nsRootAccessible* rootDocument = document->RootAccessible();
> >+      rootDocument->DocumentActivated(document);
> >+    }
> >+  }
> >+}
> 
> The body is only relevant for windows right? Can you #ifdef it for now?

This comment was not respected upon landing...
(In reply to comment #15)
> (In reply to comment #12)
> > Comment on attachment 515308 [details] [diff] [review]
> > patch
> > 
> > > void
> > >+nsAccessibilityService::PresShellActivated(nsIPresShell* aPresShell)
> > >+{
> > >+  nsIDocument* DOMDoc = aPresShell->GetDocument();
> > >+  if (DOMDoc) {
> > >+    nsDocAccessible* document = GetDocAccessibleFromCache(DOMDoc);
> > >+    if (document) {
> > >+      nsRootAccessible* rootDocument = document->RootAccessible();
> > >+      rootDocument->DocumentActivated(document);
> > >+    }
> > >+  }
> > >+}
> > 
> > The body is only relevant for windows right? Can you #ifdef it for now?
> 
> Body does something on windows only if emulation enabled. presshells don't get
> activated by thousands, so excess couple light methods won't harm us.

This is probably causing this orange:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1299102341.1299103712.26360.gz
Well, maybe bug 637644 is at fault here...
OK thanks. Note the code in this changeset is not expected to be exercised by our mochitests.
(In reply to comment #40)

> > The body is only relevant for windows right? Can you #ifdef it for now?
> 
> This comment was not respected upon landing...

It wasn't fixed but definitely respected, see comment #15.
(In reply to comment #45)
> (In reply to comment #40)
> 
> > > The body is only relevant for windows right? Can you #ifdef it for now?
> > 
> > This comment was not respected upon landing...
> 
> It wasn't fixed but definitely respected, see comment #15.

My bad, sorry.  :-)
You need to log in before you can comment on or make changes to this bug.