Closed Bug 833256 Opened 12 years ago Closed 11 years ago

role note shouldn't pick up the name from subtree

Categories

(Core :: Disability Access APIs, defect)

Product:
Core
Component:
Disability Access APIs
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla22

People

(Reporter: surkov, Assigned: joshyyuan)

References

(Blocks 1 open bug)

Details

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

Attachments

(1 file, 5 obsolete files)

changed name to null (try #2)
2.57 KB, patch
tbsaunde
: review+
Details | Diff | Splinter Review 
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)
Hi!

Can I work on this bug?  If so, where should I begin?  I've downloaded and built the firefox files already as well.
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).
Assignee: nobody → joshyyuan
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"?
you can add markup like <div role="note">subtree</div> and do testName() for it, expected name should be empty.
please make sure to run a11y mochitests
cd objdir/_tests/testing/mochitests;
python runtests.py --a11y --autorun
Attached patch first patch attempt (obsolete) — — Splinter Review 
This is what I have so far.  I wanted to make sure I've being doing things correctly up to this point
Attachment #717551 - Flags: feedback?(surkov.alexander)
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
Attachment #717551 - Flags: review?(trev.saunders)
Attachment #717551 - Flags: feedback?(surkov.alexander)
Attachment #717551 - Flags: feedback+
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"
Attachment #717551 - Flags: review?(trev.saunders) → review-
Josh, please address comment #10 and upload new patch.
since my html isn't so good.. what should go in the second parameter of testName()?
(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
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!
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
yes I do (with correctness: _test -> _tests). are you sure you are in objdir?
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?
(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
Attached patch made changes from comment #10 (obsolete) — — Splinter Review 
tried using runtests.py in testing/mochitest

I think it works?
Attachment #717551 - Attachment is obsolete: true
Attachment #721559 - Flags: feedback?(surkov.alexander)
Comment on attachment 721559 [details] [diff] [review]
made changes from comment #10

you have f+ from me, you need r+ from trevor
Attachment #721559 - Flags: feedback?(surkov.alexander) → review?(trev.saunders)
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
(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 on attachment 721559 [details] [diff] [review]
made changes from comment #10

please request review again when comment 22 is fixed, and tests actually pass.
Attachment #721559 - Flags: review?(trev.saunders)
(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.
> 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.
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?
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..
Attachment #721559 - Attachment is obsolete: true
(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
I'm pulling for an update now just to make sure I have everything.  I'm hoping this will make things work
Attachment #724308 - Attachment is obsolete: true
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
Attached patch changed name to null (obsolete) — — Splinter Review 
still working on getting runtests.py to work on my computer...
Attachment #724732 - Attachment is obsolete: true
disregard previous patch, an error occurred and the patch didn't update correctly.  Sorry
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.
it was weird.  I was waiting on the patch to update this whole time, and after 30 minutes, it just kind of worked
Attachment #725087 - Attachment is obsolete: true
Attachment #725255 - Flags: feedback?(surkov.alexander)
Comment on attachment 725255 [details] [diff] [review]
changed name to null (try #2)

you need to get r+
Attachment #725255 - Flags: feedback?(surkov.alexander) → review?(trev.saunders)
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
Attachment #725255 - Flags: review?(trev.saunders) → review+
https://hg.mozilla.org/mozilla-central/rev/bd6476403edd
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: