559710 - Don't add a br node under text controls when the editor is not initialized for them yet

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[(no longer active)]

Attachment: Patch (v1)

See bug 473178 comment 27.  This is a very good perf improvement, if we can make sure that the a11y module can handle having a different anonymous content tree in that case.

The patch I attached does this, maybe David or Alexander can look into fixing this in the a11y side?

I've pushed this patch to the try server, and I'll post an update here with the failure logs when they're available.

Comment 1

[(no longer active)]

Linux accessibility test failures:

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1271381238.1271390607.15302.gz

Comment 2

[alexander :surkov (:asurkov)]

Ehsan, could you describe your patch please? I need to understand how it can affect on a11y. Honestly I don't see a reason why presence or absence of initial <br> can affect on a11y. It might change the way how accessible text methods work but it should be ok. If there is no any bad affection on a11y then all we need is to fix mochitests.

Comment 3

[David Bolter [:davidb] (NeedInfo me for attention)]

(Mid air collision)

I think the problem might be our tests. I'm not convinced we are testing a case that is required for accessibility, since our accessibility engine is pretty capable of exposing all kinds of trees.

I think perhaps we can loosen our tests. Ehsan do you have a try build that Marco (or Alex/I) can take for a spin?

Comment 4

[David Bolter [:davidb] (NeedInfo me for attention)]

OK I looking at this now (built it).

Comment 5

[David Bolter [:davidb] (NeedInfo me for attention)]

This patch caused no issues in my tests with Accerciser (Linux a11y diagnostic tool). My tests included https://bug473178.bugzilla.mozilla.org/attachment.cgi?id=357388 as well as some contenteditable examples for good measure.

Comment 6

[David Bolter [:davidb] (NeedInfo me for attention)]

The information exposed through the atk text interface was correct and updated correctly as you would expect.

Comment 7

[David Bolter [:davidb] (NeedInfo me for attention)]

I'd like Marco to spin this patch against various AT before we land it, but he is off until Monday. Can we wait?

Comment 8

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #1)
> Linux accessibility test failures:
> 
> http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1271381238.1271390607.15302.gz

Regarding the fails... why are we getting two children? What are they?

Comment 9

[(no longer active)]

(In reply to comment #2)
> Ehsan, could you describe your patch please?

Before the lazy initialization code, an editor object was being instantiated for all input text controls.  The way that the editor works is that it modifies a div element which is the root of the native anonymous content tree.  When the value is not empty, the editor uses a text node under the anonymous div for storing and displaying the value.  When the value is empty, the editor removes that text node and adds a special br node (which it calls the bogus node) under the anonymous div.

With my original lazy initialization patch, I imitated the exact same behavior when no editor was present.  This patch modifies that behavior so that at all times there is only a single text node under the anonymous div.  When the value is empty, we set the value of the text node to "\n" in order to preserve the line-height calculations, and when the value is not empty, we set the text node value to the input control value.

> I need to understand how it can
> affect on a11y. Honestly I don't see a reason why presence or absence of
> initial <br> can affect on a11y. It might change the way how accessible text
> methods work but it should be ok. If there is no any bad affection on a11y then
> all we need is to fix mochitests.

Well, that I can't be certain of, because I don't know what is expected to happen at the a11y side, and what types of assumptions the a11y code currently makes.

Comment 10

[(no longer active)]

(In reply to comment #3)
> I think perhaps we can loosen our tests. Ehsan do you have a try build that
> Marco (or Alex/I) can take for a spin?

The try server build I submitted last night is available at:

<http://build.mozilla.org/tryserver-builds/[e-mail]-try-a437d6e37e79>

Comment 11

[(no longer active)]

(In reply to comment #8)
> (In reply to comment #1)
> > Linux accessibility test failures:
> > 
> > http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1271381238.1271390607.15302.gz
> 
> Regarding the fails... why are we getting two children? What are they?

Is there any way to make the test log what it sees?  If so, I can add it to the patch and push it to try to see what children we're getting.

Comment 12

[Marco Zehe (:MarcoZ)]

I threw this try-server build against several screen readers with varying testcases (sites with input @type="text", textarea, contenteditable elements), and didn't find anything wrong. Looks like this perf improvement has no negative impact on assistive technologies.

Comment 13

[(no longer active)]

Comment on attachment 439419 [details] [diff] [review]
Patch (v1)

Requesting review from bz, and also feedback from David to give me the official go from the a11y side of things.

Also, should I just modify the a11y test to expect two children?  Or do we need a more involved fix?

Comment 14

[alexander :surkov (:asurkov)]

(In reply to comment #13)

> Also, should I just modify the a11y test to expect two children?  Or do we need
> a more involved fix?

I would really appreciate to know who is a second child. Child adoption is serious thing :)

Comment 15

[(no longer active)]

(In reply to comment #14)
> (In reply to comment #13)
> 
> > Also, should I just modify the a11y test to expect two children?  Or do we need
> > a more involved fix?
> 
> I would really appreciate to know who is a second child. Child adoption is
> serious thing :)

I'm building right now with a11y turned on, and I'll ask for David's advice with regard to child adoption later today!

Comment 16

[(no longer active)]

David gave me the verbal go on just adding a child to the list, and filing a follow-up bug to figure out why it's now needed.

I filed bug 560270 about that, and will attach a new patch here in a moment.

Comment 17

[(no longer active)]

Attachment: Patch (v1.1)

Comment 18

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 439972 [details] [diff] [review]
Patch (v1.1)

r=me for the a11y test change, with one nit:

>--- a/accessible/tests/mochitest/tree/test_combobox.xul
>+++ b/accessible/tests/mochitest/tree/test_combobox.xul


>             role: ROLE_ENTRY,
>             children: [
>               {
>                 role: ROLE_TEXT_LEAF // HTML 5 placeholder attribute value
>+              },
>+              {
>+                role: ROLE_TEXT_LEAF // XXXehsan We don't yet know what this is!

Assuming we can co-debug the follow up since the editor scares me still; okay.
Nit: please reference bug 560270 in the comment.

Comment 19

[(no longer active)]

So, this text node is coming from the textnode under the value div.  There would be a problem with just ignoring this textnode.  When the value is empty, we convert it to "\n" for line-height calculations, and the a11y code converts that newline into a space and exposes the text leaf node with that space.  This means that a11y clients will think that there's a space inside the text box, while in fact there isn't any space there.

I think the correct way to fix it is to look at the content node (or maybe the frame) for retrieving the value, instead of going through the textnode contents.

OTOH, we believe that accessibility clients do not use the text leaf contents to get the value of the textbox.  David is going to confirm this with one of his contacts, and we'll wait for him to get back at us.

Comment 20

[(no longer active)]

Comment on attachment 439972 [details] [diff] [review]
Patch (v1.1)

Switching the review request to roc as he knows the background of this bug, and Boris has a lot of review requests to catch up on.

Comment 21

[David Bolter [:davidb] (NeedInfo me for attention)]

Jamie, what API does NVDA call on text fields, text boxes and their ilk?

Comment 22

[James Teh [:Jamie]]

If the accessible supports IAccessibleText (which it must), we currently use IAccessibleText alone to get the text.

Out of curiosity, why is this being converted to a space anyway? This makes sense when it is just DOM content, but in this case, it's being used for line height calculation, so it should be different. Can that be detected?

Comment 23

[(no longer active)]

(In reply to comment #22)
> If the accessible supports IAccessibleText (which it must), we currently use
> IAccessibleText alone to get the text.

If I understand things correctly, our IAccessibleText implementation just asks the content node for the value, so this should be fine.  David, please correct me if I'm wrong.

> Out of curiosity, why is this being converted to a space anyway? This makes
> sense when it is just DOM content, but in this case, it's being used for line
> height calculation, so it should be different. Can that be detected?

For input elements, if you set the value of the single line text boxes to something like "test\nme" using js, the input control converts that newline to a space by default before showing that on screen.  The accessibility code is trying to simulate that behavior for the text leaves inside the text control.

So, David, if the value of the editor.singleLine.pasteNewlines is 2 (which is the default for Firefox), newlines will always be converted into spaces inside the text node in question, so in fact the accessibility code can check that pref and ignore text nodes which have a value of "\n" if that pref's value is 2.  Is that something which we can do inside the a11y code?

Note that the pref in question is actually being misused here (it was originally added to only affect the "paste" behavior, as its name suggests), and the mess will be fixed with bug 549475 eventually.

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 439972 [details] [diff] [review]
Patch (v1.1)

the layout code is fine, but fix the XXX comment.

Comment 25

[(no longer active)]

Attachment: Current patch

Current patch with the fixed comment.  Waiting for David's green light before pushing this.

Comment 26

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to comment #22)
> If the accessible supports IAccessibleText (which it must), we currently use
> IAccessibleText alone to get the text.

Oh that makes things more worrisome.

> Out of curiosity, why is this being converted to a space anyway? This makes
> sense when it is just DOM content, but in this case, it's being used for line
> height calculation, so it should be different. Can that be detected?

Let's discuss on bug 560270.

Comment 27

[(no longer active)]

Attachment: New patch

I fixed a few unneeded things in the patch.  I'm carrying the r+'s forward on this patch.

Comment 28

[(no longer active)]

Attachment: Patch (v1.2)

So, apparently the non-breaking space inside the anonymous div's for empty elements are not needed.  I confirmed it with bz on IRC, and ran our reftests agaist this patch, so I'm removing them in this patch.  This is a trivial change, but I'm asking for roc's review again here.

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+    textContent = mValueDiv->GetChildAt(0);

Why not just set it to textNode?

Comment 30

[(no longer active)]

Attachment: Patch (v1.3)

Makes sense.

Comment 31

[(no longer active)]

The try server shows an accessibility test failure, which I don't quite understand:

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1272058499.1272067606.30923.gz

I don't get this failure on Mac with a11y enabled.  I'm currently building it on Linux to see what's going on.

Comment 32

[(no longer active)]

Attachment: Patch (v1.4)

This fixes one more accessibility test failure which I didn't catch earlier because the failing test does not run on Mac.

Comment 33

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/26ce8f7e485c