314519 - Caret skips blank lines using right-arrow key in some cases of preformatted text (e.g. View Source)

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[baffclan]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051031 Firefox/1.6a1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051031 Firefox/1.6a1

In the view source window, caret skips blank line by right arrow key.

Reproducible: Always

Steps to Reproduce:
1. open http://www.mozilla.org/
2. open source window
3. click Line 20 left position and display caret
4. press right arrow key eight times

Actual Results:  
caret moves to the 22th line.


Expected Results:  
caret moves to the 21th line.
don't skip the blank line.

Comment 1

[Uri Bernstein]

This happens in "formatted" preformatted text, when the line below (above) the blank line the caret should move into start (ends) with a formatting tag (such as <i>).

I'll attach a minimal testcase shortly.

Comment 2

[Uri Bernstein]

Attachment: testcase

Moving from the first/third line of "x"s using right/left arrow skips the blank line. Moving from the second line in either direction works correctly.

Comment 3

[Uri Bernstein]

This comes down to the fact that nsTextFrame::PrepareUnicodeText() returns "0" for aTextLen when called with the newline frame from PeekOffset(). This causes PeekOffset to skip this frame and go into the next frame (look for a comment saying "if no renderable length, you cant park here").

The code in PrepareUnicodeText() is complicated and ancient. I don't think I'm going to be looking into this in the foreseeable future.

Comment 4

[Uri Bernstein]

RBS - care to take a look and say if you think PrepareUnicodeText() is doing the right thing? (see comment #3)

Comment 5

[rbs]

*** This bug has been marked as a duplicate of 85505 ***

Comment 6

[Uri Bernstein]

Actually, I don't think this is a duplicate of bug 85505. That bug is about places where the caret can't be at all. But here the caret can easily be placed in the blank line (e.g. using up-arrow, down-arrow, left(right)-arrow) - just not using one specific arrow key.
I think that at least in principle, this bug is fixable without fixing 85505. Am I wrong?

Comment 7

[rbs]

Point 1) in bug 85505 is a bit overstated. I think the reporter meant "get the caret to work _properly_" (rather than "get the caret to work _at all_"), as he first said in bug 92921 comment 5.

In principle, this bug can be fixed on its own indeed (using. e.g., some tricks to re-orient the selection/caret as we did in bug 15364 & bug 205006). Feel free to re-open if you think that it deserves separate cycles.

Since it was filed in the view-source context, which is quite low on the radars, I marked it as a dup because it is strategically better to do it under bug 85505, which is not view-source specific.

As you see from bug 92921 and bug 85505, many people will rejoice if this bug fixed. Most of those who are going to be happy are not view-source users -- it is always possible to put the caret anywhere in the viewsource window with Go to Line (Ctrl+L).

Comment 8

[Uri Bernstein]

I think the summary and the testcase make it clear that this is not viewsource specific (viewsource is just an example).
Since this bug is very specific and has a testcase, I'll re-open it and make it depend on 85505.

Comment 9

[Uri Bernstein]

Fixing bug 331444 made this now happen consistently:
blank lines in (non-textarea) preformatted text are now always skipped when using left/right arrows.
The fact that some cases worked before was just due to unintentional hint-flipping, as far as I can tell.

However, patch coming up...

Comment 10

[Uri Bernstein]

Attachment: patch

Preformatted empty text frames between lines should not be skipped.
I found that IsEmpty() tests for the preformatted case, and also is apparently more efficient than the current method of testing for emptyness, so I used it.

Comment 11

[Uri Bernstein]

Checked in:

Checking in layout/generic/nsTextFrame.cpp;
/cvsroot/mozilla/layout/generic/nsTextFrame.cpp,v  <--  nsTextFrame.cpp
new revision: 1.569; previous revision: 1.568
done

Comment 12

[Uri Bernstein]

For some reason, I effectively backed out the patch to this bug in my patch for bug 300131. That, naturally, regressed this bug.

I'm re-opening, and unless I can find out why I reverted this change, I'll attach an up-to-date patch to restore it. 

Comment 13

[Uri Bernstein]

Attachment: patch, again

So, I don't know what I was thinking when I changed this in bug 300131.
This is basically the same as the original patch, adjusted for bug 300131.

Comment 14

[Uri Bernstein]

Checked in:

/cvsroot/mozilla/layout/generic/nsTextFrame.cpp,v  <--  nsTextFrame.cpp
new revision: 1.603; previous revision: 1.602