Closed Bug 467387 Opened 16 years ago Closed 15 years ago

Expose non-editable documents as readonly, regardless of role

Categories

(Core :: Disability Access APIs, defect)

Product:
Core
Component:
Disability Access APIs
Platform:
x86
Windows XP
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.9.1b4

People

(Reporter: aaronlev, Assigned: davidb)

References

(Blocks 1 open bug)

Details

(Keywords: access, verified1.9.1)

Attachments

(2 files, 11 obsolete files)

merged some tests
14.79 KB, patch
surkov
: review+
MarcoZ
: review+
Details | Diff | Splinter Review
patch to push
14.69 KB, patch
beltzner
: approval1.9.1+
Details | Diff | Splinter Review 
For http://codetalks.org/source/landmarks/test-all-minimal.html

We have <body role="article">
It should still be READONLY, since for historical reasons the nsDocAccessible needs to be marked READONLY to show that it's not an editable document.

This is messing up the JAWS 10 landmark implementation for this testcase.
Flags: blocking1.9.2?
Assignee: nobody → david.bolter
Attached patch passing test = strange? (obsolete) — — Splinter Review 
Aaron, I tried to create a failing mochitest for this bug by adding role="article" to the body tag in test_nsIAccessibleDocument.html but it doesn't seem to break the nsIAccessible readonly state.

I'm not currently building on Windows and so will need help to pinpoint this...
Comment on attachment 358000 [details] [diff] [review]
passing test = strange?

Hi Marco, can you think of a way to write a mochitest for this bug?
Attachment #358000 - Flags: review?(marco.zehe)
Slightly better would have been to use 
if (Role(this) != nsIAccessibleRole::ROLE_DOCUMENT)

But actually, even better than that -- since you know you have an mRoleMapEntry, you can just check to see
if (mRoleMapEntry->role != nsIAccessibleRole::ROLE_DOCUMENT)
Can it be because of getAriaState implementation by nsDocAccessible?
Comment on attachment 358000 [details] [diff] [review]
passing test = strange?

1. The test file you used is used to test something else. Please create a new test file for this test.
2. As Aaron said, since you already know that you have a roleMapEntry, use that instead of querying for the role.
3. Also check the impl of nsDocAccessible::getState to see if that overrides things in an undesired fashion, and if the fix is more appropriate there.
Attachment #358000 - Flags: review?(marco.zehe) → review-
is there anything here we need to fix?
(In reply to comment #5)
> (From update of attachment 358000 [details] [diff] [review])
> 1. The test file you used is used to test something else. Please create a new
> test file for this test.

Yep, was hoping someone else would :)

> 2. As Aaron said, since you already know that you have a roleMapEntry, use that
> instead of querying for the role.

Will address.

> 3. Also check the impl of nsDocAccessible::getState to see if that overrides
> things in an undesired fashion, and if the fix is more appropriate there.

Looking
(In reply to comment #4)
> Can it be because of getAriaState implementation by nsDocAccessible?

My patch changes nsAccessible::GetARIAState(aState) which is called from nsDocAccessible's GetAriaState.

I'll take another look, but I really need to set up my windoze env I guess :(
Attached patch now check in doc accessible (obsolete) — — Splinter Review 

        Attachment #358000 -
        Attachment is obsolete: true

        Attachment #359316 -
        Flags: review?(marco.zehe)

    
    

    
  
Comment on attachment 359316 [details] [diff] [review]
now check in doc accessible

Do we get an editor also for form fields like textareas within the page? In that case, we wouldn't want the doc to not be read-only.

        Attachment #359316 -
        Flags: review?(surkov.alexander)

    
    

    
  
(In reply to comment #10)
> (From update of attachment 359316 [details] [diff] [review])
> Do we get an editor also for form fields like textareas within the page? In
> that case, we wouldn't want the doc to not be read-only.

I'll write some tests.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        added test to comfort Marco (obsolete)
        — 
          — Splinter Review
      

    


  
We need someone to test that this fixes the problem.

        Attachment #359316 -
        Attachment is obsolete: true

        Attachment #359767 -
        Flags: review?(marco.zehe)

        Attachment #359316 -
        Flags: review?(surkov.alexander)

        Attachment #359316 -
        Flags: review?(marco.zehe)

    
    

    
  
I don't think it's good. Believe it or not, you can have a doc accessible that has a widget role. This would be for a mashup widget.

This example does that:
http://www.mozilla.org/access/dhtml/listbox-owner.htm

Would this code cause the inner listbox to be readonly?

    
    

    
  
(In reply to comment #13)
No I don't think so and I actually have an improved patch locally, but I have some questions.

1. For the main document do we want to force READONLY no matter what?
2. For frame documents we let ARIA states be (no readonly override)?

Maybe the right fix is to have landmarks be READONLY?  Or to file against Jaws?

        Attachment #359767 -
        Flags: review?(marco.zehe)

    
    

    
  
The order of execution is that an nsAccessible GetState call will call (virtual methods): GetStateInternal, and then GetARIAState. In the case of a document, nsDocAccessible GetStateInternal calls nsAccessible::GetState, and currently already looks for an editor, setting READONLY only if there isn't one. This I didn't realize until recently, and ideally we don't want to check for an editor twice. The nsDocAccessible GetARIAState, similarly calls nsAccessible::GetARIAState, and then this funny bit where it checks its parent for an nsPIAccessible interface which apparently indicates it is a frame or iframe etc. If the parent has this interface then the parent's GetARIAState is called as the final state override. I'm not sure what this parent is.

    
    

    
  
Aaron, I'm guessing that if the parent of the frame with the nsPIAccessible interface is the main document, then you are correct, my third patch would cause all frame docs to be READONLY.

If my assumptions are correct, and we need this fix for Jaws users then I'll need to go back to a variation of my first patch (using aaron's advice from comment 3).

    
    

    
  
I'm adding bug 461922 as a dependancy since we might change the way we think about this fix if that work goes ahead.
Depends on: 461922

    
    

    
  
David, can you add a test to comfort me ;) because I really don't understand what you want to fix - every patch has different code and different mochitests

    
    

    
  
Just fix for roleMapEntry->role == ROLE_DOC

We'll see what happens with iframe mashup widgets and JAWS after that.

    
    

    
  
(In reply to comment #19)
> Just fix for roleMapEntry->role == ROLE_DOC
> 
> We'll see what happens with iframe mashup widgets and JAWS after that.

Aaron, I've tried putting various aria roles on a body element and we keep getting nsIAccessible:STATE_READONLY for the nsIAccessibleDocument. (see my comment 1)

Recommending closing as invalid.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        needs testing (obsolete)
        — 
          — Splinter Review
      

    


  
I think this is what Aaron is looking for but someone with windows needs to check that before this patch the document reported by Aaron is not readonly, and with the patch, the document is readonly (using accexplorer or dom inspector).

        Attachment #359767 -
        Attachment is obsolete: true

        Attachment #360099 -
        Flags: review?(aaronleventhal)

    
  

        Attachment #360099 -
        Flags: review?(aaronleventhal) → review+

    
    

    
  
This fixes the problem in JAWS, at least as long as you don't navigate into the application part, in which case JAWS will again turn on application mode and there's no way to turn it off again. But that's then not our problem I believe.

    
    

    
  
(In reply to comment #22)
> This fixes the problem in JAWS, at least as long as you don't navigate into the
> application part, in which case JAWS will again turn on application mode and
> there's no way to turn it off again. But that's then not our problem I believe.

Thanks Marco. So we have this sort of bad-tasting fix... do you think we should push it?

    
    

    
  
(In reply to comment #23)

> Thanks Marco. So we have this sort of bad-tasting fix... do you think we should
> push it?

Please don't push it until we have mochitest or you point msaa specific code proving it's impossible to create mochitests.

    
    

    
  
Is it also a fix if you just add STATE_READONLY to the states in the nsARIAMap entries for "article" and "document"?

kNoReqStates -> nsIAccessible::STATE_READONLY

That would be cleaner/clearer if it works.

    
    

    
  
I just attempted to transform the test page linked to in comment #0 into a Mochitest, and it indeed does not show the problem:
1. The state is READONLY, as expected.
2. Opening the test page within the Chrome test harness, not running all tests but only this one, JAWS correctly turns on virtual cursor.

This is the test file with just the Mochitest bits added, and a call to test the role and one to test the states.

    
    

    
  
I confirmed with Marco over IRC that we can't seem to create a failing mochitest to capture this bug. I haven't found anything in the msaa code that would be causing this.

Is it possible that ISimpleDOM is at play here?

    
    

    
  
Sounds like timing issue. I put role="article" on body and sometimes I get readonly state, sometimes I don't. So technically proposed patch fixes the bug but it didn't address real problem I think.

    
    

    
  
Timing bugs are painful. I wonder if we can find out what event Jaws uses to trigger its queries. If it is an object show event, I think we have a few other timing bugs related to that.

    
    

    
  


  
I like Aaron's suggestion best, if it works. Can someone try it?

        Attachment #360545 -
        Flags: review?(marco.zehe)

        Attachment #360545 -
        Flags: review?(surkov.alexander)

    
    

    
  
Comment on attachment 360545 [details] [diff] [review]
Using Aaron's suggestion from comment 25 (needs manual testing)

Surkov, can you try this out? I'd like to know if this works and/or affects the timing.

    
    

    
  
It should expose editable document as readonly. Do we want this?

    
    

    
  
(In reply to comment #32)
> It should expose editable document as readonly. Do we want this?

We don't want that.

Let's think. Right now anytime we put a role on the body element, we make sure that we don't expose a READONLY state. So a screen reader might (Jaws will) turn off the virtual cursor. We might want Jaws to turn on the virtual cursor if the role is article or document, except in the case that it is an editable article or document.

With my latest patch, for an article or document that has an associated editor (ie is editable) I think we expose READONLY, and ext state EDITABLE :)  This is surely nutty.

Patch coming.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        WIP - for discussion (obsolete)
        — 
          — Splinter Review
      

    


  
This builds on the previous patch but allows the existence of an associated editor to clear the READONLY bitflag. I could alternatively have changed GetARIAState to accept the extended state as a second [in] param, and then only clear the READONLY bit if there is no ext state EDITABLE -- but I think this approach is simpler.

Thoughts?

        Attachment #360545 -
        Attachment is obsolete: true

        Attachment #360780 -
        Flags: review?(surkov.alexander)

        Attachment #360545 -
        Flags: review?(surkov.alexander)

        Attachment #360545 -
        Flags: review?(marco.zehe)

    
    

    
  
I should clarify that when I refer to the "existence of an associated editor" above, that is captured in my check of the extra state (see patch) which is already set by the document's GetStateInternal.

    
    

    
  
David, please keep in mind dual bug 434707. The code we should patch is a bit complicated and it's not always very easy to think "but what if". David, could we dance from mochitests? They should include ARIA roles on body, editable documents, subdocuments. So we will see where we fail and what should we fix.

    
    

    
  
Thanks for alerting me to the related bug. I will add tests here for sure, as long as you don't see problems with my approach?

    
    

    
  
(In reply to comment #37)
> Thanks for alerting me to the related bug. I will add tests here for sure, as
> long as you don't see problems with my approach?

Ok, I didn't look deeply but this should work, just one thing I can't say I'm happy with setting/unsetting the same property few times.

    
    

    
  
(In reply to comment #38)
> (In reply to comment #37)
> > Thanks for alerting me to the related bug. I will add tests here for sure, as
> > long as you don't see problems with my approach?
> 
> Ok, I didn't look deeply but this should work, just one thing I can't say I'm
> happy with setting/unsetting the same property few times.

I'm not happy with that either. It usually signifies the need to refactor/redesign. I think this the best small step though, until we can take a longer crack at some redesign.

    
    

    
  
if meantime you wanted to provide correct behaviour and fix it by mochitests then I think it's ok.
Summary: A role on the <body> should not affect READONLY state → Expose non-editable documents as readonly, regardless of role

    
    

    
  

      
      
      
      

        Attached patch
          
          
        added tests (obsolete)
        — 
          — Splinter Review
      

    


  
I think this is the behavior we want, tests included.

        Attachment #360099 -
        Attachment is obsolete: true

        Attachment #360780 -
        Attachment is obsolete: true

        Attachment #361282 -
        Flags: review?(surkov.alexander)

        Attachment #360780 -
        Flags: review?(surkov.alexander)

    
    

    
  
As well I'd like to see document state tests

<html>
  <body>
  </body>
</html>

<html>
  <body role="article">
  </body>
</html>

<html>
  <head>
     <script>
        document.designMode="on";
     </script>
  </head>
  <body> + plus one test with ARIA role
  </body>
</html>

<html>
  <body>
     <iframe src="url.html" role="article">
   </body>
</html>

So, variations like these.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        even more tests. (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #361282 -
        Attachment is obsolete: true

        Attachment #361305 -
        Flags: review?(surkov.alexander)

        Attachment #361282 -
        Flags: review?(surkov.alexander)

    
    

    
  
There are really two changes here:

1. Make sure we don't expose STATE_READONLY and EXT_STATE_EDITABLE at the same time. (EXT_STATE_EDITABLE wins)
2. Have article and document default to STATE_READONLY.

Oh and adding lots of tests :)
Status: NEW → ASSIGNED

    
    

    
  
Comment on attachment 361305 [details] [diff] [review]
even more tests.


>-    // Once DHTML role is used, we're only readonly if DHTML readonly used
>+    // Once an ARIA role is used, default to not-readonly. This can be overridden
>+    // by aria-readonly or if the node's nsIContent is editable.
>     *aState &= ~nsIAccessibleStates::STATE_READONLY;

"if the node's nsIContent is editable." - readonly bit removing is not overridden if editable case. Better "This can be overriden by aria-readonly attribute or if ARIA role has readonly bit by default (for example, role="article")" - something like sounds more correct.

Below, where you have *aState |= mRoleMap->state; I'd like to see comment that readonly property can be overriden in editable case.

>+		test_nsIAccessible_designmodedocument.html \
>+		test_nsIAccessible_designmodedocumentarticle.html \
> 		test_nsIAccessible_editablebody.html \
> 		test_nsIAccessible_editabledoc.html \
>+		test_nsIAccessible_framerolestates.html \
>+		test_nsIAccessible_readonlybody.html \
>+		test_nsIAccessible_readonlybodyarticle.html \

It's better to call them test_states_xxx I guess because we test states and nsIAccessible is excess here to mention.

>+  <script type="application/javascript">
>+    function doTest()
>+    {
>+      document.designMode = "on";
>+      
>+      var docAcc = getAccessible(document, [nsIAccessibleDocument]);
>+      if (docAcc) {
>+        testStates(docAcc, 0, EXT_STATE_EDITABLE, STATE_READONLY);

you could combine few tests into one if you will use JS here like

designMode on/off, add/remove aria role from body. But
1) dynamic testing is always good because it tends to reveal unexpected problems usually due to document is changed
2) static teststing is also very good because it tends to reveal unexpected problems related due to document is just loaded or not loaded :)

It's up to you I think, though dynamic tests are more compact. I like your tests expecting iframes part. There I meant to test not iframe element but document inside iframe element. You could create tests with iframes that contains tests you wrote without iframes.

        Attachment #361305 -
        Flags: review?(surkov.alexander)

    
    

    
  
David, do I get right bug 434707 is fixed by this bug as well?

    
    

    
  
(In reply to comment #46)
> David, do I get right bug 434707 is fixed by this bug as well?

Yes, looks like it.

    
    

    
  
(In reply to comment #45)
> (From update of attachment 361305 [details] [diff] [review])
> 
> >-    // Once DHTML role is used, we're only readonly if DHTML readonly used
> >+    // Once an ARIA role is used, default to not-readonly. This can be overridden
> >+    // by aria-readonly or if the node's nsIContent is editable.
> >     *aState &= ~nsIAccessibleStates::STATE_READONLY;
> 
> "if the node's nsIContent is editable." - readonly bit removing is not
> overridden if editable case. Better "This can be overriden by aria-readonly
> attribute or if ARIA role has readonly bit by default (for example,
> role="article")" - something like sounds more correct.
> 

Well, we are removing the readonly state bit here. So "overriding" in this context means adding it back later, which currently happens if the nsIContent is editable. I think the proposed comment in the patch is more correct.

    
    

    
  
(In reply to comment #45)
> (From update of attachment 361305 [details] [diff] [review])

> Below, where you have *aState |= mRoleMap->state; I'd like to see comment that
> readonly property can be overriden in editable case.

My personal style is not to put to many comments that refer to other parts of the code because the comments are hard to maintain and can become lies :)

    
    

    
  


  
Surkov, I like the new test file names, and the addition of a couple of dynamic designMode tests. My thinking is that this is decent coverage for this bug. I added comments as per your suggestions.

        Attachment #361305 -
        Attachment is obsolete: true

        Attachment #362042 -
        Flags: review?(surkov.alexander)

    
    

    
  
(In reply to comment #48)

> Well, we are removing the readonly state bit here. So "overriding" in this
> context means adding it back later, which currently happens if the nsIContent
> is editable. I think the proposed comment in the patch is more correct.

So we add readonly state bit if content is editable?

(In reply to comment #49)
> My personal style is not to put to many comments that refer to other parts of
> the code because the comments are hard to maintain and can become lies :)

Yes but on another hand if you don't change related comment then it means something wrong happens and you can't be sure your fix is correct.

    
    

    
  
Comment on attachment 362042 [details] [diff] [review]
patch, tweaked tests, filenames, and comments


> 
>   *aState |= mRoleMapEntry->state;
>+  // Note: the readonly bitflag will be overridden later content is editable

readonly bitflag (not aria-readonly) we set above, so move this comment on line above.

>diff --git a/accessible/tests/mochitest/test_nsIAccessible_editablebody.html b/accessible/tests/mochitest/test_nsIAccessible_editablebody.html

it's worth to rename it to test_states_ while you are here

>-      testStates(document, 0, EXT_STATE_EDITABLE);
>+      testStates(document, 0, EXT_STATE_EDITABLE, STATE_READONLY);

it's not necessary change because testStates does it automatically. It check states on integrity. Below the same.

> 
>+      // 467387

possibly "fixed in bug" because onetime I noticed I thought about comments like this that is a bug we need to address yet.

>+      var docAcc = getAccessible(document, [nsIAccessibleDocument]);
>+      if (docAcc) {

personally I would prefer
if (!docAcc) {
  SimpleTest.finish();
  return;l
}

note you don't call SimpleTest.finish() if there is no document accessible.

as well note you can combine this test with test_nsIAccessible_editabledoc.html

>diff --git a/accessible/tests/mochitest/test_states_designmodedocumentarticle.html b/accessible/tests/mochitest/test_states_designmodedocumentarticle.html

very long file name, possible /test_states_editabledoc_ariarole.html or something, at least I guess we need to save "editabledoc" because we used this or at least we need to keep names in sync.

>diff --git a/accessible/tests/mochitest/test_states_framerole.html b/accessible/tests/mochitest/test_states_framerole.html

still don't get why do we need to test accessible iframe element here. It's not related with document accessibles. It's just container for document accessibles iirc.

>diff --git a/accessible/tests/mochitest/test_states_readonlybody.html b/accessible/tests/mochitest/test_states_readonlybody.html

don't you have this test above already?

>diff --git a/accessible/tests/mochitest/test_states_readonlybodyarticle.html b/accessible/tests/mochitest
/test_states_readonlybodyarticle.html

and this one? Do you want to test these stuffs without dynamic things?

I still don't see tests for documents inside iframes.

        Attachment #362042 -
        Flags: review?(surkov.alexander)

    
    

    
  
(In reply to comment #51)
> (In reply to comment #48)
> 
> > Well, we are removing the readonly state bit here. So "overriding" in this
> > context means adding it back later, which currently happens if the nsIContent
> > is editable. I think the proposed comment in the patch is more correct.
> 
> So we add readonly state bit if content is editable?

I'll clarify/fix the comment in the next patch thanks.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        better patch. (obsolete)
        — 
          — Splinter Review
      

    


  
I've condensed the tests a bit. I've added a TODO in the frames test because I'm getting an uncaught exception for that case. I'll investigate further; it may require a spin off bug.

        Attachment #362042 -
        Attachment is obsolete: true

    
    

    
  
Note if you uncomment the exception causing test, you have to reverse the order of the statements (sorry - last minute cut and paste error).

    
    

    
  

      
      
      
      

        Attached patch
          
          
        proposed fix. (obsolete)
        — 
          — Splinter Review
      

    


  
All tests pass for me on OSX.

        Attachment #362245 -
        Attachment is obsolete: true

        Attachment #362273 -
        Flags: review?(surkov.alexander)

        Attachment #362273 -
        Flags: review?(marco.zehe)

        Attachment #362273 -
        Attachment is patch: true

        Attachment #362273 -
        Attachment mime type: application/octet-stream → text/plain

        Attachment #362273 -
        Flags: review?(surkov.alexander)

    
    

    
  
Comment on attachment 362273 [details] [diff] [review]
proposed fix.


>+  // Note: the readonly bitflag will be overridden later content is editable

nit: that's possibly correct english but 'if' is missed for my taste :)

>+		z_states_frame.html \
>+		z_states_framearticle.html \
>+		z_states_framecheckbox.html \
>+		z_states_frametextbox.html \

nit: a bit strange naming ("z" prefix)

>+  <a target="_blank"
>+     href="https://bugzilla.mozilla.org/show_bug.cgi?id=467387"
>+     title="Expose non-editable documents as readonly, regardless of role">
>+    Mozilla Bug 467387
>+  </a>

>+

sure it's worth to mention bug here for new line changes ;)

>   <script type="application/javascript">
>     function doTest()
>     {
>+      testStates(document, STATE_READONLY);
>+
>       document.designMode = "on";
> 
>       testStates(document, 0, EXT_STATE_EDITABLE);
>       testStates("p", 0, EXT_STATE_EDITABLE);

please add tests for "document", "article" and etc) here (when designMode is on)

>+
>+      document.designMode = "off";
>+
>+      testStates(document, STATE_READONLY);

please add test for "p" here.

>+  <title>nsIAccessible document testing</title>

nit: states of document accessible testing

>+
>+      var docAcc = getAccessible(document, [nsIAccessibleDocument]);
>+      if (docAcc) {
>+        document.designMode = "on";
>+
>+        testStates(docAcc, 0, EXT_STATE_EDITABLE, STATE_READONLY);

nit: it doesn't make to pass STATE_READONLY.

>+  
>+        document.designMode = "off";
>+
>+        testStates(docAcc, STATE_READONLY);
>+      }
>+      SimpleTest.finish();

nit: SimpleTest.finish() isn't called if there is no document accessible.

>+      frameDoc.designMode = "on";
>+      testStates(frameDoc,  0, EXT_STATE_EDITABLE);      

nit: remove all whitespaces in the end of lines.

I like tests you are going to put here. But something stopped you? Btw, it's nice to have test when iframe element has ARIA role.

>+      
>+      window.frames[""]

oops?

>+++ b/accessible/tests/mochitest/test_states_readonlybody.html

test_states_readonlydoc ?

>+++ b/accessible/tests/mochitest/test_states_readonlybodyarticle.html

the same.

>+</head>
>+<body role="checkbox">

textbox

several nits and questions, so expecting new patch.

    
    

    
  
(In reply to comment #55)
> Note if you uncomment the exception causing test, you have to reverse the order
> of the statements (sorry - last minute cut and paste error).

Sorry I noticed nothing to uncomment, can you give more details, what kind of exception do you get?

    
    

    
  
(In reply to comment #58)
> (In reply to comment #55)
> > Note if you uncomment the exception causing test, you have to reverse the order
> > of the statements (sorry - last minute cut and paste error).
> 
> Sorry I noticed nothing to uncomment, can you give more details, what kind of
> exception do you get?

That comment referred to an earlier patch (now a bug filed as bug 478810).

        Attachment #362273 -
        Flags: review?(marco.zehe)

    
    

    
  


  
(In reply to comment #57)
> (From update of attachment 362273 [details] [diff] [review])

Thanks Surkov, I address all points here. Note I have merged/renamed some tests for simplicity. I hope you like it.

> 
> >+  // Note: the readonly bitflag will be overridden later content is editable
> 
> nit: that's possibly correct english but 'if' is missed for my taste :)

Mine too. Fixed :)

> 
> >+		z_states_frame.html \
> >+		z_states_framearticle.html \
> >+		z_states_framecheckbox.html \
> >+		z_states_frametextbox.html \
> 
> nit: a bit strange naming ("z" prefix)
> 

Yes, but it is to keep the files out of the way (alphabetically). Strange but not uncommon.

>+  </a>
> 
> >+
> 
> sure it's worth to mention bug here for new line changes ;)

Fixed.

> 
> >   <script type="application/javascript">
> >     function doTest()
> >     {
> >+      testStates(document, STATE_READONLY);
> >+
> >       document.designMode = "on";
> > 
> >       testStates(document, 0, EXT_STATE_EDITABLE);
> >       testStates("p", 0, EXT_STATE_EDITABLE);
> 
> please add tests for "document", "article" and etc) here (when designMode is
> on)

Done.

> 
> >+
> >+      document.designMode = "off";
> >+
> >+      testStates(document, STATE_READONLY);
> 
> please add test for "p" here.

AFAIK we don't expose readonly for regular html content like "p" so there is nothing really to test for p when designMode is off. I think this is why there was already no test for p in that file when designMode is off.

> 
> >+  <title>nsIAccessible document testing</title>
> 
> nit: states of document accessible testing

Fixed.

> 
> >+
> >+      var docAcc = getAccessible(document, [nsIAccessibleDocument]);
> >+      if (docAcc) {
> >+        document.designMode = "on";
> >+
> >+        testStates(docAcc, 0, EXT_STATE_EDITABLE, STATE_READONLY);
> 
> nit: it doesn't make to pass STATE_READONLY.

Right, fixed this one too.

> 
> >+  
> >+        document.designMode = "off";
> >+
> >+        testStates(docAcc, STATE_READONLY);
> >+      }
> >+      SimpleTest.finish();
> 
> nit: SimpleTest.finish() isn't called if there is no document accessible.

It is outside the "if" block, so I think it is called :)

> 
> >+      frameDoc.designMode = "on";
> >+      testStates(frameDoc,  0, EXT_STATE_EDITABLE);      
> 
> nit: remove all whitespaces in the end of lines.

Done.

> 
> I like tests you are going to put here. But something stopped you? Btw, it's
> nice to have test when iframe element has ARIA role.
> 
> >+      
> >+      window.frames[""]
> 
> oops?

Yes. Cruft. Thanks.

> 
> >+++ b/accessible/tests/mochitest/test_states_readonlybody.html
> 
> test_states_readonlydoc ?
> 
> >+++ b/accessible/tests/mochitest/test_states_readonlybodyarticle.html
> 
> the same.

Done and done.

> 
> >+</head>
> >+<body role="checkbox">
> 
> textbox

Thanks for catching this Surkov, I've fixed this and re-encountered the exception I referred to earlier, now captured as bug 478810. Added a related todo_is test to this patch.

        Attachment #362273 -
        Attachment is obsolete: true

        Attachment #362663 -
        Flags: review?(surkov.alexander)

        Attachment #362663 -
        Flags: review?(surkov.alexander)

        Attachment #362663 -
        Flags: review?(marco.zehe)

        Attachment #362663 -
        Flags: review+

    
    

    
  
Comment on attachment 362663 [details] [diff] [review]
merged some tests


>+      var works = true;
>+      try {
>+          testStates(frameDocTextbox, 0, 0, STATE_READONLY);
>+      }
>+      catch (e) {
>+        thrown = false;
>+      }
>+      todo_is(works, "Checking states of a textbox frame doc should not throw (Bug 478810)");

todo, not todo_is, right?

>+      
>+      frameDoc.designMode = "on";
>+      testStates(frameDoc,  0, EXT_STATE_EDITABLE);
>+      // designMode doesn't propagate to frame docs:
>+      testStates(frameDocArticle, STATE_READONLY);
>+      testStates(frameDocCheckbox, 0, 0, STATE_READONLY);

nit: confusing test or comment, I just don't realize how desgnMode can be propogated from frame's document to another frame's documents.

rest looks ok, thank you great tests and your patience, r=me. Asking additional review from Marco.

    
  

        Attachment #362663 -
        Flags: review?(marco.zehe) → review+

    
    

    
  
Comment on attachment 362663 [details] [diff] [review]
merged some tests

>+      var works = true;
>+      try {
>+          testStates(frameDocTextbox, 0, 0, STATE_READONLY);
>+      }
>+      catch (e) {
>+        thrown = false;
>+      }

Shouldn't this be

works = false;

?

>+<p>Texbox<p>

Missing / in the closing tag.

r=me with that fixed/explained.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch to push
        — 
          — Splinter Review
      

    


  
Surkov, Marco, thanks, your catches were good. This should be the patch to push.

    
    

    
  
Pushed on David's behalf in changeset:
http://hg.mozilla.org/mozilla-central/rev/b7cc3170f52b
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED

        Attachment #362711 -
        Flags: approval1.9.1?

    
  
Flags: in-testsuite+

    
    

    
  
Comment on attachment 362711 [details] [diff] [review]
patch to push

a191=beltzner

        Attachment #362711 -
        Flags: approval1.9.1? → approval1.9.1+

    
    

    
  
Pushed on David's behalf to mozilla-1.9.1 in changeset:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/661a49b5f838
Keywords: fixed1.9.1
Target Milestone: --- → mozilla1.9.1b4

    
    

    
  
Verified fixed in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b4pre) Gecko/20090422 Shiretoko/3.5b4pre (.NET CLR 3.5.30729)
Keywords: fixed1.9.1 → verified1.9.1

    
  
Flags: blocking1.9.2?

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: