role note shouldn't pick up the name from subtree

RESOLVED FIXED in mozilla22

Status

()

Core
Disability Access APIs
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: surkov, Assigned: Josh Yuan)

Tracking

(Blocks: 1 bug, {access})

unspecified
mozilla22
access
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

4 years ago
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

4 years ago
didn't found the file@
http://mxr.mozilla.org/mozilla-central/source/accessible/src/msaa/RoleMap.h
The file is at http://mxr.mozilla.org/mozilla-central/source/accessible/src/base/RoleMap.h
(Assignee)

Comment 3

4 years ago
Hi!

Can I work on this bug?  If so, where should I begin?  I've downloaded and built the firefox files already as well.
(Reporter)

Comment 4

4 years ago
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).
(Reporter)

Updated

4 years ago
Assignee: nobody → joshyyuan
(Assignee)

Comment 5

4 years ago
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"?
(Reporter)

Comment 6

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

Comment 7

4 years ago
please make sure to run a11y mochitests
cd objdir/_tests/testing/mochitests;
python runtests.py --a11y --autorun
(Assignee)

Comment 8

4 years ago
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
Attachment #717551 - Flags: feedback?(surkov.alexander)
(Reporter)

Comment 9

4 years ago
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-
(Reporter)

Comment 11

4 years ago
Josh, please address comment #10 and upload new patch.
(Assignee)

Comment 12

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

Comment 13

4 years ago
(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
(Assignee)

Comment 14

4 years ago
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!
(Assignee)

Comment 15

4 years ago
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
(Reporter)

Comment 16

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

Comment 17

4 years ago
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?
(Reporter)

Comment 18

4 years ago
(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
(Assignee)

Comment 19

4 years ago
Created attachment 721559 [details] [diff] [review]
made changes from comment #10

tried using runtests.py in testing/mochitest

I think it works?
Attachment #717551 - Attachment is obsolete: true
Attachment #721559 - Flags: feedback?(surkov.alexander)
(Reporter)

Comment 20

4 years ago
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)
(Reporter)

Comment 21

4 years ago
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.
(Assignee)

Comment 26

4 years ago
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?
(Assignee)

Comment 27

4 years ago
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..
Attachment #721559 - Attachment is obsolete: true
(Reporter)

Comment 28

4 years ago
(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
(Assignee)

Comment 29

4 years ago
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
Attachment #724308 - Attachment is obsolete: true
(Reporter)

Comment 30

4 years ago
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
(Assignee)

Comment 31

4 years ago
Created attachment 725087 [details] [diff] [review]
changed name to null

still working on getting runtests.py to work on my computer...
Attachment #724732 - Attachment is obsolete: true
(Assignee)

Comment 32

4 years ago
disregard previous patch, an error occurred and the patch didn't update correctly.  Sorry
(Assignee)

Comment 33

4 years ago
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.
(Assignee)

Comment 34

4 years ago
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
Attachment #725087 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #725255 - Flags: feedback?(surkov.alexander)
(Reporter)

Comment 35

4 years ago
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+
pushed for you
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/bd6476403edd
https://hg.mozilla.org/mozilla-central/rev/bd6476403edd
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in before you can comment on or make changes to this bug.