Last Comment Bug 833256 - role note shouldn't pick up the name from subtree
: role note shouldn't pick up the name from subtree
Status: RESOLVED FIXED
[good first bug][mentor=surkov.alexan...
: access
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla22
Assigned To: Josh Yuan
:
Mentors:
Depends on:
Blocks: htmla11y
  Show dependency treegraph
 
Reported: 2013-01-22 00:12 PST by alexander :surkov
Modified: 2013-03-18 13:16 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
first patch attempt (2.58 KB, patch)
2013-02-23 14:19 PST, Josh Yuan
tbsaunde+mozbugs: review-
surkov.alexander: feedback+
Details | Diff | Splinter Review
made changes from comment #10 (2.57 KB, patch)
2013-03-05 21:52 PST, Josh Yuan
no flags Details | Diff | Splinter Review
made changes you suggested to line 582 (2.59 KB, patch)
2013-03-13 01:07 PDT, Josh Yuan
no flags Details | Diff | Splinter Review
oops forgot to change line 215 before, fixed that (2.59 KB, patch)
2013-03-13 18:54 PDT, Josh Yuan
no flags Details | Diff | Splinter Review
changed name to null (2.59 KB, patch)
2013-03-14 13:46 PDT, Josh Yuan
no flags Details | Diff | Splinter Review
changed name to null (try #2) (2.57 KB, patch)
2013-03-14 21:45 PDT, Josh Yuan
tbsaunde+mozbugs: review+
Details | Diff | Splinter Review

Description alexander :surkov 2013-01-22 00:12:08 PST
Steve noticed:
its causing duplicate announcement of content in a JAWS beta when using
Firefox

the behavior was introduced in bug 765172 and doesn't seem motivated (see 765172 comment #10).

It seems eNameFromSubtreeIfReqRule constant should be a good choice in this case (see RoleMap.h)
Comment 1 Gaurav Pruthi 2013-01-23 04:09:14 PST
didn't found the file@
http://mxr.mozilla.org/mozilla-central/source/accessible/src/msaa/RoleMap.h
Comment 2 Hubert Figuiere [:hub] 2013-01-23 06:29:28 PST
The file is at http://mxr.mozilla.org/mozilla-central/source/accessible/src/base/RoleMap.h
Comment 3 Josh Yuan 2013-02-20 13:48:11 PST
Hi!

Can I work on this bug?  If so, where should I begin?  I've downloaded and built the firefox files already as well.
Comment 4 alexander :surkov 2013-02-20 15:51:06 PST
You need to create a patch (https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F). I'd recommend to look at the patch of bug 765172 where unwanted behavior was introduced and change that part (see this bug description).
Comment 5 Josh Yuan 2013-02-22 13:22:15 PST
Alright, so I've made change to the eNameFromSubtreeIfReqRule constant in RoleMap.h.  But my html skills aren't that great.  What kind of test case should I be writing.  I recognize the current testcases in the html file in what they do and how they work.  How do I test for these "duplicate announcements"?
Comment 6 alexander :surkov 2013-02-22 17:40:45 PST
you can add markup like <div role="note">subtree</div> and do testName() for it, expected name should be empty.
Comment 7 alexander :surkov 2013-02-22 17:41:28 PST
please make sure to run a11y mochitests
cd objdir/_tests/testing/mochitests;
python runtests.py --a11y --autorun
Comment 8 Josh Yuan 2013-02-23 14:19:53 PST
Created attachment 717551 [details] [diff] [review]
first patch attempt

This is what I have so far.  I wanted to make sure I've being doing things correctly up to this point
Comment 9 alexander :surkov 2013-02-23 17:50:31 PST
Comment on attachment 717551 [details] [diff] [review]
first patch attempt

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

f=me, thank you

passing review request to Trevor

::: accessible/tests/mochitest/name/test_general.html
@@ +209,5 @@
>        testName("img_eq", "x^2 + y^2 + z^2")
>        testName("txt_eq", "x^2 + y^2 + z^2")
>  
> +      ////////////////////////////////////////////////////////////////////////
> +      // tests for duplicate announcement of content 

nit: whitespace at the end of line
Comment 10 Trevor Saunders (:tbsaunde) 2013-02-24 01:48:57 PST
Comment on attachment 717551 [details] [diff] [review]
first patch attempt

>+      testName("test_note", "dupicate announcement?");

that's broken, the first argument to testName() is the id of the element to test, and the div you add has no id attribute.

>+  <!-- duplicate announcement -->
>+  <div role="test_note">subtree</div>

role="test_note" is broken too there is no such role it needs to be role="note"
Comment 11 alexander :surkov 2013-02-25 20:16:47 PST
Josh, please address comment #10 and upload new patch.
Comment 12 Josh Yuan 2013-02-28 01:37:40 PST
since my html isn't so good.. what should go in the second parameter of testName()?
Comment 13 alexander :surkov 2013-02-28 01:46:43 PST
(In reply to Josh Yuan from comment #12)
> since my html isn't so good.. what should go in the second parameter of
> testName()?

the text content of the div@role="note"

you need just run this test to make sure it working:
cd objdir/_test/testing/mochitest;
python runtests.py --a11y --test-path=accessible/name/test_general.html
Comment 14 Josh Yuan 2013-03-02 11:59:35 PST
just letting you know I'm still here! things got busy, but I should have time to submit my next patch to you very soon!
Comment 15 Josh Yuan 2013-03-04 22:51:11 PST
are you certain that that is the correct file path?  I'm looking at the mochitest folder right now in the file path you gave you, and I don't see a runtests.py.  Which explains the error that I get when I try to run that test
Comment 16 alexander :surkov 2013-03-05 00:47:26 PST
yes I do (with correctness: _test -> _tests). are you sure you are in objdir?
Comment 17 Josh Yuan 2013-03-05 02:46:00 PST
yes, I was in the objdir in my mozilla source code.  I did a search for runtests.py and found multiple copies scattered in other folders.  Would those work for this test case?
Comment 18 alexander :surkov 2013-03-05 03:32:35 PST
(In reply to Josh Yuan from comment #17)
> yes, I was in the objdir in my mozilla source code.  I did a search for
> runtests.py and found multiple copies scattered in other folders.

runtests.py should be in objdir/_tests/testing/mochitest (at least it's true for me)

>  Would
> those work for this test case?

I don't know
Comment 19 Josh Yuan 2013-03-05 21:52:44 PST
Created attachment 721559 [details] [diff] [review]
made changes from comment #10

tried using runtests.py in testing/mochitest

I think it works?
Comment 20 alexander :surkov 2013-03-05 21:54:08 PST
Comment on attachment 721559 [details] [diff] [review]
made changes from comment #10

you have f+ from me, you need r+ from trevor
Comment 21 alexander :surkov 2013-03-05 21:56:36 PST
Comment on attachment 721559 [details] [diff] [review]
made changes from comment #10

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

::: accessible/tests/mochitest/name/test_general.html
@@ +211,5 @@
>  
> +      ////////////////////////////////////////////////////////////////////////
> +      // tests for duplicate announcement of content
> +
> +      testName("note", "dupicate announcement?");

the first argument should 'id' attribute of the element you need to test
Comment 22 Trevor Saunders (:tbsaunde) 2013-03-06 12:10:01 PST
(In reply to Josh Yuan from comment #19)
> Created attachment 721559 [details] [diff] [review]
> made changes from comment #10
> 
> tried using runtests.py in testing/mochitest
> 
> I think it works?

I'm pretty sure it doesn't.  You didn't fix the first part of comment 10, you need <div id=test_note" role="note">subtree</div>
Comment 23 Trevor Saunders (:tbsaunde) 2013-03-06 12:15:19 PST
Comment on attachment 721559 [details] [diff] [review]
made changes from comment #10

please request review again when comment 22 is fixed, and tests actually pass.
Comment 24 Trevor Saunders (:tbsaunde) 2013-03-06 12:18:38 PST
(In reply to Trevor Saunders (:tbsaunde) from comment #22)
> (In reply to Josh Yuan from comment #19)
> > Created attachment 721559 [details] [diff] [review]
> > made changes from comment #10
> > 
> > tried using runtests.py in testing/mochitest
> > 
> > I think it works?
> 
> I'm pretty sure it doesn't.  You didn't fix the first part of comment 10,

actually I might be wrong about tests failing testName() is crazy and if you give it a garbage id it'll just fail to get an accessible and return.
Comment 25 Trevor Saunders (:tbsaunde) 2013-03-06 12:20:59 PST
> actually I might be wrong about tests failing testName() is crazy and if you
> give it a garbage id it'll just fail to get an accessible and return.

I should learn to completely check what I'm saying before spewing garbage, tests will fail because getAccessible() calls ok(false) if you give it a bad id.
Comment 26 Josh Yuan 2013-03-13 01:04:53 PDT
I'm getting this error:

Traceback (most recent call last):
  File "runtests.py", line 25, in <module>
    from automation import Automation
ImportError: No module named automation

Any ideas where this could stem from?  I'm double checking the python files and things seem to check out.  Is it possible that I may have had an error occur during the source code retrieval and that I some data may be missing?
Comment 27 Josh Yuan 2013-03-13 01:07:26 PDT
Created attachment 724308 [details] [diff] [review]
made changes you suggested to line 582

in the mean time, here's a patch for the changes suggested in comment #22.  I'm having issues getting the test to work though..
Comment 28 alexander :surkov 2013-03-13 01:29:37 PDT
(In reply to Josh Yuan from comment #26)
> Is it possible that I may have had an error
> occur during the source code retrieval and that I some data may be missing?

I think you would be told if hg clone failed. was the build successful?

(In reply to Josh Yuan from comment #27)

> in the mean time, here's a patch for the changes suggested in comment #22. 
> I'm having issues getting the test to work though..

it's not enough, see comment #21
Comment 29 Josh Yuan 2013-03-13 18:54:44 PDT
Created attachment 724732 [details] [diff] [review]
oops forgot to change line 215 before, fixed that

I'm pulling for an update now just to make sure I have everything.  I'm hoping this will make things work
Comment 30 alexander :surkov 2013-03-13 23:05:45 PDT
Comment on attachment 724732 [details] [diff] [review]
oops forgot to change line 215 before, fixed that

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

::: accessible/tests/mochitest/name/test_general.html
@@ +211,5 @@
>  
> +      ////////////////////////////////////////////////////////////////////////
> +      // tests for duplicate announcement of content
> +
> +      testName("test_note", "dupicate announcement?");

I think name should be null in this case
Comment 31 Josh Yuan 2013-03-14 13:46:47 PDT
Created attachment 725087 [details] [diff] [review]
changed name to null

still working on getting runtests.py to work on my computer...
Comment 32 Josh Yuan 2013-03-14 13:47:39 PDT
disregard previous patch, an error occurred and the patch didn't update correctly.  Sorry
Comment 33 Josh Yuan 2013-03-14 21:41:41 PDT
Something went wrong with my msvc command line after I attempted to update the source code.  It stalls whenever I try to create a new patch.  Unless you have any ideas to the issue(s), I will likely be re-building firefox source code overnight.
Comment 34 Josh Yuan 2013-03-14 21:45:29 PDT
Created attachment 725255 [details] [diff] [review]
changed name to null (try #2)

it was weird.  I was waiting on the patch to update this whole time, and after 30 minutes, it just kind of worked
Comment 35 alexander :surkov 2013-03-14 23:19:13 PDT
Comment on attachment 725255 [details] [diff] [review]
changed name to null (try #2)

you need to get r+
Comment 36 Trevor Saunders (:tbsaunde) 2013-03-17 19:05:49 PDT
Comment on attachment 725255 [details] [diff] [review]
changed name to null (try #2)

ok, I ran the test for you seems to work so r=me
Comment 37 Trevor Saunders (:tbsaunde) 2013-03-17 19:13:30 PDT
pushed for you
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/bd6476403edd
Comment 38 Ed Morley [:emorley] 2013-03-18 13:16:50 PDT
https://hg.mozilla.org/mozilla-central/rev/bd6476403edd

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