833256 - role note shouldn't pick up the name from subtree

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

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]

didn't found the file@
http://mxr.mozilla.org/mozilla-central/source/accessible/src/msaa/RoleMap.h

Comment 2

[Hubert Figuiere [:hub]]

The file is at http://mxr.mozilla.org/mozilla-central/source/accessible/src/base/RoleMap.h

Comment 3

[Josh Yuan]

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 (:asurkov)]

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]

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 (:asurkov)]

you can add markup like <div role="note">subtree</div> and do testName() for it, expected name should be empty.

Comment 7

[alexander :surkov (:asurkov)]

please make sure to run a11y mochitests
cd objdir/_tests/testing/mochitests;
python runtests.py --a11y --autorun

Comment 8

[Josh Yuan]

Attachment: 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 (:asurkov)]

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)]

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 (:asurkov)]

Josh, please address comment #10 and upload new patch.

Comment 12

[Josh Yuan]

since my html isn't so good.. what should go in the second parameter of testName()?

Comment 13

[alexander :surkov (:asurkov)]

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

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]

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 (:asurkov)]

yes I do (with correctness: _test -> _tests). are you sure you are in objdir?

Comment 17

[Josh Yuan]

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 (:asurkov)]

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

Attachment: made changes from comment #10

tried using runtests.py in testing/mochitest

I think it works?

Comment 20

[alexander :surkov (:asurkov)]

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 (:asurkov)]

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)]

(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)]

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)]

(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)]

> 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]

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]

Attachment: 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 (:asurkov)]

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

Attachment: 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 (:asurkov)]

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]

Attachment: changed name to null

still working on getting runtests.py to work on my computer...

Comment 32

[Josh Yuan]

disregard previous patch, an error occurred and the patch didn't update correctly.  Sorry

Comment 33

[Josh Yuan]

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]

Attachment: 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 (:asurkov)]

Comment on attachment 725255 [details] [diff] [review]
changed name to null (try #2)

you need to get r+

Comment 36

[Trevor Saunders (:tbsaunde)]

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)]

pushed for you
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/bd6476403edd

Comment 38

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/bd6476403edd