268352 - Text cursor is placed on wrong position when typing too much in an input field

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Volker Moell]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8a5) Gecko/20041107 Mnenhy/0.6.1.10012
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8a5) Gecko/20041107 Mnenhy/0.6.1.10012

If you type more text in an <input type=text> field than it visually fits and
delete some chars from behind with backspace, you are not able to place the text
cursor via mouse-click at the end of the text

Reproducible: Always
Steps to Reproduce:
1.Type in on http://google.de/ a longer text as the input field fits
2.Press Backspace a few times
3.Click on the white space at the end of the text to place the text cursor at
the end of the text.

Actual Results:  
The text cursor appears somewhere in the middle of the text.

Expected Results:  
The text cursor should appear at the end of the text.

There is another side effect:
When deleting all of the visible characters with backspace, the cursor
disappears at the beginning of the box and the text is not scrolling horizontal.

This bug is older than 2004-09-30 (which is the oldest version I could test).
And it seems to be platform undependend: On my Linux Mozilla
--> Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8a5) Gecko/20041107
Mnenhy/0.6.1.10012
I see thes bug, and Juergen Harter sees it on 
--> Mozilla/5.0 (Windows; U; Windows NT 5.1; de-AT; rv:1.8a5) Gecko/20041020
Mnenhy/0.6.1.10010

Comment 1

[Felix Miata]

I too see this in current Linux trunk. After hitting HOME, it takes 14 RIGHT
ARROW before I see cursor again.

Comment 2

[Felix Miata]

I  never touched the mouse to cause the behavior described in comment 1 (not
mouse action dependent).

Comment 3

[Volker Moell]

(In reply to comment #2)
> I  never touched the mouse to cause the behavior described in comment 1 (not
> mouse action dependent).

The main bug *is* mouse dependent, as I described it.
The *side effects* (mine and yours) are not mouse dependent.

Comment 4

[Martijn Wargers (dead)]

This sounds a bit like bug 265940, not?

Comment 5

[Felix Miata]

*** Bug 268490 has been marked as a duplicate of this bug. ***

Comment 6

[Volker Moell]

(In reply to comment #4)
> This sounds a bit like bug 265940, not?

The second part (the side effect), yes.  
But not the main part (placing cursor with mouse click).

Comment 7

[Mats Palmgren (inactive)]

The mouse-click problem is not fixed by bug 265940.

Comment 8

[Mats Palmgren (inactive)]

Bug also occurs in 1.7.2 and 1.6, but not in Mozilla 1.5 and older.

Comment 9

[Felix Miata]

From: https://bugzilla.mozilla.org/bug_status.html#severity

Minor:	minor loss of function, or other problem where easy workaround is present

I see no workaround, which means either normal severity, or even major. I tend
to think major, since the only way to fix it once encountered seems to be
reloading the whole page and starting all over. -> major

Comment 10

[Felix Miata]

Arguably this is blocking, as https://bugzilla.mozilla.org/query.cgi and editing
existing bugs is virtually impossible with builds that do this.

Comment 11

[Boris Zbarsky [:bzbarsky]]

For future reference, http://archive.mozilla.org/ has older builds... (though it
doesn't help too much in this case).

It looks like this regressed between 2003-11-10-05 and 2003-11-13-12.

In that range, bug 224013 is a possible candidate for causing this regression. 
So is bug 191699.

Full list of checkins in that date range:

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2003-11-10+05%3A00%3A00&maxdate=2003-11-13+12%3A00%3A00&cvsroot=%2Fcvsroot

Comment 12

[Boris Zbarsky [:bzbarsky]]

OK, backing out the bug 224013 fix fixes this bug.

The text control frame inherits from nsBoxFrame, so it gets its GetFrameForPoint
from there.  In this case, it looks like the div inside the text control is the
frame returned by GetFrameForPoint() in current builds, whereas in the old
builds it was the text control itself.

Now I can hack GetFrameForPoint on text controls to restore the old behavior,
but the question I have is what code is getting confused by the new behavior. 
In particular, why does this confuse editor?

Comment 13

[Boris Zbarsky [:bzbarsky]]

Simon, do you have any idea what the answer to the question in comment 12 might be?

Comment 14

[kinmoz]

I was talking to bz over AIM today, and he asked me to jot down some thoughts
about this bug. Keep in mind that I have no mozilla source tree in front of me,
and that it's been about a year since I last poked around in layout.

There seems to be 2 problems pointed out in the description of how to reproduce
this bug.

The 1st problem is that if the textfield has scrolled off to the right as you
type ... any backspaces that reduce the width of the scrolling view should cause
the view to scroll to the right, that is, you shouldn't be able to scroll past
the right side of the view.

The 2nd problem is more what this bug is about, and it looks to me as if the
code that is calculating the closest frame to the point where you clicked is not
properly translating the coordinates of the point as it pushes it down the frame
tree.

If I remember correctly, the upper left corner of a layout frame is relative to
its parent frame, unless it's parent frame has an internal view. If a parent
frame has an internal view, the upper left corner of it's child frames are
relative to the upper left corner of this view, not the parent frame.

My guess is that the code in question isn't taking into account this crossing of
view boundaries when moving down the frame tree ... you can tell this by looking
at where the caret is actually placed when you click in the blank area on the
right side of the textfield ... it gets placed in the content as if the content
was scrolled all the way to left.

The soonest I can try to help with this bug is sometime this weekend.

Comment 15

[kinmoz]

FYI, I did some poking around in bugs I fixed in the past and ran across Bug
83650 which contains a patch that demonstrates how to use

    nsFrame::GetOriginToChildViewOffset()

when pushing a point down the frame tree.

Comment 16

[Mats Palmgren (inactive)]

Attachment: fixes the scrolling problem

FWIW, if we fix the 1st problem (the scrolling) then I don't think problem 2
will occur. This patch fixes the scrolling problem...

Comment 17

[kinmoz]

Comment on attachment 170448 [details] [diff] [review]
fixes the scrolling problem

I still think we need to fix problem 2 because we know that code doesn't work
properly. Also, I think it's still might be possible to recreate the problem if
you add padding to the div or box frame that surrounds the actual scrolling
frame.

Comment 18

[kinmoz]

So I took a look at this bug tonight ... er this morning ... and I'm seeing some
really wacky point translations going on in the call chain triggered by the call 
to GetFrameForPoint() in nsPresShell::HandleEvent().

I'm cc'ing roc because it looks like he added the implementation of
nsBlockFrame::GetFrameForPointUsing(), and one of the problems I'm seeing is
that it is adding the originOffset it calculates to the point instead of
subtracting it, and it looks like the lineCombinedArea it compares the point
against is relative to the block frame and not the view contained inside the
block frame.

I have some changes in my local tree that I think straightens out some of the
wackiness but I think this bug is also due to the fact that once we find a frame
to target the event to, nsPresShell::HandleEvent() passes the aEvent directly to
the mCurrentEventFrame without ever translating the coordinates in the event to
account for the views that are between the targeted frame and frame in
nsPresShell::HandleEvent().

In fact, I see this comment in PresShell::HandleEventInternal():

      // 3. Give event to the Frames for browser default processing.
      // XXX The event isn't translated into the local coordinate space         
      // of the frame...                                                        
      if (GetCurrentEventFrame() && NS_SUCCEEDED (rv) &&
          aEvent->eventStructType != NS_EVENT) {
        rv = mCurrentEventFrame->HandleEvent(mPresContext, (nsGUIEvent*)aEvent,
                                             aStatus);
      }

Comment 19

[kinmoz]

Note to self ... stick to debugging when you are awake!

Duh, adding the originOffset is the right thing to do when moving down the frame
hierarchy.

The passing of the non-translated event point to mCurrentEventFrame is still
suspect though.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix coordinate systems

kin, thank you!  Fixing that one spot to transform coordinates did make this go
away (though the scrolling problem is still there, and we should still take
Mats' patch).

Robert, this fixes  this testcase, but I can't tell for sure whether it's
right... is the view that's passed to HandleEventInternal always the view that
aEvent->point is relative to?  Why's it ending up null sometimes?

Comment 21

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

Comment on attachment 173241 [details] [diff] [review]
Fix coordinate systems

HandleEventInternal gets called with a null view only by HandleEventWithTarget,
which is used to dispatch an event to some content. Maybe the point translation
doesn't matter in those cases --- I'd have to check.

HandleEventInternal is almost always called on the innermost view containing
the frame under the point, that's why this translation is normally not
necessary. I guess in this case mCurrentEventFrame is set funny.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 170448 [details] [diff] [review]
fixes the scrolling problem

Mats, why is this calling scrollTo?  Wouldn't we want to scrollBy the amount by
which viewRect.XMost() is less than portRect.width?

Also, what about the case when viewRect is simply narrower than portRect?

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 173241 [details] [diff] [review]
Fix coordinate systems

I checked this patch in.

Comment 24

[Mats Palmgren (inactive)]

(In reply to comment #22)
> Mats, why is this calling scrollTo?  Wouldn't we want to scrollBy the amount
> by which viewRect.XMost() is less than portRect.width?

There is no nsIScrollableView::ScrollBy method as far as can see.

> Also, what about the case when viewRect is simply narrower than portRect?

Can that really occur for a text control?

Comment 25

[Mats Palmgren (inactive)]

Attachment: Fix the scrolling, rev. 2

This takes care of view < port width also.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 173334 [details] [diff] [review]
Fix the scrolling, rev. 2

Yeah, ok.  This works.

Comment 27

[Mats Palmgren (inactive)]

Comment on attachment 173334 [details] [diff] [review]
Fix the scrolling, rev. 2

Checked in 2005-02-04 13:56 PDT

Comment 28

[Mats Palmgren (inactive)]

-> Boris

Comment 29

[Mats Palmgren (inactive)]

-> FIXED

Comment 30

[Boris Zbarsky [:bzbarsky]]

The coordinate systems patch caused bug 281424.