Last Comment Bug 706335 - Accessible text breaks when quoting header is removed from plain text reply
: Accessible text breaks when quoting header is removed from plain text reply
Status: VERIFIED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: x86_64 Windows 7
: -- major (vote)
: mozilla11
Assigned To: alexander :surkov
:
: alexander :surkov
Mentors:
Depends on:
Blocks: texta11y
  Show dependency treegraph
 
Reported: 2011-11-29 18:09 PST by James Teh [:Jamie]
Modified: 2011-12-04 07:44 PST (History)
5 users (show)
mzehe: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (753 bytes, text/html)
2011-11-30 02:56 PST, alexander :surkov
no flags Details
patch (5.70 KB, patch)
2011-12-01 08:05 PST, alexander :surkov
bzbarsky: review+
mzehe: review+
Details | Diff | Splinter Review

Description James Teh [:Jamie] 2011-11-29 18:09:07 PST
This bug is really obscure and I can't even speculate as to what's going wrong here, so I'm just going to provide the steps to reproduce, as strange as they are. :) Tested with Thunderbird daily (Mozilla/5.0 (Windows NT 6.1; WOW64; rv:11.0a1) Gecko/20111128 Thunderbird/11.0a1).

Preparation:
1. Start Thunderbird.
2. Make sure there is at least one account configured with at least one message containing at least three lines.
3. Configure the test account's composition and addressing settings (via Tools menu -> Account Settings -> the test account -> Composition & Addressing)as follows:
Compose messages in HTML format: unchecked
Automatically quote the original message when replying: checked
Then: start my reply above the quote
and place my signature: below the quote (recommended)
Include signature for replies: checked

Steps to reproduce:
1. Open a message in the test account as configured above.
2. Reply to the message.
3. Ensure you are focused in and at the top of the body of the reply.
4. Press down arrow twice to move to the quoting header line (which will be something like "On 30/11/2011 10:54 AM, foo bar wrote:").
5. Press shift+downArrow then delete to remove this quoting header line.
The caret will now be located at the start of the line after the line just deleted.
6. Obtain the accessible for the reply body.
7. Retrieve the entire text of this accessible.
Expected: The quoting header line should not be present.
Actual: The quoting header line is still present in the text.
8. Query the caret offset.
9. Retrieve the line offsets for the caret offset retrieved in step 8.
Expected: The start offset should be the caret offset from step 8.
Actual: The offsets encompass a line either before or after the current line or perhaps even multiple lines, depending on the message.
From this point on in the text, line and word offsets continue to break in weird and fascinating ways. Sometimes, pressing home will result in one caret offset, but pressing rightArrow then leftArrow (which should move back to the same character) result in a different caret offset.

I suspect this is a regression which occurred some time in the last few months (or maybe even weeks), but I'm not certain of that yet.
Comment 1 alexander :surkov 2011-11-30 02:23:35 PST
Confirm. Cached text doesn't get updated.
Comment 2 James Teh [:Jamie] 2011-11-30 02:32:51 PST
Deleting the quoting header line is not the only thing that triggers this. I've also seen it while replying inline to a quoted message. I can't figure out how to reproduce this reliably, though.
Comment 3 alexander :surkov 2011-11-30 02:56:43 PST
Created attachment 577907 [details]
testcase
Comment 4 alexander :surkov 2011-11-30 09:03:23 PST
The problem is text frame for text node having no data is not removed here (both in Thunderbird case and in test case) and we are not notified by nsTextFrame::ReflowText that text frame doesn't have any rendered text anymore.

1) I'm not sure why text frame doesn't get removed in Thunderbird case. I assume it doesn't get removed in attached testcase because the text is wrapped by HTML pre element. And the question, why does it make sense to keep frame for text nodes having no text?

2) If there's a point to keep these text frames then how can nsTextFrame::ReflowText be fixed to handle the case of no rendered text?
Comment 5 alexander :surkov 2011-11-30 09:07:24 PST
Btw, thunderbird uses white-space: pre-wrap; style which should be equivalent to HTML pre element in means of text frames handling.
Comment 6 alexander :surkov 2011-11-30 09:49:30 PST
Marco, can you figure out please where we regressed? Was it Firefox 4 or later? If later then it'd be nice to have regression range.
Comment 7 Boris Zbarsky [:bz] (still a bit busy) 2011-11-30 13:41:58 PST
> why does it make sense to keep frame for text nodes having no text?

We drop such frames in some cases when whitespace is not being preserved.  We could do that optimization for preserved-whitespace frames that correspond to text nodes with no text in them, but that's a rare case on the web, so we haven't worried about it.

> then how can nsTextFrame::ReflowText be fixed to handle the case of no rendered text?

In what sense does it need to handle it?
Comment 8 Marco Zehe (:MarcoZ) 2011-12-01 00:03:15 PST
Jamie, is there a way within NVDA's Python console to reproduce the bug? Simply following the steps in the test case, e. g. deleting the quote header line, does correctly update NVDA's view of the world for me. The text is gone, and as far as I can see from object navigation, the accessible containing that portion of text, too. So right now it appears I cannot reproduce with nightly.
Comment 9 alexander :surkov 2011-12-01 00:26:22 PST
(In reply to Boris Zbarsky (:bz) from comment #7)
> > why does it make sense to keep frame for text nodes having no text?
> 
> We drop such frames in some cases when whitespace is not being preserved. 
> We could do that optimization for preserved-whitespace frames that
> correspond to text nodes with no text in them, but that's a rare case on the
> web, so we haven't worried about it.

Should I file bug for this?

> > then how can nsTextFrame::ReflowText be fixed to handle the case of no rendered text?
> 
> In what sense does it need to handle it?

ReflowText notifies a11y in the end of it that text might be changed, http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsTextFrameThebes.cpp#7616. In this case we don't get notification so I'm guessing ReflowText returns early. Can you think of where I could add extra notification to handle this case?
Comment 10 Boris Zbarsky [:bz] (still a bit busy) 2011-12-01 00:42:19 PST
> Should I file bug for this?

The complexity of adding a check for this case probably outweighs any benefits from the change....

> so I'm guessing ReflowText returns early

Yep.  This part:

7116   // We don't need to reflow if there is no content.
7117   if (!maxContentLength) {
7118     ClearMetrics(aMetrics);
7119     aStatus = NS_FRAME_COMPLETE;
7120     return;
7121   }

The other early return from ReflowText can be if textrun creation fails, and from Reflow() if we don't have a line layout...  Not sure whether you care about those cases.  It seems like the simplest thing would be to use an RAII helper in ReflowText that notifies the a11y service in its destructor, so you don't have to whack-a-mole early returns.
Comment 11 alexander :surkov 2011-12-01 08:05:43 PST
Created attachment 578269 [details] [diff] [review]
patch
Comment 12 alexander :surkov 2011-12-01 08:07:07 PST
(In reply to Boris Zbarsky (:bz) from comment #10)

> The other early return from ReflowText can be if textrun creation fails, and
> from Reflow() if we don't have a line layout...

Could you give an exampe please? I tried things like
<table>hello<tr><td>cell</td></table> but text node gets out of table in DOM tree.
Comment 13 Marco Zehe (:MarcoZ) 2011-12-01 08:10:17 PST
Comment on attachment 578269 [details] [diff] [review]
patch

r=me for the test part in a11y. BTW I think ReflowTextA11ySpokesman is a funny name. ;-)
Comment 14 Boris Zbarsky [:bz] (still a bit busy) 2011-12-01 10:45:26 PST
> Could you give an exampe please?

Nope.  The comment there about tables doesn't make much sense, and I can't think of situations where we wouldn't have a line layout offhand...
Comment 15 Boris Zbarsky [:bz] (still a bit busy) 2011-12-01 12:21:47 PST
Comment on attachment 578269 [details] [diff] [review]
patch

s/Spokesman/Notifier/  and add the right NS_STACK_CLASS annotation, and r=me
Comment 16 alexander :surkov 2011-12-02 00:56:19 PST
inbound land with bz comments addressed http://hg.mozilla.org/integration/mozilla-inbound/rev/7319edc477a3
Comment 17 Ed Morley [:emorley] 2011-12-02 12:08:51 PST
https://hg.mozilla.org/mozilla-central/rev/7319edc477a3
Comment 18 Marco Zehe (:MarcoZ) 2011-12-04 07:44:24 PST
Verified fixed in Mozilla/5.0 (Windows NT 6.1; WOW64; rv:11.0a1) Gecko/20111204 Firefox/11.0a1

Note You need to log in before you can comment on or make changes to this bug.