612872 - HAL/Supernova needs more sophisticated window emulation

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[David Bolter [:davidb] (NeedInfo me for attention)]

"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."

Comment 1

[David Bolter [:davidb] (NeedInfo me for attention)]

Waiting for a product key from Dolphin to make debugging easier.

Comment 2

[David Bolter [:davidb] (NeedInfo me for attention)]

Got the key, tried making the native windows 10x10 and SuperNova started working again so we are looking good here. Patch to come later.

Comment 3

[David Bolter [:davidb] (NeedInfo me for attention)]

Ignore comment #2, I launched FF3.6 by mistake for that test. The problem is more complex.

Comment 4

[David Bolter [:davidb] (NeedInfo me for attention)]

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.

Comment 5

[David Bolter [:davidb] (NeedInfo me for attention)]

Further contact made; trying something.

Comment 6

[David Bolter [:davidb] (NeedInfo me for attention)]

Pursuing vendor fix. I'll close this when I can.

Comment 7

[David Bolter [:davidb] (NeedInfo me for attention)]

Vendor will/has fix in new product but backporting is uncertain.

Comment 8

[alexander :surkov (:asurkov)]

try server build - http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-6639d0798381

Comment 9

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 10

[alexander :surkov (:asurkov)]

try server build - http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-dbd3fc6e6d95

Comment 11

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 515308 [details] [diff] [review]
patch

>+ *   Alexander Surkov <[e-mail]> (origianl author)

Nit: "original"

Comment 12

[David Bolter [:davidb] (NeedInfo me for attention)]

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?

Comment 13

[David Bolter [:davidb] (NeedInfo me for attention)]

Does resizing the window work?

Comment 14

[Marco Zehe (:MarcoZ)]

Comment on attachment 515308 [details] [diff] [review]
patch

This try-server build works fine with JAWS and WE.

Comment 15

[alexander :surkov (:asurkov)]

(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.

Comment 16

[alexander :surkov (:asurkov)]

(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 17

[David Bolter [:davidb] (NeedInfo me for attention)]

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 18

[David Bolter [:davidb] (NeedInfo me for attention)]

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 <[e-mail]> (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.

Comment 19

[David Bolter [:davidb] (NeedInfo me for attention)]

Let's get an f+ from David Carrington.

Comment 20

[David Bolter [:davidb] (NeedInfo me for attention)]

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.

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 22

[alexander :surkov (:asurkov)]

(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

Comment 23

[alexander :surkov (:asurkov)]

(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.

Comment 24

[alexander :surkov (:asurkov)]

(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+.

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Isn't there a massive performance penalty for doing this? For example, have you checked how scrolling performance is affected? This is really scary.

Comment 26

[alexander :surkov (:asurkov)]

(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.

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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();})())

Comment 28

[alexander :surkov (:asurkov)]

(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.

Comment 29

[alexander :surkov (:asurkov)]

(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.

Comment 30

[David Bolter [:davidb] (NeedInfo me for attention)]

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+).

Comment 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 32

[David Bolter [:davidb] (NeedInfo me for attention)]

Alexander mentioned in channel that we could make this HAL only.

Comment 33

[Johnathan Nightingale [:johnath]]

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

Comment 34

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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+.

Comment 35

[alexander :surkov (:asurkov)]

Attachment: patch2

with fixed David's and Robert's comments

Comment 36

[alexander :surkov (:asurkov)]

(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 37

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 516158 [details] [diff] [review]
patch2

Ugh.

Comment 38

[David Bolter [:davidb] (NeedInfo me for attention)]

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.

Comment 39

[alexander :surkov (:asurkov)]

landed on 2.0 (fx4rc) - http://hg.mozilla.org/mozilla-central/rev/3a4967bd43e8

Comment 40

[(no longer active)]

(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...

Comment 41

[(no longer active)]

(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

Comment 42

[(no longer active)]

And now:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1299104381.1299105585.2223.gz

Comment 43

[(no longer active)]

Well, maybe bug 637644 is at fault here...

Comment 44

[David Bolter [:davidb] (NeedInfo me for attention)]

OK thanks. Note the code in this changeset is not expected to be exercised by our mochitests.

Comment 45

[alexander :surkov (:asurkov)]

(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.

Comment 46

[(no longer active)]

(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.  :-)