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)
Core
Disability Access APIs
Tracking
()
RESOLVED
FIXED
People
(Reporter: aaronlev, Assigned: surkov)
References
(Blocks 2 open bugs)
Details
(Keywords: access)
Attachments
(6 files, 4 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
ginnchen+exoracle
:
review-
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
application/xhtml+xml
|
Details | |
(deleted),
patch
|
aaronlev
:
review+
damons
:
approval1.9+
|
Details | Diff | Splinter Review |
If an accessible is created for an object which is absolutely positioned or floating, GetChildAtPoint() will not walk into it.
Reporter | ||
Comment 1•18 years ago
|
||
Reporter | ||
Comment 2•18 years ago
|
||
Reporter | ||
Comment 3•18 years ago
|
||
To do: fix combo boxes, and do we need GetBoundsFrame()
Reporter | ||
Comment 4•18 years ago
|
||
Attachment #228462 -
Attachment is obsolete: true
Reporter | ||
Comment 5•18 years ago
|
||
Sorry, Ginn, another patch for you.
Attachment #228466 -
Attachment is obsolete: true
Attachment #228709 -
Flags: review?(ginn.chen)
Reporter | ||
Comment 6•18 years ago
|
||
*** Bug 344146 has been marked as a duplicate of this bug. ***
Reporter | ||
Comment 7•18 years ago
|
||
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.
Reporter | ||
Comment 8•18 years ago
|
||
Attachment #228709 -
Attachment is obsolete: true
Attachment #228809 -
Flags: review?(ginn.chen)
Attachment #228709 -
Flags: review?(ginn.chen)
Reporter | ||
Comment 9•18 years ago
|
||
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.
Reporter | ||
Comment 11•18 years ago
|
||
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 13•18 years ago
|
||
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-
Comment 14•18 years ago
|
||
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.
Reporter | ||
Comment 15•18 years ago
|
||
On trunk bounds are messed up by multiple tabs being open.
Another issue: bug 305513 -- CSS columns.
Reporter | ||
Comment 16•18 years ago
|
||
I plan to fix this and bug 350841 at the same time.
Comment 17•18 years ago
|
||
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".
Comment 19•18 years ago
|
||
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 20•18 years ago
|
||
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.
Reporter | ||
Comment 22•18 years ago
|
||
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.
Comment 23•18 years ago
|
||
(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?
Reporter | ||
Comment 24•18 years ago
|
||
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.
Reporter | ||
Comment 26•18 years ago
|
||
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?
Comment 28•18 years ago
|
||
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
Reporter | ||
Comment 29•18 years ago
|
||
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
Reporter | ||
Updated•18 years ago
|
Reporter | ||
Comment 30•18 years ago
|
||
*** Bug 357837 has been marked as a duplicate of this bug. ***
Reporter | ||
Updated•18 years ago
|
Summary: GetChildAtPoint() broken for accessibles of out of flow objects → Bounds, GetChildAtPoint() and visible extState broken for accessibles of out of flow objects
Reporter | ||
Updated•18 years ago
|
Reporter | ||
Comment 31•18 years ago
|
||
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.
Reporter | ||
Updated•18 years ago
|
Comment 32•18 years ago
|
||
Is this the underlying problem behind bug 372624 and/or bug 369855?
Reporter | ||
Comment 33•18 years ago
|
||
No, it's unrelated. The mozilla/accessible code does not affect anything in layout, which is where the caret code lives.
Reporter | ||
Updated•17 years ago
|
Assignee: aaronleventhal → surkov.alexander
Assignee | ||
Comment 34•17 years ago
|
||
I can't get now why it returns not topmost frame.
Assignee | ||
Comment 35•17 years ago
|
||
Assignee | ||
Comment 36•17 years ago
|
||
(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?
Assignee | ||
Comment 37•17 years ago
|
||
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.
Reporter | ||
Comment 38•17 years ago
|
||
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)
Assignee | ||
Comment 39•17 years ago
|
||
1) state patch
2) bounds looks ok
3) bug 388927 address getChildAtPoint()
Attachment #279698 -
Flags: review?(aaronleventhal)
Reporter | ||
Updated•17 years ago
|
Attachment #279698 -
Flags: review?(aaronleventhal)
Attachment #279698 -
Flags: review+
Attachment #279698 -
Flags: approval1.9?
Updated•17 years ago
|
Attachment #279698 -
Flags: approval1.9? → approval1.9+
Reporter | ||
Comment 40•17 years ago
|
||
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.
Description
•