Wrong caret position in Report Bug dialog when focused due...

Wrong caret position in Report Bug dialog when focused due
to GetTextLength not proper for multiline textfields. Just use the given 
x since that is sufficient.
BUG=2368
(http://crbug.com/2368)

[Mohamed Mansour (USE mhm), 2008-11-30 19:04:08]

Hi, would like this to be reviewed! Explained in the issue
http://code.google.com/p/chromium/issues/detail?id=2368

[Peter Kasting, 2008-12-01 00:28:28]

Taking the review, since I wrote this.

When we wrote the TextField class, we only had single-line textfields.  I wonder
if more stuff is wrong with it than just this...

http://codereview.chromium.org/13008/diff/202/5
File chrome/views/text_field.cc (right):

http://codereview.chromium.org/13008/diff/202/5#newcode824
Line 824: const int right_bound = std::min(r.right, PosFromChar(x).x);
The old code is definitely wrong for multiline textfields, but the new code is
wrong in general.  PosFromChar(x) isn't meaningful, because x is a pixel
coordinate, not a character index.  And it's not always legal to return x if x
== 0.

It seems like you're going to have to worry about y-coordinates somehow.  I'm
not sure.  Perhaps in the multiline case, we can bypass all the ClipXCoord...()
processing entirely and just let the edit behave the default way?  You'd have to
check whether the "phantom newline" comes back, though.

[Mohamed Mansour (USE mhm), 2008-12-01 01:06:13]

Hey Peter, 

To tell you the truth, I don't know why we need ClipXCoordToVisibleText, I did a
test case where I pass just the x and y, and it worked great. I really don't
know why its needed. It worked in both cases. We could just do a bounds check if
its 0 or greater just in case.

Could you explain to me why this method is needed? And what does phantom-newline
mean?

BTW, I saw many issues with the TextField class for multi-line support, such as,
You cannot select upwards ( text ). Would it be better to introduce a TextArea,
instead of hacking this TextField  to work for both single and multiple.

[Peter Kasting, 2008-12-01 04:42:48]

On 2008/12/01 01:06:13, Mohamed Mansour wrote:
> Could you explain to me why this method is needed? And what does
phantom-newline
> mean?

:)

CRichEditCtrls have an annoying habit of inserting a virtual newline at the end
of the user's input.  This newline isn't included in the calculated text length,
but can be selected (it appears as a small amount of additional whitespace
selectable distinctly from the displayed text).  This crazy code is all designed
to prevent the user from ever seeing this "phantom newline", by modifying the
mouse messages passed to the CRichEditCtrl to appear to be clipped to only the
visible text.  As you can tell by the comments in some of the calling code, the
amount of hacky craziness required to implement this is rather large...

Getting this function wrong will result in effects like occasionally seeing the
right edge of the selection flicker due to the "phantom newline" being selected
(or not).

> BTW, I saw many issues with the TextField class for multi-line support, such
as,
> You cannot select upwards ( text ). Would it be better to introduce a
TextArea,
> instead of hacking this TextField  to work for both single and multiple.

I agree; at the very least we'd need to take a careful look at the existing
code.  I'm sort of frustrated that someone just used this class without paying
attention to its single-linedness :(

If you can note any other problems you see (such as the selection issue) on the
bug, perhaps we can morph it to "add TextArea or else refactor TextField to make
it work for multiline cases".

Of course, if this is the only case in the product we use this (and I think it
is), perhaps we can design a different solution that's less work than doing a
proper multiline edit.  And certainly getting it wrong is much lower impact than
other, more frequently-encountered bugs...

[Mohamed Mansour (USE mhm), 2008-12-01 05:20:04]

> 
> CRichEditCtrls have an annoying habit of inserting a virtual newline at the
end
> of the user's input.  This newline isn't included in the calculated text
length,
> but can be selected (it appears as a small amount of additional whitespace
> selectable distinctly from the displayed text).  This crazy code is all
designed
> to prevent the user from ever seeing this "phantom newline", by modifying the
> mouse messages passed to the CRichEditCtrl to appear to be clipped to only the
> visible text.  As you can tell by the comments in some of the calling code,
the
> amount of hacky craziness required to implement this is rather large...
> 
> Getting this function wrong will result in effects like occasionally seeing
the
> right edge of the selection flicker due to the "phantom newline" being
selected
> (or not).
> 

Oh thats ugly :x Thanks for the explanation!


> I agree; at the very least we'd need to take a careful look at the existing
> code.  I'm sort of frustrated that someone just used this class without paying
> attention to its single-linedness :(

Yea, I was surprised too cause nothing works for multi-line :)


> If you can note any other problems you see (such as the selection issue) on
the
> bug, perhaps we can morph it to "add TextArea or else refactor TextField to
make
> it work for multiline cases".
> 
> Of course, if this is the only case in the product we use this (and I think it
> is), perhaps we can design a different solution that's less work than doing a
> proper multiline edit.  And certainly getting it wrong is much lower impact
than
> other, more frequently-encountered bugs...

Peter, the whole TextField is messed up for multi-line.  Many issues were seen,
for 
instance:
1) Text Selection doesn't work
2) Can't select text upwards
3) Selecting text downwards = funky
4) There is no mouse middle scroll to scroll text if control.
5) There is no scroll pane if we have too much text in that given area!
6) Cannot properly remove selection once selected

So I don't know, this wont work for multi-selected textfields. If you can take a
look
at it and tell me what you think. So you think Google Chrome will never use
another
Multi-line TextField ? I think it would be better to make a TextArea since in
the future
we might need it, and it would be better to separate two different controls for
readability,
unless making it all in one class is cool too. Like TextField in C#, it can be
multi and 
single. 

It all depends on you :) You got the powa! We should do the morph (refactor or
TextArea) you 
decide. Currently, many bugs, should have another design to incorporate
multi-select.

[Peter Kasting, 2008-12-01 05:53:29]

On 2008/12/01 05:20:04, Mohamed Mansour wrote:
> 1) Text Selection doesn't work
> 2) Can't select text upwards
> 3) Selecting text downwards = funky
> 4) There is no mouse middle scroll to scroll text if control.
> 5) There is no scroll pane if we have too much text in that given area!
> 6) Cannot properly remove selection once selected

Nasty.  I pasted that into the bug and morphed it.  Let's go ahead and close
this particular review.  I don't know who wrote the bug reporter UI, perhaps I
can find some people to advise on where to go from here.

[Mohamed Mansour (USE mhm), 2008-12-01 05:58:55]

Yea okay :( Let me know what is needed, and I could attempt fixing it with the
new design