794620 - group position of HTML radio buttons might be incorrect

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

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>

Comment 1

[Aishwarya]

Attachment: Patch

I have checked for accessibility before incrementing.

Comment 2

[alexander :surkov (:asurkov)]

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"

Comment 3

[alexander :surkov (:asurkov)]

Aishwarya, are you working on it? I'd be great to finish it. Thank you.

Comment 4

[Aishwarya]

Attachment: 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?

Comment 5

[alexander :surkov (:asurkov)]

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

Comment 6

[Aishwarya]

Attachment: Fixed white space and moved check into if condition

Please let me know if anything else needs to be changed. Thanks! =)

Comment 7

[Trevor Saunders (:tbsaunde)]

(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 8

[Trevor Saunders (:tbsaunde)]

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

Comment 9

[Aishwarya]

Attachment: Changed the testattribute of radio button

I have changed the test attribute of radio button to 1. Please let me know if its correct.

Comment 10

[Trevor Saunders (:tbsaunde)]

(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.

Comment 11

[alexander :surkov (:asurkov)]

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

Comment 12

[David Bolter [:davidb] (NeedInfo me for attention)]

Trevor do you have time to review?

Comment 13

[Trevor Saunders (:tbsaunde)]

(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

Comment 14

[David Bolter [:davidb] (NeedInfo me for attention)]

(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...

Comment 15

[David Bolter [:davidb] (NeedInfo me for attention)]

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.

Comment 16

[David Bolter [:davidb] (NeedInfo me for attention)]

Alternatively one of us will run them...

Comment 17

[alexander :surkov (:asurkov)]

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.

Comment 18

[alexander :surkov (:asurkov)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/6a7e8e9791be

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6a7e8e9791be