Closed Bug 638106 Opened 13 years ago Closed 13 years ago

CKEditor document should be editable

Categories

(Core :: Disability Access APIs, defect)

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

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla2.0
Tracking Flags:
Tracking Status
blocking2.0 --- final+

People

(Reporter: davidb, Assigned: surkov)

References

(
URL
)

Details

(Keywords: regression, Whiteboard: [hardblocker])

Attachments

(2 files)

patch
2.98 KB, patch
davidb
: review+
Details | Diff | Splinter Review
patch [landed, mochitest]
10.25 KB, patch
MarcoZ
: review+
Details | Diff | Splinter Review 
From an email:
"In 3.6.13, the editor is shown as having role of document with msaa state 
focused|focusable and IA2 state of editable|horizontal|opaque.
In 4.0 beta 12, the role is still document, but the msaa state is focused|readonly|focusable and the ia2 state is horizontal|opaque|stale.
As a result, JAWS no longer recognizes this as an editable control."
Assignee: nobody → bolterbugz
Working: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b10pre) Gecko/20110125 Firefox/4.0b10pre

Broken: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b11pre) Gecko/20110126 Firefox/4.0b11pre

Regression range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=6a097e294828&tochange=e0fc18b3bc41
I'm not seeing readonly on trunk with accprobe.
I am seeing the busy state.
Unfortunately I will be AFK for about 15 hours. Alexander, Marco we will likely need to lobby for a FF4 fix here but I couldn't recreate the bug via accprobe. Marco mentioned in IRC that refreshing the virtual buffer didn't fix things.

Marco is going to confirm if comment 2 is the right guess.
Assignee: bolterbugz → surkov.alexander
(In reply to comment #2)
> Probably: http://hg.mozilla.org/mozilla-central/rev/e0fc18b3bc41

Confirmed. The parent changeset doesn't expose it, a build with this exact changeset added does. This is on a release build. Which might explain why you're not seeing it in a debug build. So this would indicate some sort of either timing or optimization related.
I'm waiting for my non-debug build. Note we should add as much info here as possible to help drivers should it be necessary. An impact statement, workarounds, severity etc. The timing of this bug discovery is very unfortunate I'll try to find time later tonight to recreate and to produce a reduced test case.
I do confirm the readonly in accprobe on my non debug build.
Looking at tree inspector and comparing the a11y tree with what I see in the DOM. The a11y tree is roughly:

form
 section
   application
     iframe
       document <-- readonly


The DOM is roughly:
form
 ...
  iframe
    #document <-- designMode "off"
      html (Document node)
      HTML
        HEAD
        BODY <-- contenteditable=true
        
What does our a11y document object correspond with in this DOM tree?
This is pretty bad. I can't believe the report came in today, sigh.
requesting a blocking, ckeditor (and similar web editors) should be broken for screen reader users.
blocking2.0: --- → ?
It's not just because they're setting contentEditable="true" on the body, as it works fine using this code:
...
<body onLoad="document.getElementById('editableDoc').contentWindow.document.body.contentEditable = 'true';">
<iframe id="editableDoc" src="about:blank"></iframe>
...
I can't believe this is happening. Approving blocking. Hope that fix works.
blocking2.0: ? → final+
Whiteboard: [d?][hardblocker][eta: real soon!]
Attached patch patch — — Splinter Review 

        Attachment #516486 -
        Flags: review?(bolterbugz)

    
    

    
  
(In reply to comment #12)
> It's not just because they're setting contentEditable="true" on the body, as it
> works fine using this code:

contentEditable shouldn't result in body recreation (iirc, Ehsan fixed that recently), that means they should reinsert body or html element into a document.
Whiteboard: [d?][hardblocker][eta: real soon!] → [d?][hardblocker][eta: real soon!][has patch]

    
    

    
  
Comment on attachment 516486 [details] [diff] [review]
patch

r=me. You're fast dude.


> void
> nsDocAccessible::NotifyOfInitialUpdate()
> {
>+  // The content element may be changed before the initial update and then we
>+  // miss the notification (since content tree change notifications are ignored
>+  // prior to initial update). Make sure the content element is valid.
>+  nsIContent* contentElm = nsCoreUtils::GetRoleContent(mDocument);
>+  if (mContent != contentElm)
>+    mContent = contentElm;
>+

Please consider guarding against nsCoreUtils::GetRoleContent returning null (although I'm not sure how that would happen).

I wouldn't hold off if you can't get this under mochitest soon. We need to land this ASAP.

        Attachment #516486 -
        Flags: review?(bolterbugz) → review+

    
    

    
  
(In reply to comment #16)

> Please consider guarding against nsCoreUtils::GetRoleContent returning null
> (although I'm not sure how that would happen).

Yes, I fixed that locally. It makes sense at least because we don't set null in other places.

> I wouldn't hold off if you can't get this under mochitest soon. We need to land
> this ASAP.

I'll try to to do mochitest, anyway I'll land it today.
Whiteboard: [d?][hardblocker][eta: real soon!][has patch] → [hardblocker][eta: real soon!][has patch]

    
    

    
  
can this be landed sooner than "today", like possibly "now"? this is the last blocker.

    
    

    
  
this might not be the last blocker.. there are 2 nominations plus the other 1 blocker?

    
    

    
  
(In reply to comment #19)
> this might not be the last blocker.. there are 2 nominations plus the other 1
> blocker?

but i would like to see this landed ASAP too.

    
    

    
  
I'm finishing mochitest, I will land it really soon.

    
    

    
  
great! :D

    
    

    
  
landed on 2.0 (fx4rc) - http://hg.mozilla.org/mozilla-central/rev/7570423a050f
Status: NEW → RESOLVED
Closed: 13 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0

    
  
Whiteboard: [hardblocker][eta: real soon!][has patch] → [hardblocker]

    
    

    
  
Comment on attachment 516529 [details] [diff] [review]
patch [landed, mochitest]

Giving a postume r+ on the Mochitest part.

        Attachment #516529 -
        Flags: review+

    
    

    
  
Trouble verifying this on the March 3 nightly build since the build was made from a very early time (around 9 PM PST March 2) and didn't pick up the above checkin.

    
    

    
  
This tinderbox build picked up the change just fine: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b13pre) Gecko/20110302 Firefox/4.0b13pre
Status: RESOLVED → VERIFIED

    
    

    
  
Also confirmed in this regular nightly build: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b13pre) Gecko/20110303 Firefox/4.0b13pre. This is the respin of the March 3 nightly from this changeset: http://hg.mozilla.org/mozilla-central/rev/290712e55ade

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: