947170 - Regression: atk_text_get_text_at_offset broken for last object when closing tag is preceded by newline char

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Joanmarie Diggs]

Attachment: test case

Steps to reproduce:
1. Load the attached test in Firefox
2. Enable caret navigation (F7)
3. Launch the attached accessible-event listener in a terminal
4. Place the caret in the document text and arrow amongst its characters

Expected results: The line of text containing the caret, start offset, and end offset would appear in the terminal as you arrow around.

Actual results: An empty string with start and end offsets of 0 appear.

This regression was introduced in the 27 July nightly build:
  * 26 july: ('Hello world ', 0, 12)
  * 27 july: ('', 0, 0)

If you modify the test case so that the closing </p> is on the same line as the text, this bug does not occur. Similarly, if you add text after that paragraph the bug does not occur.

Comment 1

[Joanmarie Diggs]

Attachment: accessible-event listener

Comment 2

[alexander :surkov (:asurkov)]

so the problem here the trailing '\n' character is rendered as space but when we try to do nsIFrame::PeekOffset eSelectEndLine then it returns len - 1 because of the code
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#6353 (the comment says: "Do not position the caret after the terminating newline if we're trying to move to the end of line (see bug 596506)"). Maybe it should have some extra check like StyleText()->NewlineIsSignificant() from above or something else?

Robert, do you have ideas?

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes, we should check NewlineIsSignificant. Everywhere we call HasTerminalNewline and don't check NewlineIsSignificant is a bug. Really we should just make a new method HasSignificantTerminalNewline which does both checks, and convert all HasTerminalNewline callers to it. I'd gladly accept a patch for that!

Comment 4

[alexander :surkov (:asurkov)]

Robert, what about http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsTextFrame.cpp#7455 where WhiteSpaceIsSignificant() is used instead?

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That looks like a bug.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Filed (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #3)
> Really we
> should just make a new method HasSignificantTerminalNewline which does both
> checks, and convert all HasTerminalNewline callers to it. I'd gladly accept
> a patch for that!

Filed bug 953438 with patch.

Comment 7

[alexander :surkov (:asurkov)]

thank you! I'll add a11y test when patch is landed

Comment 8

[alexander :surkov (:asurkov)]

Attachment: mochitest patch

Comment 9

[alexander :surkov (:asurkov)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/9022f0cacaed

Comment 10

[alexander :surkov (:asurkov)]

(In reply to alexander :surkov from comment #9)
> http://hg.mozilla.org/integration/mozilla-inbound/rev/9022f0cacaed

a mochitest added,

fixed by bug 953438

Comment 11

[(PTO June 19-July 7) Ryan VanderMeulen]

https://hg.mozilla.org/mozilla-central/rev/9022f0cacaed