899433 - Accessibility returns empty line for last line in certain cases

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[James Teh [:Jamie]]

In Thunderbird's message composer and Firefox's Web Console, accessibility returns empty lines for any character on the last line.

I'm not sure where else this can be reproduced. I wasn't able to reproduce it in isolation in a web document.

Str:
1. Open a new message in Thunderbird and make sure it contains no text.
Alternatively:
1. Open the Web Console in Firefox.
2. Type the word "test".
3. Retrieve the accessible for the text input area.
4. Retrieve the line offsets at offset 0; e.g. IAccessibleText::textAtOffset(0, IA2_TEXT_BOUNDARY_LINE).
Expected: The offsets should be (0, 4).
Actual: The offsets are (0, 0).
5. Repeat step 5 for offsets 1, 2, 3 and 4.
Same results.

This seems similar to bug 875794. In fact, when this occurs, there is a trailing line feed character even when enter isn't pressed.

I'm not sure when this started occurring, but I'd say it was within the last couple of months.

Impact: I can't read the last line when composing messages in Thunderbird, nor can I read the line I'm working on while working in the Firefox Web Console.

Comment 1

[alexander :surkov (:asurkov)]

seems to be similar to bug 873358, I see HTML br under input

Comment 2

[James Teh [:Jamie]]

I think accessibility needs to be able to handle this. You can reproduce it like this:
<div designMode="true">foo<br></div>
This is perfectly valid, but line offsets break.

Comment 3

[alexander :surkov (:asurkov)]

(In reply to James Teh [:Jamie] from comment #2)
> I think accessibility needs to be able to handle this. You can reproduce it
> like this:
> <div designMode="true">foo<br></div>
> This is perfectly valid, but line offsets break.

Nice, thank you

Comment 4

[alexander :surkov (:asurkov)]

filed bug 900943 for textbox case, let's keep this bug for comment #2 case.

Comment 5

[alexander :surkov (:asurkov)]

(In reply to James Teh [:Jamie] from comment #2)
> I think accessibility needs to be able to handle this. You can reproduce it
> like this:
> <div designMode="true">foo<br></div>
> This is perfectly valid, but line offsets break.

it doesn't seem I can reproduce it. Jamie, did you try it on Nightly?

Comment 6

[James Teh [:Jamie]]

(In reply to alexander :surkov from comment #5)
> > <div designMode="true">foo<br></div>
> it doesn't seem I can reproduce it.
Sorry, typo. That should have been:
<div contentEditable="true">foo<br></div>

Comment 7

[alexander :surkov (:asurkov)]

(In reply to James Teh [:Jamie] from comment #6)
> (In reply to alexander :surkov from comment #5)
> > > <div designMode="true">foo<br></div>
> > it doesn't seem I can reproduce it.
> Sorry, typo. That should have been:
> <div contentEditable="true">foo<br></div>

right, but I get correct results. Perhaps a test case?

Comment 8

[James Teh [:Jamie]]

Attachment: Test case.

Weird. I am using just that code (wrapped in html and body tags and with a title for completeness). Tested with Firefox 26.0a1 (2013-08-06). On the editable section accessible, IAccessibleText::textAtOffset(0, IA2_TEXT_BOUNDARY_LINE) returns (0, 0).

Comment 9

[alexander :surkov (:asurkov)]

It seems there's layout problem here so that these two examples look/behave the same way:

<div contentEditable="true">foo</div>
<div contentEditable="true">foo<br></div>

Robert, thoughts?

Comment 10

[James Teh [:Jamie]]

(In reply to alexander :surkov from comment #9)
> It seems there's layout problem here so that these two examples look/behave
> the same way:
...
> <div contentEditable="true">foo</div>
I can't reproduce the problem with this example, only with the added br. I don't understand why we're getting different results here.

Comment 11

[alexander :surkov (:asurkov)]

(In reply to James Teh [:Jamie] from comment #10)
> (In reply to alexander :surkov from comment #9)
> > It seems there's layout problem here so that these two examples look/behave
> > the same way:
> ...
> > <div contentEditable="true">foo</div>
> I can't reproduce the problem with this example, only with the added br. I
> don't understand why we're getting different results here.

I meant keyboard behavior: in case "foo<br>" example, there's no new empty line and it seems it's equivalent to "foo" example (but they behave differently on AT API level)

Comment 12

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

(In reply to alexander :surkov from comment #9)
> It seems there's layout problem here so that these two examples look/behave
> the same way:
> 
> <div contentEditable="true">foo</div>
> <div contentEditable="true">foo<br></div>
> 
> Robert, thoughts?

If you dump the frame tree, you'll see there's a single line with a <br> at the end. There is no second line because we don't create lines with no content at all.

Comment 13

[alexander :surkov (:asurkov)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #12)
> (In reply to alexander :surkov from comment #9)
> > It seems there's layout problem here so that these two examples look/behave
> > the same way:
> > 
> > <div contentEditable="true">foo</div>
> > <div contentEditable="true">foo<br></div>
> > 
> > Robert, thoughts?
> 
> If you dump the frame tree, you'll see there's a single line with a <br> at
> the end. There is no second line because we don't create lines with no
> content at all.

right, iirc you said previously it's done for optimization proposes. Anyway do you agree that the second case should have two lines? If so then can you think of approaches how it can be fixed please?

Comment 14

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

It doesn't render with two lines in Gecko. Why do you think it should be treated as two lines?

Comment 15

[alexander :surkov (:asurkov)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #14)
> It doesn't render with two lines in Gecko. Why do you think it should be
> treated as two lines?

I was thinking approximately this way:
<br> is equivalent to '\n' new line character which basically means new line; so if <br> doesn't really mean a new line then it contradicts to the case when you have two <br> elements like <div>foo<br><br></div> because in this case there's two lines ('foo' and empty line). So it seems the rule is (lines count = br's number - 1) which is strange with me since it's not clear where that -1 comes from.

so I have the same question why the original example shouldn't be treated as two lines?

Comment 16

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

The rules are:
1. Every line has to have some content.
2. When a <br> occurs, it is considered to be on the same line as any non-<br> content before it.

(I haven't checked the spec lately, but Chrome treats this the same as Gecko. See e.g. data:text/html,<div style='border:1px solid black'>Hello<br></div> .)

So in this case:
<div>Hello<br></div>
The <br> is the last node on the first line, and there is no second line.

In this case:
<div>Hello<br><br></div>
The first <br> is the last node on the first line. The second line contains the second <br> only. The <br> counts as content for rule 1.

Comment 17

[alexander :surkov (:asurkov)]

ok, thank you, as long as these rules are applied the behavior is reasonable. so we need a fix on a11y side since it's treated as '\n' char and AT things there's one line more.

Comment 18

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 19

[alexander :surkov (:asurkov)]

Attachment: patch

(failures fixed)

Comment 20

[Trevor Saunders (:tbsaunde)]

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

>+  TreeWalker walker(this, mContent);
>+  Accessible* child = nullptr;

nit, you could put this one in the while condition for smalelr scope right?

>+  Accessible* lastChild = nullptr;
>+  while ((child = walker.NextChild())) {
>+    if (lastChild)
>+      AppendChild(lastChild);
>+
>+    lastChild = child;
>+  }

It might be easier to just call Accessible::CacheChildren() and then see if the last child is a br and remove it if so

have you thought about a way to do this that doesn't involve the UnbindFromDocument() thing?

Comment 21

[alexander :surkov (:asurkov)]

It seems like it's ok to make content checks like if this is BR elm and it is last elm then ignore it. That can be part of your IsAcceptableChild() work. If I land it as is (this code will be changed by your IsAcceptableChild patch anyway) then is it ok?

Comment 22

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/94b0dd8ead0f

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/94b0dd8ead0f

Comment 24

[alexander :surkov (:asurkov)]

Jamie, Marco, it'd be great to give it a try before we request it backporting to aurora

Comment 25

[James Teh [:Jamie]]

I can verify that the issue is fixed in Firefox for the test case I attached and for the Web Console. However, it isn't fixed in Thunderbird (2013-09-12 nightly) as per the steps I provided in comment 0.

Comment 26

[alexander :surkov (:asurkov)]

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

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 882281
User impact if declined: text can't be read by screen reader at certain circumstances 
Testing completed (on m-c, etc.): yes
Risk to taking this patch (and alternatives if risky): fair risk
String or IDL/UUID changes made by this patch: no

Comment 27

[alexander :surkov (:asurkov)]

(In reply to James Teh [:Jamie] from comment #25)
> I can verify that the issue is fixed in Firefox for the test case I attached
> and for the Web Console. However, it isn't fixed in Thunderbird (2013-09-12
> nightly) as per the steps I provided in comment 0.

probably Thunderbird didn't pick up the change yet, somebody knows?

Comment 28

[Alex Keybl [:akeybl]]

(In reply to alexander :surkov from comment #26)
> Comment on attachment 801722 [details] [diff] [review]
> patch
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): bug 882281
> User impact if declined: text can't be read by screen reader at certain
> circumstances 
> Testing completed (on m-c, etc.): yes
> Risk to taking this patch (and alternatives if risky): fair risk
> String or IDL/UUID changes made by this patch: no

Can you define "fair risk"? We prefer low/moderate/high, and alternatives if possible. For instance, can we backout bug 882281?

Comment 29

[alexander :surkov (:asurkov)]

(In reply to Alex Keybl [:akeybl] from comment #28)
> (In reply to alexander :surkov from comment #26)
> > Comment on attachment 801722 [details] [diff] [review]
> > patch
> > 
> > [Approval Request Comment]
> > Bug caused by (feature/regressing bug #): bug 882281
> > User impact if declined: text can't be read by screen reader at certain
> > circumstances 
> > Testing completed (on m-c, etc.): yes
> > Risk to taking this patch (and alternatives if risky): fair risk
> > String or IDL/UUID changes made by this patch: no
> 
> Can you define "fair risk"? We prefer low/moderate/high, 

I'd define it as between low and moderate: a patch is not so trivial but changed code has more or less good test coverage, manual testing didn't reveal any problems.

> and alternatives if
> possible. For instance, can we backout bug 882281?

it's big meta bug and getting back to the previous state would probably be a bigger evil than this bug.

Comment 30

[Lukas Blakk [:lsblakk] use ?needinfo]

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

FF26 is now on Aurora and along with it, this fix.  If FF25 Beta is affected, please nominate for uplift - given the risk assessment we can probably take this in this early first/second week.

Comment 31

[alexander :surkov (:asurkov)]

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

[Approval Request Comment]
Bug caused by (feature/regressing bug #):  bug 882281
User impact if declined: text can't be read by screen reader at certain circumstances 
Testing completed (on m-c, etc.): yes
Risk to taking this patch (and alternatives if risky): between low and moderate risk: a patch is not so trivial but changed code has more or less good test coverage, manual testing didn't reveal any problems
String or IDL/UUID changes made by this patch: no

Comment 32

[Ryan VanderMeulen [:RyanVM]]

test_HTMLSpec.html doesn't exist on beta, so that change was omitted.

https://hg.mozilla.org/releases/mozilla-beta/rev/696aca164b58

Comment 33

[u279076]

Given this has in-testsuite coverage is there any need for QA testing around this fix?

Comment 34

[alexander :surkov (:asurkov)]

(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #33)
> Given this has in-testsuite coverage is there any need for QA testing around
> this fix?

Not necessary I think, James from NVDA confirmed the fix: comment #25

Comment 35

[u279076]

Thank you Alex.