If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Accessibility returns empty line for last line in certain cases

RESOLVED FIXED in Firefox 25

Status

()

Core
Disability Access APIs
--
major
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Jamie, Assigned: surkov)

Tracking

(Blocks: 1 bug, {regression})

Trunk
mozilla26
x86_64
All
regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox25 fixed, firefox26 fixed)

Details

(Whiteboard: [qa-])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

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

Updated

4 years ago
Blocks: 613857
(Assignee)

Comment 1

4 years ago
seems to be similar to bug 873358, I see HTML br under input
(Reporter)

Comment 2

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

Comment 3

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

Updated

4 years ago
Depends on: 900943
(Assignee)

Comment 4

4 years ago
filed bug 900943 for textbox case, let's keep this bug for comment #2 case.
(Assignee)

Comment 5

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

Comment 6

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

Comment 7

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

Comment 8

4 years ago
Created attachment 786602 [details]
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).
(Assignee)

Comment 9

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

Comment 10

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

Comment 11

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

Updated

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

Comment 13

4 years ago
(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?
(Assignee)

Comment 15

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

Comment 17

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

Comment 18

4 years ago
Created attachment 801639 [details] [diff] [review]
patch
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #801639 - Flags: review?(trev.saunders)
(Assignee)

Comment 19

4 years ago
Created attachment 801722 [details] [diff] [review]
patch

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

Comment 21

4 years ago
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?
(Assignee)

Updated

4 years ago
Severity: normal → major
OS: Windows 7 → All
(Assignee)

Comment 22

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/94b0dd8ead0f
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/94b0dd8ead0f
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
(Assignee)

Comment 24

4 years ago
Jamie, Marco, it'd be great to give it a try before we request it backporting to aurora
Flags: needinfo?
(Reporter)

Comment 25

4 years ago
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?
(Assignee)

Comment 26

4 years ago
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?
(Assignee)

Comment 27

4 years ago
(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?
(Assignee)

Comment 29

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

Comment 31

4 years ago
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?

Updated

4 years ago
Attachment #801722 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(Assignee)

Updated

4 years ago
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
status-firefox25: --- → fixed
status-firefox26: --- → fixed
Keywords: checkin-needed
Given this has in-testsuite coverage is there any need for QA testing around this fix?
Flags: needinfo?(surkov.alexander)
(Assignee)

Comment 34

4 years ago
(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.