group position of HTML radio buttons might be incorrect

RESOLVED FIXED in mozilla22

Status

()

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

People

(Reporter: surkov, Assigned: Aishwarya)

Tracking

unspecified
mozilla22
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=surkov.alexander@gmail.com][lang=c++])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

5 years ago
if one of <input type="radio"> has display:none style then it still participates in group position calculations.

see HTMLRadioButtonAccessible::GetPositionAndSizeInternal (http://mxr.mozilla.org/mozilla-central/source/accessible/src/html/HTMLFormControlAccessible.cpp#142)

we don't take into account whether the element is accessible or not. All you need is to check HasAccessible() before incrementing 'count' variable

please don't forget to add a test into attributes/test_obj_group.html

markup is:
<form>
<input type="radio" style="display: none;">
<input type="radio" id="radio5">
</form>
(Assignee)

Comment 1

5 years ago
Created attachment 665235 [details] [diff] [review]
Patch

I have checked for accessibility before incrementing.
Attachment #665235 - Flags: feedback?(surkov.alexander)
(Reporter)

Comment 2

5 years ago
Comment on attachment 665235 [details] [diff] [review]
Patch

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

::: accessible/src/html/HTMLFormControlAccessible.cpp
@@ +173,5 @@
>                                type, eCaseMatters) &&
>          inputElm->AttrValueIs(kNameSpaceID_None, nsGkAtoms::name,
>                                name, eCaseMatters)) {
> +      if(inputElm->HasAccessible())
> +        count++; 

nit: space after if
nit: space in the end of line
nit: new line please

please make sure to build the patch and run mochitests

inputElm doesn't have HasAccessible() method, but DocAccessible has it. You should do instead
if (mDoc->HasAccessible(inputElm))

::: accessible/tests/mochitest/attributes/test_obj_group.html
@@ +321,5 @@
>  
> +  <form>
> +    <input type="radio" style="display: none;">
> +    <input type="radio" id="radio5">
> +  </form>

please add testing code, see in this file above, for example, testing for "radio4"
Attachment #665235 - Flags: feedback?(surkov.alexander)
(Reporter)

Comment 3

5 years ago
Aishwarya, are you working on it? I'd be great to finish it. Thank you.
(Assignee)

Comment 4

5 years ago
Created attachment 678116 [details] [diff] [review]
Proposed patch - Added test to radio5 and changed the calling function to count

I have taken the radio5 in the form to be of a new form group with 2 elements in the set. Hence I have given  testGroupAttrs("radio5", 1, 2). Could you let me know if this is correct?
Attachment #665235 - Attachment is obsolete: true
Attachment #678116 - Flags: feedback?(surkov.alexander)
(Reporter)

Comment 5

5 years ago
Comment on attachment 678116 [details] [diff] [review]
Proposed patch - Added test to radio5 and changed the calling function to count

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

please fix nits, file new patch and ask :tbsaunde for review

::: accessible/src/html/HTMLFormControlAccessible.cpp
@@ +173,4 @@
>                                type, eCaseMatters) &&
>          inputElm->AttrValueIs(kNameSpaceID_None, nsGkAtoms::name,
>                                name, eCaseMatters)) {
> +      if (mDoc->HasAccessible(inputElm))

it's equivalent but I would prefer to put HasAccessible into 'if' above

@@ +174,5 @@
>          inputElm->AttrValueIs(kNameSpaceID_None, nsGkAtoms::name,
>                                name, eCaseMatters)) {
> +      if (mDoc->HasAccessible(inputElm))
> +        count++;
> +            

nit: whitespaces on empty line

::: accessible/tests/mochitest/attributes/test_obj_group.html
@@ +34,5 @@
>        // HTML input@type="radio" within form
>        testGroupAttrs("radio1", 1, 2);
>        testGroupAttrs("radio2", 2, 2);
> +      testGroupAttrs("radio5", 1, 2);
> +        

same

@@ +322,5 @@
>  
> +  <form>
> +    <input type="radio" style="display: none;">
> +    <input type="radio" id="radio5">
> +  </form>    

nit: whitespaces at the end of line
Attachment #678116 - Flags: feedback?(surkov.alexander) → feedback+
(Assignee)

Comment 6

5 years ago
Created attachment 678337 [details] [diff] [review]
Fixed white space and moved check into if condition

Please let me know if anything else needs to be changed. Thanks! =)
Attachment #678116 - Attachment is obsolete: true
Attachment #678337 - Flags: review?(surkov.alexander)
(Assignee)

Updated

5 years ago
Attachment #678337 - Flags: review?(surkov.alexander) → review?(trev.saunders)
(In reply to Aishwarya from comment #4)
> Created attachment 678116 [details] [diff] [review]
> Proposed patch - Added test to radio5 and changed the calling function to
> count
> 
> I have taken the radio5 in the form to be of a new form group with 2
> elements in the set. Hence I have given  testGroupAttrs("radio5", 1, 2).
> Could you let me know if this is correct?

that doesn't sound right, you have two radio inputs, but only one should be counted after your patch since the other is display: none however your current test expects there too be 2 radio inputs not 1 as there should be.
Comment on attachment 678337 [details] [diff] [review]
Fixed white space and moved check into if condition

per comment 7 the tests test for the old behavior, and so fail when with your change to HTMLFormControlAccessible.cpp
Attachment #678337 - Flags: review?(trev.saunders) → review-
(Assignee)

Comment 9

5 years ago
Created attachment 678734 [details] [diff] [review]
Changed the testattribute of radio button

I have changed the test attribute of radio button to 1. Please let me know if its correct.
Attachment #678337 - Attachment is obsolete: true
Attachment #678734 - Flags: feedback?(trev.saunders)
(In reply to Aishwarya from comment #9)
> Created attachment 678734 [details] [diff] [review]
> Changed the testattribute of radio button
> 
> I have changed the test attribute of radio button to 1. Please let me know
> if its correct.

does it pass the tests? if so then I suspect so otherwise not.
(Reporter)

Comment 11

5 years ago
in case if you don't know how to run a11y mochitests:

cd yourobjdir/_tests/testing/mochitest;
python runtests.py --a11y --test-path=accessible/attributes/test_obj_group.html
(then click on a test link)

to run them all together:
python runtests.py --a11y --autorun
(Reporter)

Updated

5 years ago
Attachment #678734 - Flags: feedback?(trev.saunders) → review?(trev.saunders)
Trevor do you have time to review?
(In reply to David Bolter [:davidb] from comment #12)
> Trevor do you have time to review?

uhm afaik this is blocked on a response to comment 10
(In reply to Trevor Saunders (:tbsaunde) from comment #13)
> (In reply to David Bolter [:davidb] from comment #12)
> > Trevor do you have time to review?
> 
> uhm afaik this is blocked on a response to comment 10

I see. Hmmm...
Aishwarya could you try running the tests?

From your source directory you could run (change objdir to your build dir):
make -C objdir mochitest-a11y

OR

cd objdir/_tests/testing/mochitest/
python runtests.py --a11y

Be careful not to use the keyboard or mouse during tests.
Assignee: nobody → aiishwarya.sivaraman
Flags: needinfo?(aiishwarya.sivaraman)
Alternatively one of us will run them...
Assignee: aiishwarya.sivaraman → nobody
Flags: needinfo?(aiishwarya.sivaraman)
(Reporter)

Comment 17

5 years ago
Comment on attachment 678734 [details] [diff] [review]
Changed the testattribute of radio button


>+  <form>
>+    <input type="radio" style="display: none;">
>+    <input type="radio" id="radio5">

they should have same @name, r=me with a fix.
Attachment #678734 - Flags: review?(trev.saunders) → review+
(Reporter)

Comment 18

5 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/6a7e8e9791be
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/6a7e8e9791be
Assignee: nobody → aiishwarya.sivaraman
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in before you can comment on or make changes to this bug.