Closed Bug 454997 Opened 16 years ago Closed 16 years ago

<body contenteditable="true"> exposed incorrectly

Categories

(Core :: Disability Access APIs, defect)

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

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: aaronlev, Assigned: surkov)

References

Details

(Keywords: access)

Attachments

(1 file, 4 obsolete files)

patch5
8.19 KB, patch
MarcoZ
: review+
aaronlev
: review+
Details | Diff | Splinter Review 
The nsDocumentAccessible and its descendants should be exposing EDITABLE and not be exposing READONLY.
Flags: wanted1.9.1?
Filed bug 455695 on tabbing issue with this -- both bugs affect screen reader support for Gmail.
Attached patch patch (obsolete) — — Splinter Review 
1) expose edtiable state on document if body is contenteditable
2) merge getDOMElementFor and getRoleContent
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #339744 - Flags: review?(aaronleventhal)
Attachment #339744 - Flags: review?(marco.zehe)
Attachment #339744 - Flags: review?(marco.zehe) → review+
Comment on attachment 339744 [details] [diff] [review]
patch

A few nits:
> const STATE_SELECTABLE = nsIAccessibleStates.STATE_SELECTABLE;
> const STATE_SELECTED = nsIAccessibleStates.STATE_SELECTED;
>+const STATE_READONLY = nsIAccessibleStates.STATE_READONLY;

Please keep these in alphabetical order.

>+https://bugzilla.mozilla.org/show_bug.cgi?id=442648

Please adjust the bug number.

>+  <title>nsIAccessible textboxes chrome tests</title>

Please adjust the title.

>+      alert(getStringStates(document));

This is a debug alert, right? It would otherwise halt the test. Please remove if this is not intentional.

>+  <a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=442648">Mozilla Bug 442648</a>

Please adjust the bug number (see above).
Attached patch patch2 (obsolete) — — Splinter Review 
with Marco's comments
Attachment #339744 - Attachment is obsolete: true
Attachment #339747 - Flags: review?(aaronleventhal)
Attachment #339744 - Flags: review?(aaronleventhal)
Comment on attachment 339747 [details] [diff] [review]
patch2

Alex, are you sure GetRoleContent() was never called on a text node before? Now it's going to return the element parent for that. I'm not confident that this won't change role/name/state/relation/attributes etc. for text accessibles.

Wouldn't it be better to just change GetDOMElementFor() and leave GetRoleContent() alone?
Attachment #339747 - Flags: review?(aaronleventhal)
Attached patch patch3 (obsolete) — — Splinter Review 
I tend to agree, text nodes shouldn't grab the role from parent element.
Attachment #339747 - Attachment is obsolete: true
Attachment #340061 - Flags: review?(aaronleventhal)
Attachment #340061 - Flags: review?(aaronleventhal) → review+
Alexander, is this ready to go in anytime soon?
checked in http://hg.mozilla.org/mozilla-central/rev/8fe1cd2d3c66
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Flags: wanted1.9.1?
Resolution: --- → FIXED
Flags: in-testsuite+
Verified fixed in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b1pre) Gecko/20080927033433 Minefield/3.1b1pre
Status: RESOLVED → VERIFIED
Depends on: 457450
Had to back this one out because in addition to the Thunderbird message composition window bug, it causes crashes like 
bp-7dde0c9e-8cb5-11dd-aa40-001cc45a2ce4, making Firefox virtually unusable.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
>diff --git a/accessible/src/base/nsAccessibilityUtils.cpp b/accessible/src/base/nsAccessibilityUtils.cpp
>
>+    nsCOMPtr<nsIDOMHTMLDocument> htmlDoc(do_QueryInterface(node));
>+    if (htmlDoc) {
>+      nsCOMPtr<nsIDOMHTMLElement> bodyElement;
>+      htmlDoc->GetBody(getter_AddRefs(bodyElement));
>+      CallQueryInterface(bodyElement, element);

This last line is what fails. Seems the bodyElement is not always valid.
Attached patch patch4 (obsolete) — — Splinter Review 

        Attachment #340061 -
        Attachment is obsolete: true

        Attachment #342193 -
        Flags: review?(aaronleventhal)

    
  

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

    
    

    
  
Alex, could you put new patch here since after your checkin to bug 457219, this one no longer applies cleanly. Thanks!

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch5
        — 
          — Splinter Review
      

    


  
up to dated to the trunk

        Attachment #342193 -
        Attachment is obsolete: true

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

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

        Attachment #342193 -
        Flags: review?(aaronleventhal)

    
  

        Attachment #342206 -
        Flags: review?(aaronleventhal)

    
    

    
  
Comment on attachment 342206 [details] [diff] [review]
patch5

1. No crashes any more as far as I can see. My testcases that produced crashes with the first checked-in patch no longer reproduce. Great work!
2. Composition rich edit in GMail now exposes editable extended state. Great!

Question: In test_nsIAccessible_editablebody.html, you give the body element an ID of "body". But you never use it. Is this on purpose, or did you originally plan to use it and then forgot to remove it? You can leave it if you wish, I just noticed it and thought I'd ask.

r=me

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

    
  

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

    
    

    
  
(In reply to comment #15)

> Question: In test_nsIAccessible_editablebody.html, you give the body element an
> ID of "body". But you never use it. Is this on purpose, or did you originally
> plan to use it and then forgot to remove it? You can leave it if you wish, I
> just noticed it and thought I'd ask.

yes, eventually I forgot to remove it. thanks.

    
  
Status: REOPENED → RESOLVED
Closed: 16 years ago → 16 years ago
Resolution: --- → FIXED

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: