102192 - Active Accessibility: Support STATE_OFFSCREEN in IAccessible states

Status: RESOLVED FIXED

(SeaMonkey :: General, defect, P2)

Description

[Aaron Leventhal]

The STATE_OFFSCREEN bitflag should be returned in Accessible state when an
element is offscreen. The accessibility vendors have asked for a quick, rough
calculation. Therefore, for simplicity we just check to see if the top left
corner of the object's frame is off screen.

More comments in patch.

Comment 1

[Aaron Leventhal]

Attachment: This has been tested - works well

Comment 2

[John Gaunt (redfive)]

Umm.... I need code to do a review and all I see in the patch is some text
comments. 

Comment 3

[Aaron Leventhal]

Attachment: The last patch was the wrong file - this is the right one

Comment 4

[John Gaunt (redfive)]

Comment on attachment 51258 [details] [diff] [review]
The last patch was the wrong file - this is the right one

looks good. I'm confused why the GetOffsetFromView returns negative numbers for both the x and y, but I trust you and I'll do digging for my own satisfaction later.

r=jgaunt

Comment 5

[Aaron Leventhal]

Sorry, I might have told you that GetOffsetFromView returns negative numbers.
The one returning negative numbers is this:
containingView->GetBounds(containingViewRect);

Therefore our final comparison works:
if (frameOffset.y < -containingViewRect.y || frameOffset.y +
containingViewRect.y > scrollViewRect.height || frameOffset.x <
-containingViewRect.x || frameOffset.x + containingViewRect.x >
scrollViewRect.width)
  *aAccState |= STATE_OFFSCREEN;

The expression |-containingViewRect.y| becomes a 0 or positive value
representing how much of the top of document, in twips, is not currently shown
on the screen.
  

Comment 6

[Chris Waterson]

kmcclusk, this looks okay to me...what do you think?

Comment 7

[Kevin McCluskey (gone)]

This is fine for most cases, but I think it will fail if there are intervening views

consider the following view hierarchy where viewC has the frame:

scrollview
   |- viewA
        |-viewB
            |-viewC (Frame)
 
To get the absolute position of the Frame you must add together the offsets for
ViewA, ViewB, and ViewC as well as the frame's offset. The patch looks like it
always assumes it can calculate the position of the frame relative to the view
by adding in only it's containing views offset.

Another issue is if the frame is inside a scrolling div:

scrollingViewA
    view1
      scrollingViewB 
             view2
                 view3 (frame)


The position of scrollingViewB may be off screen and that may cause all of the
views it contains to also be offscreen, but the patch only looks for the nearest
enclosing scrollingView, so relative to ScrollingViewB the frame appears to be
onscreen.

A third issue would be clipping. Views may have clip set which would make the
element invisible, but I'm not sure if they would still be considered on screen.

Comment 8

[Kevin McCluskey (gone)]

The current patch will probably satisfy 90%+ of the existing websites.

If you want a more accurate determination of visibility, You should be able to
use some code nsViewManager.cpp to help you out.

PRBool nsViewManager::IsRectVisible(nsIView *aView, const nsRect &aRect).

This method takes the view in question and rect within the view and returns a
bool to indicate whether it is visible. 

You should be able to pass the frames bounds as aRect and the frames containing
view and you will get back a boolean which indicates whether any portion of the
rect is visible.

This call is used within the viewmanager to avoid invalidating rects which are
completely invisible.

IsRectVisible would need to made part of nsIViewManager's public interface.

Comment 9

[Aaron Leventhal]

Attachment: This doesn't work - but I think it's what Kevin is suggesting - advice please?

Comment 10

[Aaron Leventhal]

Attachment: Fix using some altered code from nsIViewManager::IsRectVisible

Comment 11

[Aaron Leventhal]

Kevin, I had to copy and paste IsRectVisible into my code, because it needed
some modification for my purposes. Mainly, I wanted half-visible rectangles to
be counted as not visible.

In the process, I had to expose two methods through nsIViewManager
GetAbsoluteRect(nsIView *aView, const nsRect &aRect, nsRect&
aAbsRect)GetVisibleRect(nsRect& aVisibleRect)

Comment 12

[Kevin McCluskey (gone)]

Comment on attachment 51494 [details] [diff] [review]
Fix using some altered code from nsIViewManager::IsRectVisible

looks good!
r=[e-mail]

Comment 13

[Chris Waterson]

Rather than copying and pasting this code (and worse, copying it _out_ of the
view module to a place where we'll never be able to find it again), how about
you modify the view manager in such a way that you can issue the query you need
to. Maybe add a boolean argument to IsRectVisible(), or at least implement
IsRectEntirelyVisible() as another method on the view manager. Kevin, would you
have any objections to this approach?

Comment 14

[Kevin McCluskey (gone)]

"Kevin, would you have any objections to this approach?".  No thats fine with
me.     If we need to add a new argument to a method on the ViewManager which
indicates whether only part of the rectangle is visible that's fine with me.

Comment 15

[Kevin McCluskey (gone)]

IsRectVisible(nsIView *aView, const nsRect &aRect) could be modified to have an
additional argument which the visible part of the aRect that is passed in.

IsRectVisible(nsIView *aView, const nsRect &aRect, nsRect &aVisibleRect)

If (aVisibleRect==aRect) then the entire rect is visible.

Comment 16

[Chris Waterson]

(aaronl: hope I didn't sound like too much of a smarty pants: I didn't know
about the IsRectVisible method, either. But now that we _do_ know, it seems like
putting this in view manager may be better...)

Comment 17

[Aaron Leventhal]

Attachment: Revision heeding reasoned refactoring request

Comment 18

[Aaron Leventhal]

Attachment: This is the patch that needs r=/sr=

Comment 19

[Chris Waterson]

I'd be fine with you leaving nsIViewManager::IsRectVisible()'s return signature
as a PRBool...no need to get carried away with COM here. (I suspect in the long
run we probably ought to consider getting rid of the extra COM layer here anyway...)

Comment 20

[Kevin McCluskey (gone)]

Comment on attachment 51736 [details] [diff] [review]
This is the patch that needs r=/sr=

r=[e-mail]. Patch looks fine to me. I agree with waterson in regard to not needing to maintain nsIViewManager.h as a com interface, but it's OK with me if you check it in "as is".

Comment 21

[Chris Waterson]

Go ahead and revert the return type to PRBool, and sr=waterson.

Comment 22

[Chris Waterson]

Oy, I'm sorry. I thought that this was already a publicly accessible method that
you'd changed the signature of. (Who's not reading the patch carefully?) Go
ahead with attachment 51736 [details] [diff] [review] as it is. sr=waterson

Comment 23

[Chris Waterson]

Comment on attachment 51736 [details] [diff] [review]
This is the patch that needs r=/sr=

sr=waterson

Comment 24

[Aaron Leventhal]

-> checked in