388927 - GetChildAtPoint() fails for scrolled content

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Attachment: testcase

make window size to see the half of table only then scroll to second column and click it. GetChildAtPoint() reutrn document accessible, but it should retunr table accessible.

The problem is GetBounds() return negative values.

Comment 1

[alexander :surkov (:asurkov)]

btw, it works fine on fx2

Comment 2

[alexander :surkov (:asurkov)]

Robert, if the frame is scrolled (is bigger than browser's window) then what will nsIFrame::getScreenRectExternal() return? For example,

       frame size
-----------------------|
|     |                |
|     |  window area   |
|----------------------|

Comment 3

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

It will always return the full size. GetScreenRectExternal doesn't do any clipping.

Comment 4

[alexander :surkov (:asurkov)]

(In reply to comment #3)
> It will always return the full size. GetScreenRectExternal doesn't do any
> clipping.
> 

Ok, I get it for width/height. But what about x/y if left part of the frame is invisible (scrolled out). I think left-top edge of window area (visible area) should be (0, 0) so what is x/y?

Comment 5

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

If the frame is scrolled up or left beyond the top or left edge of the window, its screen rect x or y can be negative; again, we're not clipping it to the window edges.

Comment 6

[alexander :surkov (:asurkov)]

Ok, how can I get rect of visible area only in coords relative screen?

Comment 7

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

I'm not exactly sure what GetChildAtPoint is supposed to do, but it should probably be using nsLayoutUtils::GetFrameForPoint in some way.

Comment 8

[alexander :surkov (:asurkov)]

Attachment: patch

I follow to the next logic: find accessible inside direct children, otherwise return null. It fixed this bug. But unfortunately it doesn't fix bug 343904. I took idea of the patch from there. I noticed the xul trees should have own implementation of the method. I'm not sure do we have another accessibles like trees.

Comment 9

[Aaron Leventhal]

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

Evan, can you be the reviewer?

Comment 10

[alexander :surkov (:asurkov)]

Attachment: patch2

fix for tree columns

Comment 11

[Evan Yan]

er.. when I was changing the product field of this bug from "firefox" to "core", the flag "blocking-firefox3" was cleared automatically. I didn't find where I can reset it.

Aaron, please reset it if needed. Sorry about that.

Comment 12

[Evan Yan]

sorry, my latest comment was meant for bug 393094... I was a bonehead.

Comment 13

[Evan Yan]

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

>+
>+  return GetCachedTreeitemAccessible(row, column, aAccessible);
>+}
>+
Have you tested whether it works when tree view changed, like sorting by another column, moving columns? 
Currently we didn't update cache when tree view changed, refer to bug 368835 comment #28.

Comment 14

[alexander :surkov (:asurkov)]

(In reply to comment #13)
> (From update of attachment 278121 [details] [diff] [review])
> >+
> >+  return GetCachedTreeitemAccessible(row, column, aAccessible);
> >+}
> >+
> Have you tested whether it works when tree view changed, like sorting by
> another column, moving columns? 
> Currently we didn't update cache when tree view changed, refer to bug 368835
> comment #28.
> 

No, I didn't tested. But I think it shouldn't work like it doens't work now because now we getChildAtPoint uses bounds of accessible and since we do not update cache then we'll return bounds for obsolete accessibles.

Comment 15

[Evan Yan]

I guess that depend on the |row| returned from mTree->GetCellAt(), is the row of _model_ or the row of _view_

Comment 16

[alexander :surkov (:asurkov)]

(In reply to comment #15)
> I guess that depend on the |row| returned from mTree->GetCellAt(), is the row
> of _model_ or the row of _view_
> 

Evan, sorry, I'm not sure I got you. Can you rephrase it?

Comment 17

[alexander :surkov (:asurkov)]

Attachment: tree testcase (requires chrome privs)

Evan, sort and columndrag seems to work. I failed because treeitem accessibles are wrappers for methods of nsITreeView therefore our cache is always valid (though probably is bigger than treeitems count).

Comment 18

[Evan Yan]

(In reply to comment #16)
> Evan, sorry, I'm not sure I got you. Can you rephrase it?
> 
Well, I was talking about MVC pattern, which was followed by xul tree implementation, I think.
I talked about two kind of row index. One is the row of view, i.e. which line it shows on screen; The other is the row of model, i.e. the index it stored in its data structure. When the tree view changed, the row of view changed, but the row of model keeps the same.

So, in your patch, if mTree->GetCellAt() returned the row of view, then we use it to call GetCachedTreeitemAccessible(), we probably can get the right accessible.

Comment 19

[Evan Yan]

(In reply to comment #17)
> Created an attachment (id=278522) [details]
> tree testcase (requires chrome privs)
> 
> Evan, sort and columndrag seems to work. 
Great!

> I failed because treeitem accessibles
> are wrappers for methods of nsITreeView therefore our cache is always valid
> (though probably is bigger than treeitems count).
> 
Yea, that's the point of my confusing comment ;) Sorry about my bad expression.

Comment 20

[alexander :surkov (:asurkov)]

Attachment: patch3

logic to keep out of flow elements from bug 343904

Comment 21

[Aaron Leventhal]

Does this fix the out of flow accessibles as well?

Comment 22

[Evan Yan]

Comment on attachment 278952 [details] [diff] [review]
patch3

looks good.

Comment 24

[alexander :surkov (:asurkov)]

(In reply to comment #21)
> Does this fix the out of flow accessibles as well?
> 

Yes, I tested for absolute position.

Comment 25

[Aaron Leventhal]

> Yes, I tested for absolute position.

Do you mean it fixes bug 343904?

Comment 26

[alexander :surkov (:asurkov)]

(In reply to comment #25)
> > Yes, I tested for absolute position.
> 
> Do you mean it fixes bug 343904?
> 

Yep but I should set STATE_FLOATING additionally I guess. Are there another problems with states for out of flow objects?

Comment 27

[Aaron Leventhal]

No other problems with states.

Comment 28

[alexander :surkov (:asurkov)]

checked in

Comment 29

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

Just wondering, could this fix be causing bug 471493?  (mutually recursive functions causing a stack overflow)

Comment 30

[alexander :surkov (:asurkov)]

(In reply to comment #29)
> Just wondering, could this fix be causing bug 471493?  (mutually recursive
> functions causing a stack overflow)

No, I think it's bug 455442.