306070 - middle-click paste and drag/drop after end of line in scrolled textarea go to wrong place offset by scroll position

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Both middle-click paste and drag-drop to a blank line in a textarea that's
scrolled away from the top of the page paste the text into the wrong place,
seemingly offset by the scroll offset.

Buggy in:
 Linux debug+opt build, CVS pull, ~8am 2005-08-25

Steps to reproduce:
 1. Paste the following text in the "Additional Comments" field above:
0




5




10




15
 2. scroll the textarea to the bottom
 3. double click the word "blocks" above the text area
 4. Either middle-click at line 8 (X11-only) or drag the highlighted word to
line 8 (probably cross platform, but I haven't tested)

Expected results: word ends up on line 8

Actual results: word ends up on line 3.

Comment 1

[Uri Bernstein]

Probably a regression from bug 303399. Taking. I'll investigate it tomorrow.

Comment 2

[Uri Bernstein]

The drag case is reproducable on mac => All/All.

Comment 3

[Boris Zbarsky [:bzbarsky]]

This works in 2005-08-22-07 and is broken in 2005-08-23-07.  Bonsai 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=2005-08-22+07%3A00%3A00&maxdate=2005-08-23+07%3A00%3A00&cvsroot=%2Fcvsroot

I'll bet $5 this is the event->point change from bug 296036.

Though it could also be bug 305239, the "offset by scroll position" part makes
me think event coordinates.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This actually affects any paste after the end of the line, not just those on
blank lines (where all the space is after the end of the line).

Also, you may run into bug 240416 while testing drag/drop into textareas.  I did.

Comment 5

[Uri Bernstein]

(In reply to comment #3)
> I'll bet $5 this is the event->point change from bug 296036.
> Though it could also be bug 305239, the "offset by scroll position" part makes
> me think event coordinates.

Hmmm... I don't see how this can be 305239 - the code affected there should
never be entered in these cases. So my money goes with bz.

I guess I'm so used to causing regressions I volunteered for this one too
early... un-taking for now, until proven otherwise.

Comment 6

[Eli Friedman]

Taking.  I'm working on a fix, but I'm not sure how long it will take.

Comment 7

[Eli Friedman]

Attachment: Patch

The part that actually fixes this bug is the nsLayoutUtils change.  The other
change fixes a bug I found but haven't filed.

Comment 8

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

I'm not sure the nsLayoutUtils change is right. I had assumed the comment for
GetEventCoordinatesForNearestView was what we wanted:
* The "nearest view" is the view returned by nsFrame::GetOffsetFromView.

If you change that, I think you'll mess up other callers. See e.g.
http://lxr.mozilla.org/seamonkey/source/layout/generic/nsFrame.cpp#1504

Comment 9

[Eli Friedman]

I've been investigating it more carefully.  GetContentAndOffsetsFromPoint is
really expecting input in the GetClosestView coordinate system.  The place you
mention is the only other place that is calling that function after the patch
for Bug 305944 gets checked in.

As near as I can tell, the code you cited
(http://lxr.mozilla.org/seamonkey/source/layout/generic/nsFrame.cpp#1484) is
supposed to allow selection to work with clicks to the right of a
moz-user-select=all box when it's at the end of a line.  However, it doesn't
work.  Therefore, I'm in favor of replacing that whole chunk of code with
"disableDragSelect = PR_TRUE;" like the XXX comment suggests.  If you're OK with
that, I'll make a new patch that does that.

Comment 10

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

(In reply to comment #9)
> I've been investigating it more carefully.  GetContentAndOffsetsFromPoint is
> really expecting input in the GetClosestView coordinate system.

Actually, looking at the code, I believe GetContentAndOffsetsFromPoint is
confused about whether it takes GetClosestView-relative or
GetOffsetFromView-relative coordinates... A prime candidate for fixing :-)
Filed bug 306222. That's probably the root cause of this and other problems...

> The place you
> mention is the only other place that is calling that function after the patch
> for Bug 305944 gets checked in.

By "that function" I assume you mean GetEventCoordinatesForNearestView...
I see here:
http://lxr.mozilla.org/seamonkey/source/layout/generic/nsFrame.cpp#1752
http://lxr.mozilla.org/seamonkey/source/layout/generic/nsFrame.cpp#1760
as well as http://lxr.mozilla.org/seamonkey/source/layout/generic/nsFrame.cpp#1509.

> As near as I can tell, the code you cited
> (http://lxr.mozilla.org/seamonkey/source/layout/generic/nsFrame.cpp#1484) is
> supposed to allow selection to work with clicks to the right of a
> moz-user-select=all box when it's at the end of a line.  However, it doesn't
> work.  Therefore, I'm in favor of replacing that whole chunk of code with
> "disableDragSelect = PR_TRUE;" like the XXX comment suggests.  If you're OK with
> that, I'll make a new patch that does that.

Wow, ContentContainsPoint is totally bogus code!

I haven't done any testing (I'm at home without a trunk build) but if it's still
possible to select content by clicking outside it (e.g. beyond the end of a
line) then we still need code like this. Code that actually works, preferably
:-). An easy fix here that wouldn't make anything worse would be to just change
the call to GetOffsetFromView to GetClosestView at the same time as you change
the behaviour of GetEventCoordinatesFromNearestView.

But actually I fear the confusion in GetContentAndOffsetsFromPoint about the
interpretation of its aPoint parameter makes any change here very risky. Maybe
the safest thing to do is actually to go ahead and at least clean up the
coordinate handling in GetContentAndOffsetsFromPoint.

Comment 11

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

Actually http://lxr.mozilla.org/seamonkey/source/layout/generic/nsFrame.cpp#1511
can just be deleted since we're already setting 'view'.

More about the confusion in GetContentAndOffsetsFromPoint:
The code block at
http://lxr.mozilla.org/seamonkey/source/layout/generic/nsFrame.cpp#2023
is clearly understanding aPoint to be relative to GetClosestView().
Unfortunately, for example, the code starting
http://lxr.mozilla.org/seamonkey/source/layout/generic/nsFrame.cpp#2046 clearly
puts thisRect in coordinates relative to GetOffsetFromView()'s view and then
assumes aPoint is in the same coordinate space...
nsTextFrame::GetContentAndOffsetsFromPoint/GetPosition(Slowly) has similar
confusion.

I think the way to proceed in direction of strictly increasing sanity is to make
GetContentAndOffsetsFromPoint (and nsTextFrame::GetPosition(Slowly)) take
frame-relative coordinates. That will eliminate most uses of
GetEventCoordinatesFromNearestView and probably fix this bug. If we do that we
can leave GetEventCoordinatesFromNearestView and its other callers unchanged for
now. If that causes other regressions, at least we're moving in the direction of
having better-specified methods so we know we're making progress in the right
direction.

Comment 12

[Adam Guthrie]

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20050904 Firefox/1.6a1
ID:2005090415

The check-in for bug 306222 seems to fix this for me.

Comment 13

[Adam Guthrie]

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20050913 Firefox/1.6a1
ID:2005091311

This was fixed by bug 306222.