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

RESOLVED FIXED in mozilla1.9alpha1

Status

()

Core
Layout
RESOLVED FIXED
13 years ago
11 years ago

People

(Reporter: baffclan, Assigned: Uri Bernstein (Google))

Tracking

(Blocks: 2 bugs, {testcase})

Trunk
mozilla1.9alpha1
testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments)

(Reporter)

Description

13 years ago
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.
(Assignee)

Comment 1

13 years ago
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: testcase
OS: Windows XP → All
Hardware: PC → All
Summary: Caret skips blank line using rigth-arrow key in preformatted text (e.g. View Source) → Caret skips blank lines using right-arrow key in some cases of preformatted text (e.g. View Source)
(Assignee)

Comment 2

13 years ago
Created attachment 201427 [details]
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.
(Assignee)

Comment 3

13 years ago
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.
(Assignee)

Comment 4

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

Comment 5

12 years ago

*** This bug has been marked as a duplicate of 85505 ***
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → DUPLICATE
(Assignee)

Comment 6

12 years ago
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

12 years ago
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).
(Assignee)

Comment 8

12 years ago
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.
Status: RESOLVED → REOPENED
Depends on: 85505
Resolution: DUPLICATE → ---
(Assignee)

Updated

12 years ago
Assignee: aaronleventhal → nobody
Status: REOPENED → NEW
Component: Keyboard: Navigation → Layout
QA Contact: keyboard.navigation → layout
(Assignee)

Comment 9

12 years ago
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...
Severity: minor → normal
(Assignee)

Comment 10

12 years ago
Created attachment 216912 [details] [diff] [review]
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.
Attachment #216912 - Flags: superreview?(roc)
Attachment #216912 - Flags: review?(roc)
(Assignee)

Updated

12 years ago
Assignee: nobody → uriber
(Assignee)

Updated

12 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

12 years ago
Blocks: 92921
Attachment #216912 - Flags: superreview?(roc)
Attachment #216912 - Flags: superreview+
Attachment #216912 - Flags: review?(roc)
Attachment #216912 - Flags: review+
(Assignee)

Comment 11

12 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9alpha
(Assignee)

Updated

12 years ago
Blocks: 85505
No longer depends on: 85505
(Assignee)

Comment 12

11 years ago
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. 
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 13

11 years ago
Created attachment 248291 [details] [diff] [review]
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.
Attachment #248291 - Flags: review?(roc)
Attachment #248291 - Flags: superreview+
Attachment #248291 - Flags: review?(roc)
Attachment #248291 - Flags: review+
(Assignee)

Comment 14

11 years ago
Checked in:

/cvsroot/mozilla/layout/generic/nsTextFrame.cpp,v  <--  nsTextFrame.cpp
new revision: 1.603; previous revision: 1.602
Status: REOPENED → RESOLVED
Last Resolved: 12 years ago11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.