Last Comment Bug 794620 - group position of HTML radio buttons might be incorrect
: group position of HTML radio buttons might be incorrect
Status: RESOLVED FIXED
[good first bug][mentor=surkov.alexan...
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla22
Assigned To: Aishwarya
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-09-26 14:13 PDT by alexander :surkov
Modified: 2013-02-22 03:17 PST (History)
5 users (show)
surkov.alexander: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (1.03 KB, patch)
2012-09-26 19:18 PDT, Aishwarya
no flags Details | Diff | Splinter Review
Proposed patch - Added test to radio5 and changed the calling function to count (1.41 KB, patch)
2012-11-04 07:04 PST, Aishwarya
surkov.alexander: feedback+
Details | Diff | Splinter Review
Fixed white space and moved check into if condition (1.46 KB, patch)
2012-11-05 08:47 PST, Aishwarya
tbsaunde+mozbugs: review-
Details | Diff | Splinter Review
Changed the testattribute of radio button (1.45 KB, patch)
2012-11-06 06:42 PST, Aishwarya
surkov.alexander: review+
Details | Diff | Splinter Review

Description alexander :surkov 2012-09-26 14:13:54 PDT
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 2012-09-26 19:18:25 PDT
Created attachment 665235 [details] [diff] [review]
Patch

I have checked for accessibility before incrementing.
Comment 2 alexander :surkov 2012-09-26 20:16:00 PDT
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 2012-10-26 15:16:13 PDT
Aishwarya, are you working on it? I'd be great to finish it. Thank you.
Comment 4 Aishwarya 2012-11-04 07:04:47 PST
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?
Comment 5 alexander :surkov 2012-11-04 17:34:35 PST
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 2012-11-05 08:47:38 PST
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! =)
Comment 7 Trevor Saunders (:tbsaunde) 2012-11-05 12:01:26 PST
(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) 2012-11-05 12:03:29 PST
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 2012-11-06 06:42:55 PST
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.
Comment 10 Trevor Saunders (:tbsaunde) 2012-11-06 15:34:38 PST
(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 2012-11-06 17:36:34 PST
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] ***PTO until 29th*** 2012-12-20 10:36:46 PST
Trevor do you have time to review?
Comment 13 Trevor Saunders (:tbsaunde) 2012-12-20 10:56:39 PST
(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] ***PTO until 29th*** 2012-12-20 10:58:40 PST
(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] ***PTO until 29th*** 2012-12-20 11:01:00 PST
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] ***PTO until 29th*** 2012-12-20 11:01:46 PST
Alternatively one of us will run them...
Comment 17 alexander :surkov 2013-02-21 08:27:21 PST
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 19 Ryan VanderMeulen [:RyanVM] 2013-02-22 03:17:42 PST
https://hg.mozilla.org/mozilla-central/rev/6a7e8e9791be

Note You need to log in before you can comment on or make changes to this bug.