Closed
Bug 706335
Opened 13 years ago
Closed 13 years ago
Accessible text breaks when quoting header is removed from plain text reply
Categories
(Core :: Disability Access APIs, defect)
Tracking
()
VERIFIED
FIXED
mozilla11
People
(Reporter: Jamie, Assigned: surkov)
References
(Blocks 1 open bug)
Details
Attachments
(2 files)
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.
Assignee | ||
Comment 1•13 years ago
|
||
Confirm. Cached text doesn't get updated.
Reporter | ||
Comment 2•13 years ago
|
||
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.
Assignee | ||
Comment 3•13 years ago
|
||
Assignee | ||
Updated•13 years ago
|
Severity: normal → major
Assignee | ||
Comment 4•13 years ago
|
||
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?
Assignee | ||
Comment 5•13 years ago
|
||
Btw, thunderbird uses white-space: pre-wrap; style which should be equivalent to HTML pre element in means of text frames handling.
Assignee | ||
Comment 6•13 years ago
|
||
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•13 years ago
|
||
> 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?
Updated•13 years ago
|
Attachment #577907 -
Attachment is patch: false
Attachment #577907 -
Attachment mime type: text/plain → text/html
Comment 8•13 years ago
|
||
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.
Assignee | ||
Comment 9•13 years ago
|
||
(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•13 years ago
|
||
> 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.
Assignee | ||
Comment 11•13 years ago
|
||
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #578269 -
Flags: review?(marco.zehe)
Attachment #578269 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 12•13 years ago
|
||
(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•13 years ago
|
||
Comment on attachment 578269 [details] [diff] [review]
patch
r=me for the test part in a11y. BTW I think ReflowTextA11ySpokesman is a funny name. ;-)
Attachment #578269 -
Flags: review?(marco.zehe) → review+
Comment 14•13 years ago
|
||
> 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•13 years ago
|
||
Comment on attachment 578269 [details] [diff] [review]
patch
s/Spokesman/Notifier/ and add the right NS_STACK_CLASS annotation, and r=me
Attachment #578269 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 16•13 years ago
|
||
inbound land with bz comments addressed http://hg.mozilla.org/integration/mozilla-inbound/rev/7319edc477a3
Comment 17•13 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
Comment 18•13 years ago
|
||
Verified fixed in Mozilla/5.0 (Windows NT 6.1; WOW64; rv:11.0a1) Gecko/20111204 Firefox/11.0a1
Status: RESOLVED → VERIFIED
Flags: in-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•