922751 - Provide a state or method to distinguish obscured elements

Status: RESOLVED WONTFIX

(Core :: Disability Access APIs, defect)

Description

[Eitan Isaacson [:eeejay]]

Currently, there is no easy way to know if an element is obscured. This is important for a few cases:

1. In Gaia there is a lot of play and complexity with z-index for hiding and unhiding dialogs. Further, dialogs are typically styled with a small degree of transparency. Assuring that elements that are under the dialog are made unavailable to the screen reader becomes very hard and error prone.

2. There are many sites, and several js libraries that provide a kind of splash or dialog in web pages. The content of the page is dimmed, and a container is brought to the foreground. Screen readers still access the content of the page, and are oblivious to the modal nature of the splash content. In jQuery alone there is lightbox, colorbox, fancybox and thickbox.

I propose that, from an a11y perspective, an element would be considered obscured if its on-screen bounding box is entirely contained by an element with a higher z-order. The higher element cannot let pointer events through (it can't have a CSS style of "pointer-events: none;"), and it must have an opacity of 0.5 or higher.

How to introduce this in the current API? I could think of 3 options:

1. Obscured objects should have a state of INVISIBLE. Pros: Desktop screen readers would automatically benefit from this addition. Cons: There is a chance of false positives. Also, content creators deliberately obfuscate text that should only be read by screen readers and not seen.

2. Obscured objects should have a new state called OBSCURED. Pros: We don't break desktop, and we add more granularity to why an object is not visible (OFFSCREEN, INVISIBLE, OBSCURED). Cons: Desktop doesn't get to use this state. Also, there may be performance ramifications that they wouldn't even benefit from.

3. A method is introduced to nsIAccessible called isObscured(). Pros: Same as above, plus no extra overhead for getting states. Cons,: This really should be a state.

Comment 1

[Eitan Isaacson [:eeejay]]

Attachment: Introduce obscured state

I decided to go with a new state, because obscured objects could still technically receive focus, so they are not really invisible. They are not offscreen because offscreen is a mess :) but seriously, because obscured items cannot be unabscured by the user.

Comment 2

[alexander :surkov (:asurkov)]

Comment on attachment 814654 [details] [diff] [review]
Introduce obscured state

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

you need to fix gAtkStateMap to stay on safe side

general notes:
1) it doesn't seem the code handles popups when popup obscures some elements underneath (should it? like is offscreen state more suitable for it?)
2) obscured state may describe inert subtrees (would it cover visible inert subtrees though?), see "the user agent must act as if the element was absent for the purposes of targeting user interaction events, may ignore the node for the purposes of text search user interfaces (commonly known as "find in page"), and may prevent the user from selecting text in that node."
3) if opacity and mouse event pointer things are out then it's invisible state

so I'm fine with new state in general so f=me but I'd like to get Jamie's thoughts.

::: accessible/src/generic/Accessible.cpp
@@ +616,5 @@
> +{
> +  nsIFrame* frame = GetFrame();
> +
> +  DocAccessible* accDocument = Document();
> +  NS_ENSURE_TRUE(accDocument, 0);

nit: no need in the check

@@ +621,5 @@
> +
> +  nsIFrame* rootFrame = accDocument->GetFrame();
> +  NS_ENSURE_TRUE(rootFrame, 0);
> +
> +  nsRect rect(frame->GetOffsetTo(rootFrame), frame->GetSize());

a note: one day the logic should be fitted to shared-node accessible like bullets of listitem (bullet may be obscured while rest of listitem is visible)

@@ +632,5 @@
> +
> +    nsRect candidateRect = nsRect(f->GetOffsetTo(rootFrame), f->GetSize());
> +
> +    if (f == frame)
> +      break;

why? and it should be before you get candidateRect
nit: staying consistent is nicer: candidateRect vs f (candidateFrame is long but perhaps ok), alternatively frame and rect vs thisFrame and thisRect

@@ +646,5 @@
> +    }
> +
> +    if (rect.Union(candidateRect).Size() == candidateRect.Size()) {
> +      return true;
> +    }

nit: no braces pls

@@ +727,5 @@
>        return states::INVISIBLE;
>    }
>  
> +  if (IsObscured())
> +    return states::OBSCURED;

not running for in-popup frames (see IsPopup check above), we should might have use cases in Firefox UI

::: accessible/src/generic/Accessible.h
@@ +236,5 @@
>     * Return bit set of invisible and offscreen states.
>     */
>    uint64_t VisibilityState();
>  
> +  bool IsObscured();

maybe NativelyObscured to keep it closer to other names like NativelyUnavailable and please add a comment (rather a pure style issue)

Comment 3

[James Teh [:Jamie]]

Would this state be present for fallback content inside a canvas element? I feel it probably shouldn't be, as this content is generally accessible where the canvas element itself isn't.

Comment 4

[alexander :surkov (:asurkov)]

(In reply to James Teh [:Jamie] from comment #3)
> Would this state be present for fallback content inside a canvas element? 

I don't think, that content is considered visible for AT.

> I
> feel it probably shouldn't be, as this content is generally accessible where
> the canvas element itself isn't.

right

How do you feel about this state otherwise?

Comment 5

[James Teh [:Jamie]]

(In reply to alexander :surkov from comment #4)
> > Would this state be present for fallback content inside a canvas element? 
> I don't think, that content is considered visible for AT.
Arguably, it should be, since it's more useful than the canvas itself.

> How do you feel about this state otherwise?
In general, I think it's a good idea. It's not so important beyond touch screens, as keyboard focus can be restricted by the author, but it's definitely important for touch screens if we want to avoid the nightmare of everyone having to use aria-hidden when something modal appears.

Comment 6

[Eitan Isaacson [:eeejay]]

I am a bit concerned that adding obscured as a state will make getState() slower, since determining if an element is obscured or not is not trivial. Maybe this should just be a separate method? isObscured()?

Comment 7

[James Teh [:Jamie]]

From the IA2 perspective, if we didn't go with a state, this could be an object attribute fetched only with IAccessible2_2::attribute. That said, this part should probably be addressed in a separate bug, since I don't even have a use case for it yet on Windows.

Comment 8

[James Teh [:Jamie]]

Comment on attachment 814654 [details] [diff] [review]
Introduce obscured state

See previous comments for my feedback. :)

Comment 9

[alexander :surkov (:asurkov)]

(In reply to Eitan Isaacson [:eeejay] from comment #6)
> I am a bit concerned that adding obscured as a state will make getState()
> slower, since determining if an element is obscured or not is not trivial.
> Maybe this should just be a separate method? isObscured()?

it makes sense, I agree though keeping it as a state is very tempting.

Comment 10

[Eitan Isaacson [:eeejay]]

I'm going to stick to a state, if anyone has ideas for how to profile this, please let me know.

The general semantics of an obscured object are as follows:
* An object is behind another object with a higher z-index AND
* the higher object's bounds completely encompass the lower object AND
* the higher object's opacity is greater than 0.5 AND
* the higher object receives pointer events (style="pointer-events: auto;")

OR:

* The object is in a viewport (NOT style="overflow: visible;") AND
* the viewport is obscured (recurse this criteria)

Comment 11

[James Teh [:Jamie]]

In general, this sounds good. However, Eitan, do you have thoughts on comment:3?

Comment 12

[Eitan Isaacson [:eeejay]]

(In reply to James Teh [:Jamie] from comment #11)
> In general, this sounds good. However, Eitan, do you have thoughts on
> comment:3?

Of course, fallback elements in a canvas are not considered obscured unless the canvas is obscured.

Comment 13

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #2)
> @@ +632,5 @@
> > +
> > +    nsRect candidateRect = nsRect(f->GetOffsetTo(rootFrame), f->GetSize());
> > +
> > +    if (f == frame)
> > +      break;
> 
> why? 

The candidate list is in stacking order. So if we reach the frame, it means it is not obscured.

Comment 14

[Eitan Isaacson [:eeejay]]

Attachment: Introduce obscured state

Just by observing the mochitest test duration, I don't see a real time difference in performance, so I thought of keeping this as a state. Tried to integrate this into VisibilityState() in a way that eliminates excessive frame tree walking, although there is still plenty of that.

Comment 15

[alexander :surkov (:asurkov)]

(In reply to Eitan Isaacson [:eeejay] from comment #13)

> > > +    if (f == frame)
> > > +      break;
> > 
> > why? 
> 
> The candidate list is in stacking order. So if we reach the frame, it means
> it is not obscured.

please add that in comment

Comment 16

[alexander :surkov (:asurkov)]

Comment on attachment 822604 [details] [diff] [review]
Introduce obscured state

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

::: accessible/src/generic/Accessible.cpp
@@ +698,5 @@
>          scrollPortRect.Deflate(kMinPixels, kMinPixels);
>          if (!scrollPortRect.Intersects(frameRect))
> +          return states::OFFSCREEN |
> +                 (IsObscured(rootFrame, parentFrame, scrollableFrame->GetScrollPortRect()) ?
> +                  states::OBSCURED : 0);

why do you parentFrame instead frame and only if it's offscreen? a comment would be helpful

(btw, wrong indentation)

Comment 17

[Eitan Isaacson [:eeejay]]

Attachment: Introduce obscured state

Fixed some coordinated offset issues. Added some commments for readability.

Comment 18

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #16)
> Comment on attachment 822604 [details] [diff] [review]
> Introduce obscured state
> 
> Review of attachment 822604 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/src/generic/Accessible.cpp
> @@ +698,5 @@
> >          scrollPortRect.Deflate(kMinPixels, kMinPixels);
> >          if (!scrollPortRect.Intersects(frameRect))
> > +          return states::OFFSCREEN |
> > +                 (IsObscured(rootFrame, parentFrame, scrollableFrame->GetScrollPortRect()) ?
> > +                  states::OBSCURED : 0);
> 
> why do you parentFrame instead frame and only if it's offscreen? a comment
> would be helpful

If an item's viewport is obscured, the item is considered obscured. In the case that the item is not offscreen, it is directly obstructed, so calling IsObscured on |this| frame at the end of the function would return true.

If the item is offscreen, it may not be directly obstructed, but if the viewport is, the item is considered obscured as well. So here we check if this is the case with parentFrame.

Comment 19

[alexander :surkov (:asurkov)]

(In reply to Eitan Isaacson [:eeejay] from comment #18)

> If the item is offscreen, it may not be directly obstructed, but if the
> viewport is, the item is considered obscured as well. So here we check if
> this is the case with parentFrame.

what about the case when item if offscreen and obscured (viewport is not)? what about the case when scroll frame contains a scroll frame (like top is obscured but bottom one is offscrolled?)

Comment 20

[Eitan Isaacson [:eeejay]]

(In reply to alexander :surkov from comment #19)
> (In reply to Eitan Isaacson [:eeejay] from comment #18)
> 
> > If the item is offscreen, it may not be directly obstructed, but if the
> > viewport is, the item is considered obscured as well. So here we check if
> > this is the case with parentFrame.
> 
> what about the case when item if offscreen and obscured (viewport is not)?

I have mixed feelings about this case. If it is offscreen, there is still a chance that if the user scrolls to it, it will be unobscured as well. I will play with an implementation for that, I am not even sure if it is possible with the current layout API.

> what about the case when scroll frame contains a scroll frame (like top is
> obscured but bottom one is offscrolled?)

I should add a test for that, but I think it should work with the current implementation.

Comment 21

[Eitan Isaacson [:eeejay]]

Attachment: Introduce obscured state

This addresses the cases Alex mentioned above, and cleans up the tests.

Comment 22

[alexander :surkov (:asurkov)]

Comment on attachment 827815 [details] [diff] [review]
Introduce obscured state

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

it'd be great if you can get superreview from roc to stay on safe side

::: accessible/src/generic/Accessible.cpp
@@ +614,5 @@
> +    }
> +
> +    nsIFrame* parent = nsLayoutUtils::GetCrossDocParentFrame(vpFrame);
> +
> +    if (!parent)

nit: no empty line between these lines pls

::: accessible/tests/mochitest/states/test_obscured.html
@@ +31,5 @@
> +    {
> +      var tabDoc = currentTabDocument();
> +
> +      function _getAcc(aSelector) {
> +        return getAccessible(tabDoc.querySelector(aSelector));

maybe accFromSelector or maybe change getAccesisble() to work with selectors

Comment 23

[Eitan Isaacson [:eeejay]]

Comment on attachment 827815 [details] [diff] [review]
Introduce obscured state

Hey Robert.

I did all sorts of things here with the layout API. Checking in with you to see if it is sensible.

Comment 24

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

Comment on attachment 827815 [details] [diff] [review]
Introduce obscured state

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

I'm going to sr+ because I don't mind if you land without these changes, but you should think about them.

::: accessible/src/generic/Accessible.cpp
@@ +581,5 @@
> +  nsIFrame* vpFrame = aRootFrame ?
> +    aRootFrame : aFrame->PresContext()->PresShell()->GetRootFrame();
> +  nsIFrame* curFrame = aFrame;
> +  while (vpFrame) {
> +    nsRect rect(curFrame->GetOffsetToCrossDoc(vpFrame), curFrame->GetSize());

Use TransformFrameRectToAncestor instead.

@@ +601,5 @@
> +        continue;
> +
> +      // If an object is less than 50% opaque, it is not considered obscuring.
> +      if (candidate->StyleDisplay()->mOpacity < 0.5f)
> +        continue;

Just checking candidate's opacity isn't very logical. For example candidate might have a parent whose opacity is 0.1, which would put all its descendants in a very transparent group.

@@ +604,5 @@
> +      if (candidate->StyleDisplay()->mOpacity < 0.5f)
> +        continue;
> +
> +      nsRect candidateRect(candidate->GetOffsetToCrossDoc(vpFrame),
> +                           candidate->GetSize());

Use TransformFrameRectToAncestor.

@@ +609,5 @@
> +
> +      // If our frame is completely contained by higher, opaque-ish element,
> +      // it is considered obscured.
> +      if (rect.Union(candidateRect).Size() == candidateRect.Size())
> +        return true;

The candidate frame might be completely transparent --- e.g., no background color, background image etc. But it's tricky to determine this since the frame might be for an element that paints itself, e.g. <img>. Up to you whether you ignore this problem.

Comment 25

[Eitan Isaacson [:eeejay]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #24)
> @@ +601,5 @@
> > +        continue;
> > +
> > +      // If an object is less than 50% opaque, it is not considered obscuring.
> > +      if (candidate->StyleDisplay()->mOpacity < 0.5f)
> > +        continue;
> 
> Just checking candidate's opacity isn't very logical. For example candidate
> might have a parent whose opacity is 0.1, which would put all its
> descendants in a very transparent group.
> 

Is there an easy way of knowing the actual opacity the frame will be composited with? I noticed that we have the same issue with how we determine OPAQUE state.

> 
> @@ +609,5 @@
> > +
> > +      // If our frame is completely contained by higher, opaque-ish element,
> > +      // it is considered obscured.
> > +      if (rect.Union(candidateRect).Size() == candidateRect.Size())
> > +        return true;
> 
> The candidate frame might be completely transparent --- e.g., no background
> color, background image etc. But it's tricky to determine this since the
> frame might be for an element that paints itself, e.g. <img>. Up to you
> whether you ignore this problem.

Is there way of knowing that the frame is not painted at all?

Comment 26

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

(In reply to Eitan Isaacson [:eeejay] from comment #25)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #24)
> > @@ +601,5 @@
> > > +        continue;
> > > +
> > > +      // If an object is less than 50% opaque, it is not considered obscuring.
> > > +      if (candidate->StyleDisplay()->mOpacity < 0.5f)
> > > +        continue;
> > 
> > Just checking candidate's opacity isn't very logical. For example candidate
> > might have a parent whose opacity is 0.1, which would put all its
> > descendants in a very transparent group.
> 
> Is there an easy way of knowing the actual opacity the frame will be
> composited with? I noticed that we have the same issue with how we determine
> OPAQUE state.

You can walk all the element's ancestors and see if any (including itself) have opacity != 1.0. You need to look across document boundaries too. "The actual opacity the frame will be composited with" isn't really a well-defined concept, that's not how opacity compositing works.

> > @@ +609,5 @@
> > > +
> > > +      // If our frame is completely contained by higher, opaque-ish element,
> > > +      // it is considered obscured.
> > > +      if (rect.Union(candidateRect).Size() == candidateRect.Size())
> > > +        return true;
> > 
> > The candidate frame might be completely transparent --- e.g., no background
> > color, background image etc. But it's tricky to determine this since the
> > frame might be for an element that paints itself, e.g. <img>. Up to you
> > whether you ignore this problem.
> 
> Is there way of knowing that the frame is not painted at all?

The question is not whether it's painted at all, but whether it paints some part of itself opaquely.

Detecting "obscured" elements seems very hard and fragile to me. Also there is a risk that app developers will come to depend on our heuristics for accessibility to work, and that's bad since our heuristics will be pretty much arbitrary. That will create compatibility issues between versions and even potentially across different browsers :-(.

Fudging the issue by introducing a new "obscured" state for ATs to use seems to me to basically throw an unsolvable problem over to the ATs, where they have even less information to solve the problem.

Personally I think you should adopt a very simple and strict approach to determining, from CSS, when an element is  invisible: display:none (no frame), or visibility:hidden. On top of that you should require apps to signal explicitly that content should not be exposed to the AT (via ARIA?).

Comment 27

[Marco Zehe (:MarcoZ)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #26)
> Detecting "obscured" elements seems very hard and fragile to me. Also there
> is a risk that app developers will come to depend on our heuristics for
> accessibility to work, and that's bad since our heuristics will be pretty
> much arbitrary. That will create compatibility issues between versions and
> even potentially across different browsers :-(.
> 
> Fudging the issue by introducing a new "obscured" state for ATs to use seems
> to me to basically throw an unsolvable problem over to the ATs, where they
> have even less information to solve the problem.
> 
> Personally I think you should adopt a very simple and strict approach to
> determining, from CSS, when an element is  invisible: display:none (no
> frame), or visibility:hidden. On top of that you should require apps to
> signal explicitly that content should not be exposed to the AT (via ARIA?).

Thank you, Robert! Following along this bug, I had a growing feeling of uneasiness, and you just summarized this very well! The approach to define clearly which elements should be made accessible to AT at any given time through CSS and ARIA. We have had enough problems with visibility over the years that an Obscured state, although seeming the easier solution at first, could easily be growing into a nightmare of maintainability.

Requiring the exact "visibility" to AT to be defined via CSS and ARIA is more work initially, and definitely more work in educating web developers, but I believe it is, in the long run, a more bullet-proof approach. It means we'll have more work in GAIA, too, initially, but I am hopeful that with the move to web components, this will all become easier.

Comment 28

[Eitan Isaacson [:eeejay]]

After roc and marco's comments, I am marking this as WONTFIX. I don't think we could afford false positives, and it seems very likely to occur on a regular basis.