Closed Bug 343904 Opened 18 years ago Closed 17 years ago

Bounds, GetChildAtPoint() and visible extState broken for accessibles of out of flow objects

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: aaronlev, Assigned: surkov)

References

(Blocks 2 open bugs)

Details

(Keywords: access)

Attachments

(6 files, 4 obsolete files)

If an accessible is created for an object which is absolutely positioned or floating, GetChildAtPoint() will not walk into it.
To do: fix combo boxes, and do we need GetBoundsFrame()
Attachment #228462 - Attachment is obsolete: true
Sorry, Ginn, another patch for you.
Attachment #228466 - Attachment is obsolete: true
Attachment #228709 - Flags: review?(ginn.chen)
*** Bug 344146 has been marked as a duplicate of this bug. ***
In the worst case this will new algorithm will walk the entire accessible tree to find the object at the given point. However, at least it returns correct results now and the code is much more sensible. I plan to file a follow-up bug to optimize this once roc refactors layout on trunk. He's planning to remove views and merge them with frames. Once he does that we shuld also change layout to expose the FrameForPoint() method. Given the resulting frame we would go up the ancestor chain until we find something with an accessible object.
Attachment #228709 - Attachment is obsolete: true
Attachment #228809 - Flags: review?(ginn.chen)
Attachment #228709 - Flags: review?(ginn.chen)
Comment on attachment 228809 [details] [diff] [review] Move text flow logic into nsHTMLTextAccessible where it belongs Request sr= from roc now since he's on a sr= marathon!
Attachment #228809 - Flags: superreview?(roc)
GetScreenRectExternal is horrifically slow on X11 systems, because it requires an X server round trip. Calling it for every frame you traverse is going to be excruciating. Here's what I think we should do. nsLayoutUtils::GetFrameForPoint exists and does nearly what you want. The only problem is that if you pass your accessible's associated frame(s?) as the starting point, it might not include out-of-flow children in the search. What I think we should do is add an additional parameter to GetFrameForPoint (and nsDisplayList::HitTest), a predicate which is tested on each frame, i.e. a function with signature "PRBool predicate(nsIFrame*, void*)", where the void* is the usual extra parameter for passing closure data. We will return the topmost frame in z-order that satisifies the predicate, and we promise to pass each frame to that function in order from top to bottom, so it can even store the complete list of frames by always returning PR_FALSE. We can provide a public entry point to this API through nsInspectorUtils. My description is wordy but actually it should be a very small amount of work for you :-). Your call to GetFrameForPoint would simply pass in a predicate that tests if the frame's content is equal to or a descendant of the accessible's content.
Roc, I don't see any nsInspectorUtils -- is this something that's been removed?
I was thinking of nsIInspectorCSSUtils but that isn't really right. Probably you should just add this new method to nsIPresShell.
Comment on attachment 228809 [details] [diff] [review] Move text flow logic into nsHTMLTextAccessible where it belongs In +PRBool nsAccessible::IsOffscreen() [..] - return PR_FALSE; + return NS_ERROR_FAILURE; we should not return NS_ERROR_FAILURE for PRBool, right? +NS_IMETHODIMP nsHTMLTextAccessible::GetBounds(PRInt32 *x, PRInt32 *y, PRInt32 *width, PRInt32 *height) +{ + // This routine will get the entire rectange for all the frames in this node + // ------------------------------------------------------------------------- + // Primary Frame for node + // Another frame, same node <- Example + // Another frame, same node I can't understand the example in comments.
Attachment #228809 - Flags: superreview?(roc)
Attachment #228809 - Flags: review?(ginn.chen)
Attachment #228809 - Flags: review-
As a side note, I just had some problems with GetChildAtPoint() crashing: it first used GetFirstChild(), which tried to dereference and return mFirstChild. In some cases mFirstChild was null. I'll see if the behavior changes after the patch.
On trunk bounds are messed up by multiple tabs being open. Another issue: bug 305513 -- CSS columns.
I plan to fix this and bug 350841 at the same time.
Blocks: 350841
Attached patch On the right track? (obsolete) (deleted) — Splinter Review
Here's a patch, attempting to implement what roc suggested in comment 10. Layout changes: * nsIPresShell::GetFrameForPoint(frame, point, predicate, closure) is implemented. * The real work is done in nsDisplayItem::HitTest which will also get the latter two params, optionally. For every frame it finds, it tests it against the given predicate, and will at the end return the last found frame that satisfies the requirement. Accessibility: * getChildAtPoint(x, y, deep) should later change name, this is just a test. * We call the new layout method with the requirement that the found frame's content be a descendant of the original accessible's content. * One problem seems to be that not all accessibles have an associated nsIContent? Is this on the right track? Comments appreciated! Mainly I need it for hit-testing, and so I may have missed any changes needed to solve the out-of-flow problem.
That's vaguely the right idea, but it looks like it could call the predicate on the same frame twice, which you don't want. In HitTest you will need to test if the item is a leaf item and only apply the predicate if it is. Also, use something like FramePredicate as the type name instead of "predicate".
Attached patch WIP v2 (deleted) — Splinter Review
Aaron, here's something you can bootstrap off of if you want to. This patch is pretty clean, and I'm not sure why it doesn't work. I guess I don't understand gecko's frame hierarchy.
Attachment #236381 - Attachment is obsolete: true
Comment on attachment 237843 [details] [diff] [review] WIP v2 Btw, Roc: I'd be interested in hearing whether the layout/ chunk makes any sense to you. Note that I haven't addressed your style nit (predicate -> FramePredicate) yet. Also, can you elaborate on why you think HitTest() could be fired twice for the same display item? I can't see that case.
I think what happens is that you have list A that contains list B which contains item C. nsDisplayList::HitTest(A) calls nsDisplayList::HitTest(B) will call the predicate on item C and return C. Then nsDisplayList::HitTest(A) will call the predicate again on item C and return it.
Blocks: 352329
The accessibility code doesn't want to be affected by mousethrough. It should probably be in its own predicate that's passed in when it's important.
(In reply to comment #22) > The accessibility code doesn't want to be affected by mousethrough. It should > probably be in its own predicate that's passed in when it's important. Are you sure we won't get some weird frame hidden beneath the one being what we perceive as "under the mouse" then?
Perhaps someone can give me a better explanation of what mousethrough is used for?
It's used in XUL to create elements that are transparent to mouse events.
When would one want their element to be transparent to mouse events? I'm trying to understand what it's really for.
I don't really recall. Use LXR to find out?
Explanation of the 'mousethrough' attribute of all XUL elements, courtesy of XULPlanet.com: "Determines whether mouse events are passed through each element of the element until one responds to it. If this attribute is not specified, the value is inherited from the parent of the element. * always: Mouse events are passed to each element in the element starting from the top until one responds to it by returning true from its event handler. * never: Mouse events are only passed to the top element in the element
Better explanation of what mousethrough is really for here: http://groups.google.com/group/mozilla.dev.tech.xul/browse_thread/thread/4ced9c2132d4fca7/d9b249f680445325?lnk=st&q=mousethrough&rnum=1#d9b249f680445325 Based on that explanation it seems that it's okay if a11y doesn't get mousethrough frames, since
Blocks: newatk
No longer blocks: keya11y
*** Bug 357837 has been marked as a duplicate of this bug. ***
Summary: GetChildAtPoint() broken for accessibles of out of flow objects → Bounds, GetChildAtPoint() and visible extState broken for accessibles of out of flow objects
Depends on: 363426
Blocks: 363426
No longer depends on: 363426
Note that out of flow objects should get STATE_FLOATING if it's not expensive to caclulate. This means children are owned but not contained by the parent.
Blocks: htmla11y
No longer blocks: newatk, 350841, 352329, 363426
Blocks: orca
Is this the underlying problem behind bug 372624 and/or bug 369855?
No, it's unrelated. The mozilla/accessible code does not affect anything in layout, which is where the caret code lives.
Assignee: aaronleventhal → surkov.alexander
Attached patch getChildAtPoint patch (deleted) — Splinter Review
I can't get now why it returns not topmost frame.
Attached file testcase (deleted) —
(In reply to comment #35) > Created an attachment (id=273974) [details] > testcase > If I click on table cell, I pass it to GetFrameForPoint() then GetFrameForPoint() returns frame for text node. But I want to get frame for html:pre which is between table cell and text node. Robert, any idea what's wrong?
Bounds looks ok for absolutely positioned nodes. GetChildAtPoint() doesn't work because we have incorrect accessible tree for those nodes. For example: DOM tree <body> <a><img style="position: absolute"></a> </body> Accessible tree should be like document textcontainer (html:img) link (html:a) but we have document link (html:a) textcontainer (html:img) because absolutely positioned nodes are out of normal flow and we should follow hierarchy proposed by CSS2 I guess.
I don't think we should change our hierarchy, or the bounds even. Apparently a child can have bounds outside of the parent. If it is floating or absolutely positioned it should get state FLOATING. I think the following algorithm can fix GetChildAtPoint(): 1. Ask layout tell us what frame has the point 2. Get the content node from the frame 3. Use GetRelevantContentNodeFor() to get the content node that could have a relevant accessible 4. Use GetAccessibleInParentChain() to get the first accessible up from there 4. if accessible == this then the point falls in this accessible but not in a child of it 5. Start loop: a) get parent accessible b) if parent == this, then previous parent accessible c) if parent == root, then return null d) go back to (a)
Attached patch patch (deleted) — Splinter Review
1) state patch 2) bounds looks ok 3) bug 388927 address getChildAtPoint()
Attachment #279698 - Flags: review?(aaronleventhal)
Attachment #279698 - Flags: review?(aaronleventhal)
Attachment #279698 - Flags: review+
Attachment #279698 - Flags: approval1.9?
Attachment #279698 - Flags: approval1.9? → approval1.9+
Checked in for Surkov.
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: