241993 - Use window class name corresponding to UI/content/general

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Aaron Leventhal]

All Mozilla windows are of class "MozillaWindowClass". This confuses screen
readers, because screen readers determine what action to take based on the
window class name.

This bug is to create new window classes which have different names, but the
same properties for all other purposes.

MozillaUIWindowClass -- for the top UI window that the screen reader should look
at. This corresponds to the HWND that receives focus when the XUL main window or
dialog window gain focus.
MozillaContentWindowClass - for the top content window that the screen reader
should traverse the MSAA tree of. This corresponds to the HWND that receives
focus when the XUL main window or dialog window gain focus.
MozillaHiddenWindowClass - for hidden windows
MozillaWindowClass - a general catchall window class

Comment 1

[Aaron Leventhal]

Attachment: Implements the new window classes without changing any behavior (broke many touchpads - tsk)

This is extremely helpful to screen reader vendors.

MozillaContentWindowClass - start of MSAA tree for content areas. These are
also the windows that get system focus when the user clicks in there.
MozillaUIWindowClass -- start of MSAA tree for dialogs and the main window.
These are the windows that get system focus.
MozillaWindowClass -- general utility windows that have an MSAA tree, but
aren't the start of one.

A typical dialog would look like this:

- #32770
   - MozillaUIWindowClass

A typical main browser window would look like this:
- MozillaWindowClass
   - MozillaUIWindowClass      (start of UI MSAA tree)
       - MozillaWindowClass
	   - MozillaWindowClass
		- MozillaContentWindowClass    (start of content MSAA tree)
		     - MozillaWindowClass

A framed browser window would look like:
- MozillaWindowClass
   - MozillaUIWindowClass
       - MozillaWindowClass
	   - MozillaWindowClass
		- MozillaContentWindowClass
		    - MozillaWindowClass
			 - MozillaWindowClass
			    - MozillaContentWindowClass      (first frame)     
 
				 - MozillaWindowClass
			 - MozillaWindowClass
			    - MozillaContentWindowClass      (second frame)    
  
				 - MozillaWindowClass

Comment 2

[Aaron Leventhal]

Comment on attachment 147264 [details] [diff] [review]
Implements the new window classes without changing any behavior (broke many touchpads - tsk)

Seeking r+sr = roc+moz

Comment 3

[Aaron Leventhal]

This also fixes two other bugs:
bug 241497: Focus events inside iframes returning garbage (in MSAA event viewer)
No bug filed: get_accChildCount returning 0 on ROLE_WINDOW and ROLE_CLIENT

Comment 4

[Ere Maijala (slow)]

Comment on attachment 147264 [details] [diff] [review]
Implements the new window classes without changing any behavior (broke many touchpads - tsk)

So, no 3rd party is using GWL_ID now? This would break any, right?

Comment 5

[Aaron Leventhal]

Ere, that won't break anyone. The only people using it were GW Micro, and that
was broken. I talked with them and we agreed this is a better way.

Comment 6

[Ere Maijala (slow)]

Comment on attachment 147264 [details] [diff] [review]
Implements the new window classes without changing any behavior (broke many touchpads - tsk)

Ok, good. r=ere

Comment 7

[Malcolm Rowe]

(way to go not quoting the bug number on the checkin comment, btw.)

widget/src/windows/nsWindow.cpp
@@ -4635,15 +4607,32 @@

+        wc.lpszClassName    = kWClassNameHidden;
+        nsWindow::sIsRegistered = nsToolkit::mRegisterClass(&wc);
+
+        wc.lpszClassName    = kWClassNameContent;
+        nsWindow::sIsRegistered &= nsToolkit::mRegisterClass(&wc);
+
+        wc.lpszClassName    = kWClassNameGeneral;
         nsWindow::sIsRegistered = nsToolkit::mRegisterClass(&wc);
+
+        wc.lpszClassName    = kWClassNameUI;
+        nsWindow::sIsRegistered &= nsToolkit::mRegisterClass(&wc);

That third call to RegisterClass looks bogus (=, not &=).

Plus, RegisterClass returns an ATOM, not a BOOL. You can't just bitwise-and the 
results together to check they all succeeded (what if the first two calls 
return 0x1 and 0x2?). 

result &= (RegisterClass() != 0);  would work ok. [or 'result &= !!RegisterClass
()', but I find that hard to read myself].

Reopening; this needs a follow-up patch.

Comment 8

[Aaron Leventhal]

Thanks for catching the error and sorry about forgetting the bug # in the
checkin comment. It's been about a year since I've been actively checking in
code to the tree, so that's my excuse.

I guess I sound like an old timer, but since when did people get so damn cranky
in this project. Is that the "in" thing to do now -- to sound as tough as
possible when pointing out mistakes?

Comment 9

[Aaron Leventhal]

Attachment: Followup patch cleans up mistakes in handling possible errors in registering classes

Comment 10

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

Don't use 'bool'. We use PRBool instead.

I think the easiest way to write this is just

  if (!nsToolkit::mRegisterClass(&wc))
    nsWindow::sIsRegistered = PR_FALSE;

People get cranky. It's an online thing :-).

Comment 11

[Malcolm Rowe]

Comment on attachment 147626 [details] [diff] [review]
Followup patch cleans up mistakes in handling possible errors in registering classes

I'm not an official reviewer, so I won't mark this r=, but the change looks
fine to me (possible nit: extra whitespace after 'bool succeeded ='?).

Many apologies for the attitude expressed in comment 7. I was annoyed about
something completely unrelated, but I shouldn't have let that leak into here.
Thanks for producing a follow-up patch so quickly.

Comment 12

[Malcolm Rowe]

> Don't use 'bool'. We use PRBool instead.
.. and that illustrates exactly *why* I'm not a reviewer ;) Yes, what roc said, 
plus an initialisation of nsWindow::sIsRegistered to a true value before, 
obviously. ah, actually sIsRegistered is a Win32 BOOL, not an NSPR PRBool, so 
perhaps we should use that (is it ok in platform-specific code?).

> People get cranky. It's an online thing :-).
That's no excuse, though.

Comment 13

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

Yeah, BOOL is OK in Windows code.

Comment 14

[Malcolm Rowe]

Ok, so:

        nsWindow::sIsRegistered = TRUE;

        wc.lpszClassName    = kWClassNameHidden;
        if (!nsToolkit::mRegisterClass(&wc))
          nsWindow::sIsRegistered = FALSE;

        wc.lpszClassName    = kWClassNameContent;
        if (!nsToolkit::mRegisterClass(&wc))
          nsWindow::sIsRegistered = FALSE;

        wc.lpszClassName    = kWClassNameGeneral;
        if (!nsToolkit::mRegisterClass(&wc))
          nsWindow::sIsRegistered = FALSE;

        wc.lpszClassName    = kWClassNameUI;
        if (!nsToolkit::mRegisterClass(&wc))
          nsWindow::sIsRegistered = FALSE;

Comment 15

[Aaron Leventhal]

Attachment: Get bool types right

Blech, I miss being able to use normal, simple types for things :)

Comment 16

[Aaron Leventhal]

C:\moz\mozilla\widget\src\windows>cvs ci -m "Bug 241993. Follow up patch that
cleans up error checking code for registering class. r+sr=roc" nsWindow.cpp
Checking in nsWindow.cpp;
/cvsroot/mozilla/widget/src/windows/nsWindow.cpp,v  <--  nsWindow.cpp
new revision: 3.500; previous revision: 3.499
done

Comment 17

[timeless]

I'm prepared to backed this out. you broke my touchpad.

The commands I'm going to use are:
cvs update -j3.500 -j3.499 widget/src/windows/nsWindow.cpp
cvs update -j3.728 -j3.727 layout/html/base/src/nsPresShell.cpp
cvs update -j3.252 -j3.251 layout/html/document/src/nsFrameFrame.cpp
cvs update -j1.214 -j1.213 xpfe/appshell/src/nsAppShellService.cpp
cvs update -j3.175 -j3.174 widget/src/windows/nsWindow.h
cvs update -j3.499 -j3.498 widget/src/windows/nsWindow.cpp

followed by a hand correction of nsPresShell due to viewmanager decom.

Comment 18

[Malcolm Rowe]

Does it help the touchpad issue if we make the content window class
'MozillaWindowClass' (as it was pre-patch), and change the outermost window to
'MozillaWindowFrameClass'?