335560 - Caret disappears at right edge of wrapping line in textarea

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Uri Bernstein]

When a line in a textarea wraps exactly at the right edge of the textarea, placing the caret at the point where the line wraps causes the caret to disappear.

This regressed between 2006-04-19 and 2006-04-20, almost certainly due to bug 235223.

Testcase coming up.

Comment 1

[Uri Bernstein]

Attachment: testcase

Placing the caret after the third "d" causes it to disappear.

Comment 2

[Benjamin Smedberg]

-> dbaron per regression

Comment 3

[Niro]

in this attachment:
https://bugzilla.mozilla.org/attachment.cgi?id=134102&action=view
the caret disappears if you'll try to place it after the comma, and it has the same regression range.
For some reason I could not reproduce it on Uri's testcase.

Comment 4

[chris hofmann]

is this fixed now?  using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060525 Minefield/3.0a1 I only see the carat disappear if I space to the right of the "ddd".   just placing the carat after the "ddd" it's still visable for me.

Comment 5

[Uri Bernstein]

(In reply to comment #4)
> is this fixed now?  using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US;
> rv:1.9a1) Gecko/20060525 Minefield/3.0a1 I only see the carat disappear if I
> space to the right of the "ddd".   just placing the carat after the "ddd" it's
> still visable for me.
> 

Still broken for me with:

Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20060526 Minefield/3.0a1 ID:2006052605

Placing the caret after the third "d" using any method causes the caret to disappear.
The behavior on XP might be slightly different due to a difference in font size and/or exact textarea width, but if you can cause the caret to disappear by placing it somewhere near the end of the line, I think what you're seeing is this bug.

Comment 6

[Uri Bernstein]

The build I'm seeing this in is non-Cairo, so I'm not sure why this should block bug 334732.

Comment 7

[chris hofmann]

ok. removed dependacy.

Comment 8

[Aaron Leventhal]

I'm seeing this on Windows as well, but not with this testcase. I'm going to attach 2 image files which show the reason. Under trunk builds, the textarea is being rendered in a different place, the x & y position are both larger by 1.

Comment 9

[Aaron Leventhal]

Attachment: Firefox 1.5 text area screenshot

Comment 10

[Aaron Leventhal]

Attachment: Firefox 3 text area screenshot

If you open both of these in tabs and make sure they're fully zoomed in, you'll see that the Firefox 3 textarea's text is down and to the right by 1 pixel.

I believe this is why the caret no longer fits in the view. It also seems to be wreaking havoc with the screen reader's offscreen model, but that's another story.

Comment 11

[Mike Cowperthwaite]

(In reply to comment #10)
> I believe this is why the caret no longer fits in the view.

I don't think this is the case.  Looking at the text area from a b.m.o bug in a current Seamonkey 1.5a, if I fill a line with characters such that a nonprintable one is in column 84, I can see the caret; then if I type space, it's not visible.  This is because the logical cursor position is at the end of the line, after the trailing space.  This is more obvious if you test in a plain-text message-compose window, which has visible rendering area beyond the wrap point at column 72 (or whatever your pref is set to) -- see bug 363624.

I use Opera to work in b.m.o and the textarea wrap behavior there is slightly different: if you've got a end-of-paragraph printable character in the last column, typing a space forces a wrap at the beginning of the previous word.  This means that there is always a space in the last column unless the printable character is immediately followed by a newline.  (I think this behavior is new to Opera 9.)  It works for me.

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #11)
> This is because the logical cursor position is at the end of
> the line, after the trailing space.

More precisely, the problem here is that we end up trying to draw the caret outside of its scrollframe, which clips the caret when it tries to draw too far outside. I'm hoping to work on this bug in about a week (after final exams are over).

Comment 13

[Mike Cowperthwaite]

(In reply to comment #12)
> I'm hoping to work on this bug in about a week

OK, but please note that I *want* the current behavior for plain-text message compose; the trunk handles this better than the branch when no clipping is involved (arguably, even when clipping *is* involved).  Again, see bug 363624.

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix

As promised, this clamps the caret to be within its nearest scrollframe's rect.

Comment 15

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

Does this actually work when there's a vertical scrollbar? I don't see how. We should probably be looking at the scroll port rect.

If we have lots of content overflowing to the right and I place the caret at the end of it in a way that doesn't trigger scroll-into-view (say because the content is overflow:hidden), this is going to drag the caret back into view, an abitrary distance, right? Is that really what we want to do?

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

There are two problems that this patch is attempting to fix (though I didn't realize what the scrollframe's width was actually returning, vertical scrollbars do break it):

- At the end of a line in a textarea typing spaces makes the caret disappear. This is because when we ask the textframe for an offset, it gives us the offset for the last space in the textframe, even if it's collapsed, so we try to draw outside of the scrollframe, which clips us.

- For some reason, we don't scroll enough when we can't wrap at all (due to a single line textarea or an unbreakable word). The old code used to do a crazy dance with a clipview. This makes a little more sense, although perhaps it's possible to teach textareas to take trimmed whitespace into account when asking for an position given an offset.

Comment 17

[Damon Sicore (:damons)]

Targeting to B1 per conversation with Blake.

Comment 18

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

Is this still an issue with the new textframe?

Comment 19

[Blake Kaplan (:mrbkap) (inactive)]

It is for single line textfields. The scrollable view refuses to scroll far enough to show the caret.

Comment 20

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

+  // Clamp our position to be within our scroll frame. If we don't, then it
+  // clips us, and we don't appear at all. See bug 335560.
+  for (nsIFrame *scrollFrame = aFrame;
+       scrollFrame;
+       scrollFrame = scrollFrame->GetParent())
+  {
+    if (scrollFrame->GetType() != nsGkAtoms::scrollFrame)
+      continue;

Use nsLayoutUtils::GetClosestFrameOfType instead?

This doesn't prevent the caret from landing under a scrollbar, does it? Should you perhaps be dealing with the scroll port frame (via nsIScrollableFrame::GetScrolledFrame)?

Comment 21

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Another offering

I have to admit that I don't quite follow why this works (I'm not sure I worry that I'm getting lucky with my test pages), but it does appear to work.

Comment 22

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

I think I misled you :-(. This isn't right when content overflows horizontally, because the scrolled-frame's right edge is not at the edge of the scrollport.

Should have stuck with the scrollframe and use nsIScrollableFrame::GetScrollableView(); its bounds are the bounds of the viewport, relative to the scrollframe (because the scrollframe always has a view).

Comment 23

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Like this?

This also seems to work. Do the comments make sense?

Comment 24

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

+    // calculate the caret's coordinatest in the same coordinate space.

coordinates

+    // The view's bounds are in the scrollFrame's coordinate space

No they're not. 'view's origin is relative to the origin of the nsIScrollableView, which is not the same as the scrollframe's origin. You need to add/subtract view->GetOffsetTo(scrollFrame->GetView()).

+    if (caretInScroll.XMost() > view->GetBounds().XMost())

How about writing
     nscoord overflow = caretInScroll.XMost() - view->GetBounds().XMost();
     if (overflow > 0) {
          mCaretRect.x -= overflow;
     }
?

Comment 25

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

> 'view's origin is relative to the origin of the
> nsIScrollableView, which is not the same as the scrollframe's origin.

For example, it's not the same when the scrollframe has a border. You might want to test this.

Comment 26

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: So, does this work?

Testing in inputs with borders still works, not sure if I can get a border any further into edit fields.

Comment 27

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

Comment on attachment 275833 [details] [diff] [review]
So, does this work?

This looks good. You could test a non-input scrollframe using caret browsing or content-editable (although caret browsing might be broken on trunk currently --- I can't get it to work on Mac)

Comment 28

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into trunk.

Comment 29

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1527381

Comment 30

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1527381