CKEditor document should be editable

VERIFIED FIXED in mozilla2.0

Status

()

Core
Disability Access APIs
VERIFIED FIXED
6 years ago
4 years ago

People

(Reporter: davidb, Assigned: surkov)

Tracking

({regression})

unspecified
mozilla2.0
regression
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 final+)

Details

(Whiteboard: [hardblocker], URL)

Attachments

(2 attachments)

(Reporter)

Description

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

Updated

6 years ago
Assignee: nobody → bolterbugz

Comment 1

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

Comment 2

6 years ago
Probably: http://hg.mozilla.org/mozilla-central/rev/e0fc18b3bc41
(Reporter)

Comment 3

6 years ago
I'm not seeing readonly on trunk with accprobe.
(Reporter)

Comment 4

6 years ago
I am seeing the busy state.
(Reporter)

Comment 5

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

Comment 6

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

Comment 7

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

Comment 8

6 years ago
I do confirm the readonly in accprobe on my non debug build.
(Reporter)

Comment 9

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

Comment 10

6 years ago
This is pretty bad. I can't believe the report came in today, sigh.
(Assignee)

Comment 11

6 years ago
requesting a blocking, ckeditor (and similar web editors) should be broken for screen reader users.
blocking2.0: --- → ?

Comment 12

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

Comment 13

6 years ago
I can't believe this is happening. Approving blocking. Hope that fix works.
blocking2.0: ? → final+
Whiteboard: [d?][hardblocker][eta: real soon!]
(Assignee)

Comment 14

6 years ago
Created attachment 516486 [details] [diff] [review]
patch
Attachment #516486 - Flags: review?(bolterbugz)
(Assignee)

Comment 15

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

Comment 16

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

Comment 17

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

Updated

6 years ago
Whiteboard: [d?][hardblocker][eta: real soon!][has patch] → [hardblocker][eta: real soon!][has patch]

Comment 18

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

Comment 19

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

Comment 20

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

Comment 21

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

Comment 22

6 years ago
great! :D
(Assignee)

Comment 23

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

Comment 24

6 years ago
Created attachment 516529 [details] [diff] [review]
patch [landed, mochitest]
(Assignee)

Updated

6 years ago
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.