Closed Bug 882767 Opened 7 years ago Closed 7 years ago

don't expose whitespace accessibles in context of grids

Categories

(Core :: Disability Access APIs, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: surkov, Assigned: surkov)

References

(Blocks 1 open bug)

Details

(Keywords: access)

Attachments

(1 file, 1 obsolete file)

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.
Attached patch patch (obsolete) — Splinter Review
probably one day we should have more generic rules to not allow text leafs under controls
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #763066 - Flags: review?(trev.saunders)
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?
(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
(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.
(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?
Attached patch patch2Splinter Review
Attachment #763066 - Attachment is obsolete: true
Attachment #763066 - Flags: review?(trev.saunders)
Attachment #768881 - Flags: review?(trev.saunders)
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)
(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 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.
Attachment #768881 - Flags: review?(trev.saunders) → review+
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/5d32adf0d68d
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.