882767 - don't expose whitespace accessibles in context of grids

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

we were reported that whitespaces between grid cells is a problem for AT. They don't have any semantics (they are used as margins between cells) so we can be smarter to not create an accessibles for them.

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch

probably one day we should have more generic rules to not allow text leafs under controls

Comment 2

[Trevor Saunders (:tbsaunde)]

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

>+    if (nsCoreUtils::IsWhitespaceString(text)) {
>+      Accessible* sibling =
>+        document->GetAccessible(content->GetPreviousSibling());
>+      if (sibling && sibling->IsTableCell())
>+        return nullptr;
>+
>+      sibling = document->GetAccessible(content->GetNextSibling());
>+      if (sibling && sibling->IsTableCell())
>+        return nullptr;

why not just always disallow text leaf as child of row?

>+nsCoreUtils::IsWhitespaceString(const nsSubstring& aString)
>+{

I think you could just use nsContentUtils::TrimWhitespace<nsContentUtils::IsHTMLWhitespaceOrNBSP>() but maybe add nice inline wrapper to nsContentUtils IsOnlyWhitespace() or something?

Comment 3

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #2)
> Comment on attachment 763066 [details] [diff] [review]
> patch

> why not just always disallow text leaf as child of row?

I thought of some crazy grids that puts anything under row, not sure if they are in the wild but people constantly do wild things

Comment 4

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #3)
> (In reply to Trevor Saunders (:tbsaunde) from comment #2)
> > Comment on attachment 763066 [details] [diff] [review]
> > patch
> 
> > why not just always disallow text leaf as child of row?
> 
> I thought of some crazy grids that puts anything under row, not sure if they
> are in the wild but people constantly do wild things

yeah, but caring about that case makes things a lot slower, and it seems really crazy, espeically under tables.

Comment 5

[alexander :surkov (:asurkov)]

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

> I think you could just use
> nsContentUtils::TrimWhitespace<nsContentUtils::IsHTMLWhitespaceOrNBSP>() but
> maybe add nice inline wrapper to nsContentUtils IsOnlyWhitespace() or
> something?

it sounds good, expect TrimWhitespace creates unnecessary temporary object in our case but you know due to some reason I get mochitest failures related with name computation when I switch to  IsHTMLWhitespaceOrNBSP. I'd filed rather a follow up on this rather than investigate it. Ok?

Comment 6

[alexander :surkov (:asurkov)]

Attachment: patch2

Comment 7

[alexander :surkov (:asurkov)]

1) don't create whitespace accessibles if context is table row
2) keep IsWhitespace logic in a11y (because of nasty failures I wouldn't want in)
3) keep mGenericTypes |= eTableCell (not necessary for this work but should be ok)

Comment 8

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #5)
> (In reply to Trevor Saunders (:tbsaunde) from comment #2)
> 
> > I think you could just use
> > nsContentUtils::TrimWhitespace<nsContentUtils::IsHTMLWhitespaceOrNBSP>() but
> > maybe add nice inline wrapper to nsContentUtils IsOnlyWhitespace() or
> > something?
> 
> it sounds good, expect TrimWhitespace creates unnecessary temporary object
> in our case but you know due to some reason I get mochitest failures related
> with name computation when I switch to  IsHTMLWhitespaceOrNBSP. I'd filed
> rather a follow up on this rather than investigate it. Ok?

ok :/

Comment 9

[Trevor Saunders (:tbsaunde)]

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

>+    if (text.IsEmpty() ||
>+        (nsCoreUtils::IsWhitespaceString(text) && aContext->IsTableRow())) {

flip the order of these two since the second is faster.

Comment 10

[alexander :surkov (:asurkov)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/5d32adf0d68d

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5d32adf0d68d