Closed Bug 768786 Opened 8 years ago Closed 8 years ago

Some accessible objects are not marked as offscreen

Categories

(Core :: Disability Access APIs, defect)

x86_64
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: Coolikov.Alex, Assigned: surkov)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 1 obsolete file)

Attached image 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.
Component: Untriaged → Disability Access
Component: Disability Access → Disability Access APIs
Product: Firefox → Core
QA Contact: untriaged → accessibility-apis
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.
(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.
Blocks: statea11y
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?
(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.
(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.
(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.
I can't reproduce, when those items are scrolled off then I see offscreen state. Did you try nightly build?
(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.
I can reproduce
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #662422 - Flags: review?(trev.saunders)
Attachment #662422 - Flags: feedback?(roc)
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))?
Attachment #662422 - Flags: feedback?(roc) → feedback+
Attached patch patch2Splinter Review
Attachment #662422 - Attachment is obsolete: true
Attachment #662422 - Flags: review?(trev.saunders)
Attachment #662494 - Flags: review?(trev.saunders)
(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 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.
Attachment #662494 - Flags: review?(trev.saunders) → review+
(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.
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
http://hg.mozilla.org/integration/mozilla-inbound/rev/880c0c02dcc0
Flags: in-testsuite+
Target Milestone: --- → mozilla18
https://hg.mozilla.org/mozilla-central/rev/880c0c02dcc0
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.