410765 - 'List all tabs' button on tabs not working in AT-SPI

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Steve Lee]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b3pre) Gecko/2008010204 Minefield/3.0b3pre
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b3pre) Gecko/2008010204 Minefield/3.0b3pre

The 'list all tabs' button on the right end of the page tabs does not work through AT-SPI Action:doAction. Press action is exposed but does nothing.

Reproducible: Always

Steps to Reproduce:
1.Open a number of tabs
2.look at the Action interface of the list all tabs button (hint CTRL+ALT+?)
3. select and do the Press action
Actual Results:  
nothing happens

Expected Results:  
menu to drop down with a list of tabs

Comment 1

[Steve Lee]

Same problem occurs with the 'bookmarks' button at the end of the bookmarks toolbar. 

Comment 2

[Marco Zehe (:MarcoZ)]

(In reply to comment #1)
> Same problem occurs with the 'bookmarks' button at the end of the bookmarks
> toolbar. 

Steve, do you mean the "star" button? If so, it *may* be that this gets fixed once we get bug 412849 fixed. Aaron, do you agree?

Comment 3

[Steve Lee]

(In reply to comment #2)
> Steve, do you mean the "star" button? If so, it *may* be that this gets fixed
> once we get bug 412849 fixed. Aaron, do you agree?

No Marco, actually that is the one part of the location control that does work, though it only changes the appearance of the star the first time and presents a 'dialog' the second time.

I meant the little double chevron button that appears at the far right of the bookmark toolbar when there are more booksmark buttons than will fit across. It's named 'bookmarks' and is the last child in accerciser. The bookmark toolbar appears between the navigation toolbar and the page tabs. 

Both these overflow menus (bookmarks and tabs) should work via a11y  

Comment 4

[alexander :surkov (:asurkov)]

we have accessible hierarchy:
pushbutton (xul:toolbarbutton)
  pushbutton (xul:dropmarket)

in gecko action is executed only on pushbutton for dropmarker. Is this hierarchy correct and if it is then should we perform the same action on pushbutton for toolbarbutton?

Comment 5

[Steve Lee]

I don't know enough XUL to comment on the internal hierarchy but all that is exposed is a ROLE_PUSH_BUTTON and I think that is correct. As long as the popup window has the correct hierarchy i don't think the menu needs to be part of the main structure as they are more dynamic than the main menus. It is the same as the non-image items in the location and search widgets. AFAIKS there is no standard semantics (ROLE or RELATION) for saying this button presents a popup, but I've pinged the at-spi list.

Comment 6

[alexander :surkov (:asurkov)]

Ok, I got you.

The problem is we have in Gecko hierarchy:
pushbutton (xul:toolbarbutton)
  pushbutton (xul:dropmarket)

but we expose on ATK the following hierarhy:
pushbutton (xul:toolbarbutton)
because of MustPrune()

action on dropmarker works but it doesn't on toolbarbutton.

I'm not sure about right way to fix this.

Comment 7

[Steve Lee]

(In reply to comment #5)
>AFAIKS there is no
> standard semantics (ROLE or RELATION) for saying this button presents a popup,
> but I've pinged the at-spi list.

Bill Haneman has indicated that 'I believe RELATION_PARENT_WINDOW_OF is/was intended to be the reciprocal of RELATION_POPUP_FOR'.

However that can only be set once the popup exists, probably after the button is activated.

Comment 8

[alexander :surkov (:asurkov)]

Attachment: doesn't work

idea is to fire real events instead of pure DOM events like Click() method do. But it doesn't work. Dunno why. Button isnt' pressed, menu isn't appeared.

Comment 9

[alexander :surkov (:asurkov)]

Attachment: wip

it seems this sends correct events but popup isn't shown, possibly it's mac issue.

Comment 10

[alexander :surkov (:asurkov)]

Marco, could you test on Windows/Linux. Does the patch doesn't work there as well?

Comment 11

[alexander :surkov (:asurkov)]

Any idea why real mouse clicks (see the patch) doesn't show menupopup on button@type="menu" like
<button type="menu">
  <menupopup>
    <menuitem label="item1"/>
    <menuitem label="item1"/>
  </menupopup>
</button>

Though I do not see the button is pressed as well when I send button up/down events.

Comment 12

[Marco Zehe (:MarcoZ)]

Surkov, with this patch, it works fine on Windows. If I do the "press" action from within AccExplorer, the popup comes up, but doesn't in a regular build. I am currently having problems using Accerciser on Linux, so can't test it there. But the problem is definitely fixed on Windows, and possibly on Linux, too.
Steve, do you build Firefox on your own and could test with this patch on Linux?

Comment 13

[alexander :surkov (:asurkov)]

Could you look does the patch fixes bug 404576 as well?

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

You many need to look at mousethrough attributes to find the right target.
Not sure if displaylist has some helper methods for this.
Or maybe nsDocument::elementfrompoint, though that one doesn't return anonymous
elements.
And btw, GetPrimaryFrame may return nsnull.

Comment 15

[alexander :surkov (:asurkov)]

(In reply to comment #14)
> You many need to look at mousethrough attributes to find the right target.
> Not sure if displaylist has some helper methods for this.
> Or maybe nsDocument::elementfrompoint, though that one doesn't return anonymous
> elements.

How I can use mousethrough attribute? I mean if I send click event to such element then element won't get notification as XUL author wanted. And it sounds ok. No?

> And btw, GetPrimaryFrame may return nsnull.
> 

thank you, I'll fix.

Comment 16

[alexander :surkov (:asurkov)]

Attachment: patch

if it fixes bug on Windows then I should do all correct. If the patch doesn't work on another systems then we should file another bugs, possibly not under accessibility component.

Comment 17

[Aaron Leventhal]

Interesting. Why do we have to do it this way where we set the x, y?

What happens if the element is currently scrolled off? What if it so far scrolled off that it's way off screen?

Comment 18

[Neil Deakin]

Why don't we just do the same widget-retrieving code as nsDOMWindowUtils?

Comment 19

[alexander :surkov (:asurkov)]

(In reply to comment #18)
> Why don't we just do the same widget-retrieving code as nsDOMWindowUtils?
> 

Smaug suggesed to use nsPresShell firstly. I don't know but possibly it's faster to compute point coordinates relative parent frame instead to get client coordinates.

Comment 20

[alexander :surkov (:asurkov)]

(In reply to comment #17)
> Interesting. Why do we have to do it this way where we set the x, y?

Sorry what way do you keep in your mind?

> What happens if the element is currently scrolled off? What if it so far
> scrolled off that it's way off screen?
> 

Actually I didn't test it. But possibly we should to scroll content into visible area, otherwise some actions don't make sense if for example popup will apear out of screen. Do you agree?

Comment 21

[Aaron Leventhal]

> Actually I didn't test it. But possibly we should to scroll content into
> visible area, otherwise some actions don't make sense if for example popup will
> apear out of screen. Do you agree?
Very good idea, yes I agree.

Comment 22

[alexander :surkov (:asurkov)]

Attachment: patch2

Comment 23

[Aaron Leventhal]

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

What if the element is in something that's not scrollable and it's still offscreen? Should we fail in that case?

Comment 24

[alexander :surkov (:asurkov)]

(In reply to comment #23)
> (From update of attachment 301276 [details] [diff] [review])
> What if the element is in something that's not scrollable and it's still
> offscreen? Should we fail in that case?
> 

With the patch it's clicked, at least oncommand event is fired. Possibly it's ok?

Comment 25

[Aaron Leventhal]

Surkov, can you test different kinds of ofscreen scenarios and see what happens?

Comment 26

[alexander :surkov (:asurkov)]

The patch doesn't work on linux and mac

Comment 27

[alexander :surkov (:asurkov)]

(In reply to comment #25)
> Surkov, can you test different kinds of ofscreen scenarios and see what
> happens?
> 

What kind of ofscreen scenarious keep in mind? I tried scrolled out button. It is made visible and clicked. If  the button can't be scrolled into visible area then it's clicked any way because I get its command event. 

Comment 28

[Aaron Leventhal]

(In reply to comment #26)
> The patch doesn't work on linux and mac

So, I guess we need a new patch then?

(In reply to comment #27)
> What kind of ofscreen scenarious keep in mind? I tried scrolled out button. It
> is made visible and clicked. If  the button can't be scrolled into visible area
> then it's clicked any way because I get its command event. 

Okay.

Comment 29

[Aaron Leventhal]

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

Cancelling review until we have a patch that works on Linux and Mac as well.

Comment 30

[alexander :surkov (:asurkov)]

(In reply to comment #29)
> (From update of attachment 301276 [details] [diff] [review])
> Cancelling review until we have a patch that works on Linux and Mac as well.
> 

If it's right approach then the patch should work on linux and mac and if it doesn't then the problem lies in something else. I think let's get some reviews to get know the approach is correct/wrong.

Comment 31

[alexander :surkov (:asurkov)]

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

Could you check if the taken approach is correct to send click events to element that is much identical with real users clicks?

Comment 32

[Olli Pettay [:smaug][[e-mail]]]

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

>+  nsEventStatus status = nsEventStatus_eIgnore;
>+  presShell->HandleEventWithTarget(&eventDown, frame, content, &status);
Event handling may delete |frame|, but you still use |frame| later.

The existing code has the bug that |content| isn't a strong pointer, so
reusing it after first event dispatch is dangerous.

Comment 33

[Aaron Leventhal]

Surkov, did you already test on Linux and Mac? Comment 26 made me think that you tested and it doesn't work.

Comment 34

[alexander :surkov (:asurkov)]

Attachment: patch3

with Olli's comments

Comment 35

[alexander :surkov (:asurkov)]

(In reply to comment #33)
> Surkov, did you already test on Linux and Mac? Comment 26 made me think that
> you tested and it doesn't work.
> 

Yes. Though I'm sure about Mac only.

Comment 36

[Aaron Leventhal]

Why wouldn't we fix xulElement->Click() so it works correctly? Won't others run into the same problem when they try to use it?

Comment 37

[alexander :surkov (:asurkov)]

Attachment: patch4

works on Windows and OS X (didn't try on Linux yet)

Comment 38

[Marco Zehe (:MarcoZ)]

Comment on attachment 331302 [details] [diff] [review]
patch4

Nit: "mouseup haven't been fired". It's either "mouseups haven't", or "mouseup hasn't". Same for all other cases where this comes up. "haven't" implies plural. if you want to use the singular, it's always "hasn't".

Comment 39

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

+  // Fire mouse button down event.
+  nsCOMPtr<nsIWidget> parentWidget = parentFrame->GetWindow();
+
+  nsMouseEvent eventDown(PR_TRUE, NS_MOUSE_BUTTON_DOWN, parentWidget,
+                         nsMouseEvent::eReal, nsMouseEvent::eNormal);
+
+  eventDown.refPoint.x = x;
+  eventDown.refPoint.y = y;
+

This isn't right. The coordinates should be relative to the widget but you have only made them relative to the view.

I would do what nsDOMWindowUtils::SendMouseEvent does: use GetOffsetToExternal to make the coordinates relative to the presshell's root frame, and use the root frame's widget.

Comment 40

[alexander :surkov (:asurkov)]

Attachment: patch5

Comment 41

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

So if the down-click causes the frame to move, your coordinates might be wrong. You probably should separate the mouse-event-dispatch code into a helper function that you call twice. The helper function can compute the coordinates each time.

Comment 42

[alexander :surkov (:asurkov)]

Attachment: patch6

Comment 43

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 331633 [details] [diff] [review]
patch6

>+  // Fire mouse down and mouse up events.
>+  nsIFrame* rootFrame = presShell->GetRootFrame();
>+  if (!rootFrame)
>+    return;
>+  
>+  nsCOMPtr<nsIWidget> rootWidget = rootFrame->GetWindow();
>+  if (!rootWidget)
>+    return;
>+
>+  PRBool res = nsAccUtils::DispatchMouseEvent(NS_MOUSE_BUTTON_DOWN, presShell,
>+                                              content, rootFrame, rootWidget);
>+  if (!res)
>+    return;
>+
>+  nsAccUtils::DispatchMouseEvent(NS_MOUSE_BUTTON_UP, presShell,
>+                                 content, rootFrame, rootWidget);
You're reusing rootFrame, which may be a dead object after the first DispatchMouseEvent.
Just make DispatchMouseEvent to re-fetch rootFrame and widget in every call.

What happens if page is scrolled when mousedown is dispatched?

DOM Events code use PR_Now, not PR_IntervalNow (though see bug 238041)

Comment 44

[alexander :surkov (:asurkov)]

(In reply to comment #43).
> 
> What happens if page is scrolled when mousedown is dispatched?
> 

Im not sure but I woudn't care a lot actually. Marco said he don't know or didn't notice similar situation in real cases. I think we will send mousedown/mouseup in any way for the same control and since the method I fix should activate it (send click) then it's that we expect.

> DOM Events code use PR_Now, not PR_IntervalNow (though see bug 238041)
> 

I didn't catch unique opinion from that bug. And since it's not fixed yet then I'm not sure I about right way the meantime. nsDOMWindowUtils::SendMouseEvent uses PR_IntervalNow still.

Comment 45

[alexander :surkov (:asurkov)]

Attachment: patch7

(In reply to comment #43)
> You're reusing rootFrame, which may be a dead object after the first
> DispatchMouseEvent.
> Just make DispatchMouseEvent to re-fetch rootFrame and widget in every call.

Comment 46

[alexander :surkov (:asurkov)]

(In reply to comment #45)
> Created an attachment (id=331776) [details]
> patch7
> 
> (In reply to comment #43)
> > You're reusing rootFrame, which may be a dead object after the first
> > DispatchMouseEvent.
> > Just make DispatchMouseEvent to re-fetch rootFrame and widget in every call.
> 

this is addressed in the last patch

Comment 47

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

Comment on attachment 331776 [details] [diff] [review]
patch7

+  nsRect rect = frame->GetRect();

Make this nsSize size = frame->GetSize().

+  PRInt32 x = presContext->AppUnitsToDevPixels(point.x) +
+    presContext->AppUnitsToDevPixels(rect.width) / 2;
+  PRInt32 y = presContext->AppUnitsToDevPixels(point.y) +
+    presContext->AppUnitsToDevPixels(rect.height) / 2;

Just make two calls to AppUnitsToDevPixels: ...(point.x + rect.width/2)

+  event.refPoint.x = x;
+  event.refPoint.y = y;

event.refPoint = nsIntPoint(x, y);

sr+ with that.

Comment 48

[alexander :surkov (:asurkov)]

Attachment: patch8

Comment 49

[alexander :surkov (:asurkov)]

http://hg.mozilla.org/mozilla-central/index.cgi/rev/ff33b02a1b68

Comment 50

[alexander :surkov (:asurkov)]

Comment on attachment 331851 [details] [diff] [review]
patch8

that's major bug for screen readers, requesting to put the patch into Firefox3.0

Comment 51

[Samuel Sidler (old account; do not CC)]

Comment on attachment 331851 [details] [diff] [review]
patch8

Approved for 1.9.0.2. Please land in CVS. a=ss

Comment 52

[alexander :surkov (:asurkov)]

checked in to 1.9.0

Checking in accessible/src/base/nsAccessibilityUtils.cpp;
/cvsroot/mozilla/accessible/src/base/nsAccessibilityUtils.cpp,v  <--  nsAccessibilityUtils.cpp
new revision: 1.33; previous revision: 1.32
done
Checking in accessible/src/base/nsAccessibilityUtils.h;
/cvsroot/mozilla/accessible/src/base/nsAccessibilityUtils.h,v  <--  nsAccessibilityUtils.h
new revision: 1.26; previous revision: 1.25
done
Checking in accessible/src/base/nsAccessible.cpp;
/cvsroot/mozilla/accessible/src/base/nsAccessible.cpp,v  <--  nsAccessible.cpp
new revision: 1.378; previous revision: 1.377
done
Checking in accessible/tests/mochitest/Makefile.in;
/cvsroot/mozilla/accessible/tests/mochitest/Makefile.in,v  <--  Makefile.in
new revision: 1.18; previous revision: 1.17
done
RCS file: /cvsroot/mozilla/accessible/tests/mochitest/test_nsIAccessible_actions.xul,v
done
Checking in accessible/tests/mochitest/test_nsIAccessible_actions.xul;
/cvsroot/mozilla/accessible/tests/mochitest/test_nsIAccessible_actions.xul,v  <--  test_nsIAccessible_actions.xul
initial revision: 1.1
done