768786 - Some accessible objects are not marked as offscreen

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Alex Kulikov, Ai Squared]

Attachment: 1.png

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/536.5 (KHTML, like Gecko) Chrome/19.0.1084.56 Safari/536.5

Steps to reproduce:

Steps to reproduce:
1) Open http://www.aisquared.com/ and find Facebook social plugin at the bottom of the page;
2)Scroll the plugin content so some text is overlapped by the "ZoomText on Facebook" section;
3)Open AccProbe and find an overlapped text;
4)Check object state.


Actual results:

Overlapped text does not have STATE_SYSTEM_OFFSCREEN state (but it is not visible on the screen).


Expected results:

Overlapped text should have STATE_SYSTEM_OFFSCREEN state.

Comment 1

[Marco Zehe (:MarcoZ)]

Is the state off-screen really for stuff that is overlapped by other layers? I thought off-screen was for items that are scrolled outside, not overlapped.

Comment 2

[Alex Kulikov, Ai Squared]

(In reply to Marco Zehe (:MarcoZ) from comment #1)
> Is the state off-screen really for stuff that is overlapped by other layers?
> I thought off-screen was for items that are scrolled outside, not overlapped.
Objects become visible if I call IAccessible2::scrollTo(IA2_SCROLL_TYPE_TOP_LEFT) method, so I think they are scrolled out.

Comment 3

[alexander :surkov (:asurkov)]

Alex, I don't see role heading anymore (grand parent of 'ZoomText on Facebook' link). Did they change the web site? Do you have another testcase?

Comment 4

[Alex Kulikov, Ai Squared]

(In reply to alexander :surkov from comment #3)
> Alex, I don't see role heading anymore (grand parent of 'ZoomText on
> Facebook' link). Did they change the web site? Do you have another testcase?
I don't think they've changed the site. I can still reproduce the issue: text that is overlapped by the "ZoomText on Facebook" section does not have STATE_SYSTEM_OFFSCREEN state.

Comment 5

[alexander :surkov (:asurkov)]

(In reply to Alex Kulikov, Ai Squared from comment #4)
> (In reply to alexander :surkov from comment #3)
> > Alex, I don't see role heading anymore (grand parent of 'ZoomText on
> > Facebook' link). Did they change the web site? Do you have another testcase?
> I don't think they've changed the site. I can still reproduce the issue:
> text that is overlapped by the "ZoomText on Facebook" section does not have
> STATE_SYSTEM_OFFSCREEN state.

which text? where can I locate it? I don't longer see the heading accessible that was selected on your screenshot.

Comment 6

[Alex Kulikov, Ai Squared]

Attachment: screenshot that illustrates the issue

Comment 7

[Alex Kulikov, Ai Squared]

(In reply to alexander :surkov from comment #5)
> (In reply to Alex Kulikov, Ai Squared from comment #4)
> > (In reply to alexander :surkov from comment #3)
> > > Alex, I don't see role heading anymore (grand parent of 'ZoomText on
> > > Facebook' link). Did they change the web site? Do you have another testcase?
> > I don't think they've changed the site. I can still reproduce the issue:
> > text that is overlapped by the "ZoomText on Facebook" section does not have
> > STATE_SYSTEM_OFFSCREEN state.
> 
> which text? where can I locate it? I don't longer see the heading accessible
> that was selected on your screenshot.
I`ve attached a new screenshot that shows the issue. Please let me know if you need any additional information.

Comment 8

[alexander :surkov (:asurkov)]

I can't reproduce, when those items are scrolled off then I see offscreen state. Did you try nightly build?

Comment 9

[Alex Kulikov, Ai Squared]

Attachment: The same issue on the FF Nightly 18.0a1 (2012-09-10)

Comment 10

[Alex Kulikov, Ai Squared]

(In reply to alexander :surkov from comment #8)
> I can't reproduce, when those items are scrolled off then I see offscreen
> state. Did you try nightly build?
Sorry for the late response. I can reproduce the issue with the latest FF nightly build. I've attached a new screenshot that shows the issue. Please let me know if you need any additional info.

Comment 11

[alexander :surkov (:asurkov)]

I can reproduce

Comment 12

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 13

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

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

Review of attachment 662422 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/generic/Accessible.cpp
@@ +636,5 @@
> +      nsRect frameRect(frame->GetOffsetTo(parentFrame), frame->GetSize());
> +      if (!scrollPortRect.Contains(frameRect)) {
> +        const nscoord kMinPixels = nsPresContext::CSSPixelsToAppUnits(12);
> +        scrollPortRect.Deflate(kMinPixels, kMinPixels);
> +        if (frameRect.YMost() <= scrollPortRect.y)

Can't you use if (scrollPortRect.Intersects(frameRect))?

Comment 14

[alexander :surkov (:asurkov)]

Attachment: patch2

Comment 15

[alexander :surkov (:asurkov)]

(In reply to alexander :surkov from comment #14)
> Created attachment 662494 [details] [diff] [review]
> patch2

1) using Intersects as roc suggested
2) use calculate offset to avoid double parent chain traversal and assertion that frame and scrollable frame belongs different documents

Comment 16

[Trevor Saunders (:tbsaunde)]

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

>+      nsRect scrollPortRect = scrollableFrame->GetScrollPortRect();
>+      nsRect frameRect(framePos, frame->GetSize());
>+      if (!scrollPortRect.Contains(frameRect)) {
>+        const nscoord kMinPixels = nsPresContext::CSSPixelsToAppUnits(12);
>+        scrollPortRect.Deflate(kMinPixels, kMinPixels);
>+        if (!scrollPortRect.Intersects(frameRect))
>+          return states::OFFSCREEN;
>+      }
>+    }

So, if I read this correctly if you have an accessible whose frame or parent frame is less than 16px by 16px but completely within the scroll rect then its considered visible, and on screen, but if you have the same amount of a larger frame in the scroll rect then its off screen, that seems kind of inconsistant, but I'm not sure what to do about it

>+      gQueue.push(new addTabInvoker("about:blank", testBackgroundTab));
>+      gQueue.push(new loadURIInvoker(gDocURI, testScrolledOff));
c> 

I'm not exactly thrilled about the indirection of the invoker taking an argument of the function to call.  I think it makes it a little more work to see what the test does.

Comment 17

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #16)

> So, if I read this correctly if you have an accessible whose frame or parent
> frame is less than 16px by 16px but completely within the scroll rect then
> its considered visible, and on screen, but if you have the same amount of a
> larger frame in the scroll rect then its off screen, that seems kind of
> inconsistant, but I'm not sure what to do about it

agree, just preserving the logic.

> >+      gQueue.push(new addTabInvoker("about:blank", testBackgroundTab));
> >+      gQueue.push(new loadURIInvoker(gDocURI, testScrolledOff));
> c> 
> 
> I'm not exactly thrilled about the indirection of the invoker taking an
> argument of the function to call.  I think it makes it a little more work to
> see what the test does.

it's good for code generalization (it can be reused very easy) but I'd agree that the testing function should be a primary thing, the action function should be a secondary thing.

Comment 18

[alexander :surkov (:asurkov)]

but if you don't mind then I would go with what we have, I don't have good ideas how to make things nicer

Comment 19

[alexander :surkov (:asurkov)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/880c0c02dcc0

Comment 20

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/880c0c02dcc0