Closed Bug 899433 Opened 7 years ago Closed 6 years ago

Accessibility returns empty line for last line in certain cases

Categories

(Core :: Disability Access APIs, defect, major)

x86_64
All
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla26
Tracking Status
firefox25 --- fixed
firefox26 --- fixed

People

(Reporter: Jamie, Assigned: surkov)

References

(Blocks 1 open bug)

Details

(Keywords: regression, Whiteboard: [qa-])

Attachments

(2 files, 1 obsolete file)

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.
Blocks: getText*a11y
seems to be similar to bug 873358, I see HTML br under input
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.
(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
Depends on: 900943
filed bug 900943 for textbox case, let's keep this bug for comment #2 case.
(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?
(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>
(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?
Attached file 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).
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?
(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.
(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)
Flags: needinfo?(roc)
(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.
Flags: needinfo?(roc)
(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?
It doesn't render with two lines in Gecko. Why do you think it should be treated as two lines?
(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?
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.
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.
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #801639 - Flags: review?(trev.saunders)
Attached patch patchSplinter Review
(failures fixed)
Attachment #801639 - Attachment is obsolete: true
Attachment #801639 - Flags: review?(trev.saunders)
Attachment #801722 - Flags: review?(trev.saunders)
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?
Attachment #801722 - Flags: review?(trev.saunders) → review+
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?
Severity: normal → major
OS: Windows 7 → All
https://hg.mozilla.org/mozilla-central/rev/94b0dd8ead0f
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Jamie, Marco, it'd be great to give it a try before we request it backporting to aurora
Flags: needinfo?
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.
Flags: needinfo?
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
Attachment #801722 - Flags: approval-mozilla-aurora?
(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?
(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?
(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 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.
Attachment #801722 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
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
Attachment #801722 - Flags: approval-mozilla-beta?
Attachment #801722 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Keywords: checkin-needed
test_HTMLSpec.html doesn't exist on beta, so that change was omitted.

https://hg.mozilla.org/releases/mozilla-beta/rev/696aca164b58
Given this has in-testsuite coverage is there any need for QA testing around this fix?
Flags: needinfo?(surkov.alexander)
(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
Flags: needinfo?(surkov.alexander)
Thank you Alex.
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.