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)

x86_64
Windows 7
defect
Not set
major

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.
Confirm. Cached text doesn't get updated.
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.
Attached file testcase (deleted) —
Severity: normal → major
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?
Btw, thunderbird uses white-space: pre-wrap; style which should be equivalent to HTML pre element in means of text frames handling.
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.
> 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?
Attachment #577907 - Attachment is patch: false
Attachment #577907 - Attachment mime type: text/plain → text/html
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.
(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?
> 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.
Attached patch patch (deleted) — Splinter Review
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #578269 - Flags: review?(marco.zehe)
Attachment #578269 - Flags: review?(bzbarsky)
(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 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+
> 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 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+
Blocks: texta11y
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
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.

Attachment

General

Created:
Updated:
Size: