The default bug view has changed. See this FAQ.

don't expose whitespace accessibles in context of grids

RESOLVED FIXED in mozilla25

Status

()

Core
Disability Access APIs
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: surkov, Assigned: surkov)

Tracking

(Blocks: 1 bug, {access})

unspecified
mozilla25
access
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

12.66 KB, patch
tbsaunde
: review+
Details | Diff | Splinter Review
(Assignee)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
Created attachment 763066 [details] [diff] [review]
patch

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?
(Assignee)

Comment 3

4 years ago
(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.
(Assignee)

Comment 5

4 years ago
(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?
(Assignee)

Comment 6

4 years ago
Created attachment 768881 [details] [diff] [review]
patch2
Attachment #763066 - Attachment is obsolete: true
Attachment #763066 - Flags: review?(trev.saunders)
Attachment #768881 - Flags: review?(trev.saunders)
(Assignee)

Comment 7

4 years ago
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+
(Assignee)

Comment 10

4 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/5d32adf0d68d
(Assignee)

Updated

4 years ago
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/5d32adf0d68d
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.