Closed Bug 542824 Opened 15 years ago Closed 15 years ago

Create child accessible for text controls from native anonymous content

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: ehsan.akhgari, Assigned: surkov)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

WIP 1
4.65 KB, patch
Details | Diff | Splinter Review
patch
17.00 KB, patch
MarcoZ
: review+
davidb
: review+
Details | Diff | Splinter Review
The patch in bug 221820 is causing a leak in a11y tests. What's happening is that a call made by the accessibility module causes the editor to be initialized twice. The best way to avoid that problem that we've found so far is to special case the elements for which we know the editor's root to be an anonymous div beneath them to avoid trying to get an nsIEditor pointer. I'll attach a patch to this bug which does that. See bug 221820 comment 85 for more context.
In general all you need is to remove nsHyperTextAccessible::CacheChildren() I think. It should work for HTML case since nsAccessibleTreeWalker will navigate into native anonymous content. You need to ensure tree/test_textctrl.html pass. But it should break xul:textbox and xul:textarea since it doesn't allow to walk into XBL anonymous content, therefore underlying html:input won't be find and we fail to create accessible for its native anonymous content. To be sure I would like to get a similar test for xul:textbox and xul:textarea.
Attached patch WIP 1 — — Splinter Review
(In reply to comment #1) > In general all you need is to remove nsHyperTextAccessible::CacheChildren() I > think. It should work for HTML case since nsAccessibleTreeWalker will navigate > into native anonymous content. You need to ensure tree/test_textctrl.html pass. > But it should break xul:textbox and xul:textarea since it doesn't allow to walk > into XBL anonymous content, therefore underlying html:input won't be find and > we fail to create accessible for its native anonymous content. To be sure I > would like to get a similar test for xul:textbox and xul:textarea. Hmm, but that wouldn't work for contenteditable stuff, right? The WIP 1 patch passes all tests, except tree/test_combobox.xul's third test. It seems that it's blocking the walker to see the XUL content under the xul:textbox element, though I don't know how to fix that.
Seems like in addition to the test failure, the leak is persisting with this patch as well. http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1264715526.1264724090.687.gz
changing summary to reflect better the bug
Summary: Special case <input type=text|password> and <textarea> elements inside nsHyperTextAccessible::CacheChildren → Create child accessible for text controls from native anonymous content
So, actually, nsIEditor::GetRootElement does _not_ do what you would expect it to do at all! The "editor root" is actually the <body> element for normal HTML documents (and the root document for other types of documents), so currently nsHyperTextAccessible::CacheChildren is probably doing the wrong thing anyway. I think what surkov plans to do in bug 530081 can actually help us clean this mess up. But to make a long story short, HTML editor just adds everything as descendants of the regular DOM tree under the contenteditable node, and plaintext editor adds everything to a native anonymous div under the nsTextControlFrame, so by having an API which can enumerate both regular children and native anonymous content, we could actually handle both cases correctly. I'm marking this as a dependent of bug 530081.
Depends on: 530081
(In reply to comment #6) > So, actually, nsIEditor::GetRootElement does _not_ do what you would expect it > to do at all! The "editor root" is actually the <body> element for normal HTML > documents (and the root document for other types of documents), so currently > nsHyperTextAccessible::CacheChildren is probably doing the wrong thing anyway. I didn't get your point. Neither body nor root element isn't accessible so we skip it and get its children. We do this for plain text editors. What could be wrong here? > I think what surkov plans to do in bug 530081 can actually help us clean this > mess up. But to make a long story short, HTML editor just adds everything as > descendants of the regular DOM tree under the contenteditable node, and > plaintext editor adds everything to a native anonymous div under the > nsTextControlFrame, so by having an API which can enumerate both regular > children and native anonymous content, we could actually handle both cases > correctly. Actually we can do all of this by nsAccessilbleTreeWalker. Bug 530081 is targeted to make our code cleaner, safer and expose generated content which is not accessible now.
(In reply to bug 221820 comment #88) > The a11y leak is being tracked in bug 542824. Other than that, I think this > patch is ready to land once it gets reviewed. I would appreciate timely > reviews here since it has the potential of rotting very soon. I don't get how this bug is related with leaks in general. If we'll stop to use editor API to cache accessible children then it probably will fix the leak but it doesn't guarantee other leaks can be found later. I mean this bug doesn't fix the real problem if I get right.
Attached patch patch — — Splinter Review
Attachment #424476 - Flags: review?(marco.zehe)
Attachment #424476 - Flags: review?(bolterbugz)
Comment on attachment 424476 [details] [diff] [review] patch r=me, cool thanks!
Attachment #424476 - Flags: review?(marco.zehe) → review+
Attachment #424476 - Flags: review?(bolterbugz) → review+
Comment on attachment 424476 [details] [diff] [review] patch r=me thanks. > function doTest() > { >- accTree = { >+ var accTree = { interesting. good catch.
landed on 1.9.3 - http://hg.mozilla.org/mozilla-central/rev/8c1aa8a2485c
Assignee: ehsan.akhgari → surkov.alexander
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
(In reply to comment #7) > (In reply to comment #6) > > So, actually, nsIEditor::GetRootElement does _not_ do what you would expect it > > to do at all! The "editor root" is actually the <body> element for normal HTML > > documents (and the root document for other types of documents), so currently > > nsHyperTextAccessible::CacheChildren is probably doing the wrong thing anyway. > > I didn't get your point. Neither body nor root element isn't accessible so we > skip it and get its children. We do this for plain text editors. What could be > wrong here? OK, let me elaborate. Consider the following document: <html> <body> <textarea>hello world</textarea> <div contenteditable="true">a<br>test</div> </body> </html> The textarea elements has an anonymous div beneath it, which has textnode and br elements as children. Calling nsIEditor::GetRootElement for that textarea would return the anonymous div, so the first child would be the "hello" textnode, which is expected. However, for the div, calling nsIEditor::GetRootElement will return the body element, and its first child would be an empty textnode, its second child would be the textarea, and so on. I think this is not what a11y expected, is that right? > > I think what surkov plans to do in bug 530081 can actually help us clean this > > mess up. But to make a long story short, HTML editor just adds everything as > > descendants of the regular DOM tree under the contenteditable node, and > > plaintext editor adds everything to a native anonymous div under the > > nsTextControlFrame, so by having an API which can enumerate both regular > > children and native anonymous content, we could actually handle both cases > > correctly. > > Actually we can do all of this by nsAccessilbleTreeWalker. Bug 530081 is > targeted to make our code cleaner, safer and expose generated content which is > not accessible now. I didn't know that...
(In reply to comment #8) > (In reply to bug 221820 comment #88) > > > The a11y leak is being tracked in bug 542824. Other than that, I think this > > patch is ready to land once it gets reviewed. I would appreciate timely > > reviews here since it has the potential of rotting very soon. > > I don't get how this bug is related with leaks in general. If we'll stop to use > editor API to cache accessible children then it probably will fix the leak but > it doesn't guarantee other leaks can be found later. I mean this bug doesn't > fix the real problem if I get right. The change in bug 221820 caused the a11y module to try to initialize the editor while another initialization was in progress, which caused the members of an editor object to leak. There is nothing wrong with this code leak-wise without the patch in bug 221820, it's just that the patch on that bug is changing the order of things, and the a11y code was not ready to handle that. I suspect that the leak should be solved now. I will test it and report the results here.
(In reply to comment #13) > However, for the div, calling nsIEditor::GetRootElement will return the body > element, and its first child would be an empty textnode, its second child would > be the textarea, and so on. I think this is not what a11y expected, is that > right? Sure, but we don't do this in a11y :) nsIEditor::GetRootElement is called for stuffs like textarea only, it's not used for contenteditable elements. (In reply to comment #14) > The change in bug 221820 caused the a11y module to try to initialize the editor > while another initialization was in progress, which caused the members of an > editor object to leak. There is nothing wrong with this code leak-wise without > the patch in bug 221820, it's just that the patch on that bug is changing the > order of things, and the a11y code was not ready to handle that. This bothers me. Probably accessibility and editor initialization are very tight related and you're right. But their relation is not evident for me and therefore it sounds like we tried to use editor interface and break it. Could you please give me an idea how it happens?
Flags: in-testsuite+
Blocks: treea11y
removing dependence from bug 530081
No longer depends on: 530081
(In reply to comment #15) > (In reply to comment #13) > > > However, for the div, calling nsIEditor::GetRootElement will return the body > > element, and its first child would be an empty textnode, its second child would > > be the textarea, and so on. I think this is not what a11y expected, is that > > right? > > Sure, but we don't do this in a11y :) nsIEditor::GetRootElement is called for > stuffs like textarea only, it's not used for contenteditable elements. Oh, OK, I didn't know that! So that part of my comment can safely be ignored. :-) > (In reply to comment #14) > > > The change in bug 221820 caused the a11y module to try to initialize the editor > > while another initialization was in progress, which caused the members of an > > editor object to leak. There is nothing wrong with this code leak-wise without > > the patch in bug 221820, it's just that the patch on that bug is changing the > > order of things, and the a11y code was not ready to handle that. > > This bothers me. Probably accessibility and editor initialization are very > tight related and you're right. But their relation is not evident for me and > therefore it sounds like we tried to use editor interface and break it. Could > you please give me an idea how it happens? Sure. Here is how things are working right now. When creating the frame for each input and textarea field, we create the corresponding anonymous content, then initialize the nsPlaintextEditor for each field eagerly, and from that point, the editor is responsible for displaying content in the field, handling value updates from js, and also handling user input. We're trying to optimize things based on the assertion that not all of the text fields on every page are actually used by the user to type something in. So, in bug 221820, I'm trying to change things so that when creating the frame, the anonymous content is created, and the current value is appended to the anon div as a simple textnode, which we will manage for displaying the content and handling value changes from js, without creating an editor. (Aside note, this is happening only for text and password inputs for now, textareas will probably follow in another bug.) We try to initialize the editor only when it's actually going to be used, like when the input field is focused. In order to avoid bugs with the existing textnodes under the anon div, we have to remove empty textnodes from the anon div before initializing the editor. So, what happens is that we have the nsTextControlFrame::EnsureEditorInitialized function which is called whenever the editor is needed to make sure that it's been initialized, and if not, initialize it right away. This function sets a simple flag when it's done initializing the editor, which it will check later to make sure that it doesn't initialize the editor more than once. Now, at some point along the way of initializing the editor, nsTextControlFrame::EnsureEditorInitialized calls nsIContent::RemoveChildAt to remove the empty textnode from the anonymous div. This call notifies the a11y code, and it goes on to try to call nsHTMLInputElement::GetEditor, which itself calls nsTextControlFrame::EnsureEditorInitialized. The second call also sees that the flag has not been set, so it creates another editor object, and sets the mEditor member nsCOMPtr variable to the new editor, which already points to the editor object created by the first EnsureEditorInitialized call, which causes the members of the nsPlaintextEditor object to leak. The main reason of the leak is EnsureEditorInitialized being called in a reentering fashion. For the full stack showing the above sequence of calls, see bug 221820 comment 85. I have also written a patch which detects other possible reentrancy paths during runtime of debug builds using an assertion, and I will also check the code statically when bug 542364 is fixed.
Ok, thank you for clarification. Can you think of the way to get rid the nsIContent::RemoveChildAt call somehow because it sounds it's sort of "in process" stuff? If I get right then we'll handle second notification the children was appended during the editor initialization. Ideally we would like to get notifications when something is really changed because every a11y notification call the children were changed makes suffer us because of performance.
I see your concern, but unfortunately I don't think that there currently is any way to avoid that RemoveChildAt call. I will probably fix this eventually, but in the interim, it would be nice if I could inform a11y somehow that a certain event is insignificant. At first I was thinking of finding some way to make a11y completely ignore the mutation notification, but I figured that you might be holding on to the old textnode somehow, and it would probably break a11y in some way if it suddenly finds out that the textnode object is no longer used, and has been replaced by another textnode. That said, I still don't see why you need to walk the native anonymous tree for input fields. I would think that you can get all the information needed form the content node...
> That said, I still don't see why you need to walk the native anonymous tree for > input fields. I would think that you can get all the information needed form > the content node... My understanding is that it is sort of a general way to find the 'value' text, and to be able to expose it along with text offsets and other info that (ns)IAccessibleText API supports. I'm wondering if expecting a role: ROLE_TEXT_LEAF for the 'autocomplete2' case is fair game.
(In reply to comment #12) > landed on 1.9.3 - http://hg.mozilla.org/mozilla-central/rev/8c1aa8a2485c second thoughts... I'm a little unsure but might there be cases of nsIAccessibleText regression with this push? If there might be anony text frames we need to expose from readonly text?
(In reply to comment #21) > (In reply to comment #12) > > landed on 1.9.3 - http://hg.mozilla.org/mozilla-central/rev/8c1aa8a2485c > > second thoughts... > > I'm a little unsure but might there be cases of nsIAccessibleText regression > with this push? If there might be anony text frames we need to expose from > readonly text? To be clear, I think we should leave this because I can't think of cases we would regress.
(In reply to comment #22) > (In reply to comment #21) > > (In reply to comment #12) > > > landed on 1.9.3 - http://hg.mozilla.org/mozilla-central/rev/8c1aa8a2485c > > > > second thoughts... > > > > I'm a little unsure but might there be cases of nsIAccessibleText regression > > with this push? If there might be anony text frames we need to expose from > > readonly text? > > To be clear, I think we should leave this because I can't think of cases we > would regress. Me too. But you got good idea we should have tree tests for readonly and disabled text fields. I think editor was created for readonly text field, but it's possibly not for disabled. So the behaviour might was changed but it's not regression. We will expose accessible tree which is fine I think.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: