Closed
Bug 882767
Opened 11 years ago
Closed 11 years ago
don't expose whitespace accessibles in context of grids
Categories
(Core :: Disability Access APIs, defect)
Core
Disability Access APIs
Tracking
()
RESOLVED
FIXED
mozilla25
People
(Reporter: surkov, Assigned: surkov)
References
(Blocks 1 open bug)
Details
(Keywords: access)
Attachments
(1 file, 1 obsolete file)
12.66 KB,
patch
|
tbsaunde
:
review+
|
Details | Diff | Splinter Review |
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•11 years ago
|
||
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 2•11 years ago
|
||
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•11 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
Comment 4•11 years ago
|
||
(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•11 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•11 years ago
|
||
Attachment #763066 -
Attachment is obsolete: true
Attachment #763066 -
Flags: review?(trev.saunders)
Attachment #768881 -
Flags: review?(trev.saunders)
Assignee | ||
Comment 7•11 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)
Comment 8•11 years ago
|
||
(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•11 years ago
|
||
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•11 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/5d32adf0d68d
Assignee | ||
Updated•11 years ago
|
Flags: in-testsuite+
Comment 11•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/5d32adf0d68d
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in
before you can comment on or make changes to this bug.
Description
•