265940 - Textfield doesn't scroll horizontally to left after backspace or left arrow

Status: VERIFIED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Martijn Wargers (dead)]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8a5) Gecko/20041024 Firefox/0.9.1+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8a5) Gecko/20041024 Firefox/0.9.1+

Take for example the location bar. 
Type a lot of text, so much that it doesn't fit anymore in the location bar.
Then press Backspace until the beginning
Result -> the caret becomes invisible, because for some reason there isn't
scrolled back to the beginning.

This is a regression, it doesn't happen in:
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8a5) Gecko/20041018
Firefox/0.9.1+
But it does happen in:
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8a5) Gecko/20041019
Firefox/0.9.1+

I think the patch for bug 262578 is causing this.
With an old debug build (which didn't have the bug), I've applied the patch and
afterwards it did have the bug.

Reproducible: Always
Steps to Reproduce:
1.
2.
3.

Comment 1

[Mark Banner (:standard8)]

I can confirm the same happens on Linux build id:

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8a5) Gecko/20041025

I noticed it when I was playing around with the search dialog in mailnews.

Comment 2

[Aaron Leventhal]

It's worse than the caret disappearing -- you can't see the start of the text
either.
Resummarizing to reflect this.

Comment 3

[Aaron Leventhal]

It's not as easy to reproduce with the left arrow for some reason. Backspace is
consistent.

Comment 4

[Aaron Leventhal]

Attachment: Only require visible scrollbars or an actual area to scroll to when looking for a scroll view in a particular direction

Selection looks for any scrolling view regardless of direction. This is because
any given selection command may have to scroll horizontally and/or vertically.

So, we should only require actual visible scrollbars if we're hitting a key
that confines scrolling in a specific direction.

Comment 5

[Mats Palmgren (inactive)]

Comment on attachment 163858 [details] [diff] [review]
Only require visible scrollbars or an actual area to scroll to when looking for a scroll view in a particular direction

>Index: nsCaret.cpp

This patch seems to be for some other bug - 
I haven't reviewed this change.

>Index: nsLayoutUtils.cpp
>+      // If scrolling in a specific direction, require visible scrollbars or
>+      // something to scroll to in that direction.

OK, but doesn't the first comment in the function explain everything already?

>       if (aDirection != eVertical &&
>           ss.mHorizontal != NS_STYLE_OVERFLOW_HIDDEN &&
>-          (totalWidth > visibleSize.width || margin.bottom))
>+          (aDirection == eEither || totalWidth > visibleSize.width || margin.bottom))
>         break;

The patch fixes this bug, but I don't think it matches what the comment(s)
says this function should do. In this particular case "aDirection == eEither"
is true and you are making us ignore wether or not this particular view
has something to scroll which is not correct. See upcoming testcase...

(BTW, I traced the total* and visibleSize.* values and they are equal and
that seems to be the root of the problem, the text control does not
follow the usual rules for when something can scroll.
Why is GetNearestScrollingView() trying to guess how a specific
nsIScrollableView/Frame is implemented anyway? 
Wouldn't a nsIScrollableView/Frame::WantToScroll(Direction) or something
be better?)

Comment 6

[Mats Palmgren (inactive)]

Attachment: Does not work with the first patch

Comment 7

[Mats Palmgren (inactive)]

> Wouldn't a nsIScrollableView/Frame::WantToScroll(Direction) or something
> be better?

Maybe the logic for detecting when things can scroll in
nsEventStateManager::DoScrollText() would work ?

Comment 8

[Aaron Leventhal]

(In reply to comment #6)
> Created an attachment (id=164126)
> Does not work with the first patch

This works for me. If you put the caret there and press down arrow, the page
gets scrolled, which is what we want.

Comment 9

[Aaron Leventhal]

(In reply to comment #5)
> (From update of attachment 163858 [details] [diff] [review])
> Wouldn't a nsIScrollableView/Frame::WantToScroll(Direction) or something
> be better?)

I think it's valid that GetNearestScrollableView depends on what we're scrolling
for:
* When scrolling for selection movement, all we care about is finding a
scrollable view, because we don't know whether right arrow will actually end up
scrolling down, etc. We'd don't want to scroll an inappropriate ancestor view.
* When scrolling for a command that implies limiting the scrolling to a specific
direction, we want to make sure we can actually scroll it that direction. We can
be more flexible about scrolling something appropriate.

We can make the code clearer. How about we add 2 boolean parameter to
GetNearestScrollableView() like aForSelection. When we're not scrolling for the
selection we can scroll it off screen. When we are scrolling for the selection
we must make sure it stays on screen.

Comment 10

[Aaron Leventhal]

(In reply to comment #5)
> (BTW, I traced the total* and visibleSize.* values and they are equal and
> that seems to be the root of the problem ...

I disagree that it's the root of the problem. For example, if you turn on caret
browsing and right arrow inside of an iframe, you only ever want to scroll the
iframe right or down. You never want to scroll the parent document for that
movement. However, if caret browsing is off the right arrow key can move to the
parent document if there is no sense in scrolling horizontally.

Comment 11

[Aaron Leventhal]

Attachment: Add aForSelection param to GetNearestScrollableView so it can be smart about whether scroll view is for selection or not

Comment 12

[Mats Palmgren (inactive)]

(In reply to comment #10)
> However, if caret browsing is off the right arrow key can move to the
> parent document if there is no sense in scrolling horizontally.

IMO, if an iframe (or anything really) is scrolled to its end position,
keyboard commands should _not_ try to scroll the parent [document] instead.
That should only occur for mouse-wheel scrolling.

Comment 13

[Mats Palmgren (inactive)]

Comment on attachment 164759 [details] [diff] [review]
Add aForSelection param to GetNearestScrollableView so it can be smart about whether scroll view is for selection or not

This looks fine to me.

Comment 14

[Mats Palmgren (inactive)]

(I have spawned off bug 268490 for the behaviour of the textfield when text
is deleted which is wrong, at least on Linux)

Comment 15

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

> For example, if you turn on caret browsing and right arrow inside of an iframe,
> you only ever want to scroll the iframe right or down. You never want to scroll
> the parent document for that movement.

Not true ... if the IFRAME is partially scrolled out of sight and you move the
caret into that out-of-sight area, we want to scroll the parent document to move
it into view.

Comment 16

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

(In reply to comment #9)
> I think it's valid that GetNearestScrollableView depends on what we're
> scrolling for:
> * When scrolling for selection movement, all we care about is finding a
> scrollable view, because we don't know whether right arrow will actually end
> up scrolling down, etc. We'd don't want to scroll an inappropriate ancestor
> view.
> * When scrolling for a command that implies limiting the scrolling to a
> specific direction, we want to make sure we can actually scroll it that
> direction. We can be more flexible about scrolling something appropriate.

Maybe it's because I'm exhausted, but why doesn't eEither take care of the first
case?

Maybe here we should just be rewriting ScrollPointIntoView by inlining the bits
of GetNearestScrollingView that we need? Wrapping GetNearestScrollingView's loop
up in another loop seems unnecessary and confusing.

Comment 17

[Aaron Leventhal]

(In reply to comment #15)
> Not true ... if the IFRAME is partially scrolled out of sight and you move the
> caret into that out-of-sight area, we want to scroll the parent document to move
> it into view.
Okay, but the patch takes that into account. What I was saying is that whether
selection is asking for the scrollable frame affects the algorithm.

> Maybe it's because I'm exhausted, but why doesn't eEither take care 
> of the first case?
Yes. The first patch did that. Mats and I discussed, and decided that an extra
parameter would make it more readable code.

> Maybe here we should just be rewriting ScrollPointIntoView by inlining the
> bits of GetNearestScrollingView that we need? Wrapping 
> GetNearestScrollingView's loop up in another loop seems unnecessary and 
> confusing.
That seems like a separate code cleanup bug. There are a lot of places where
nsTypedSelection uses GetNearestScrollingView, so I don't believe that a change
to ScrollPointIntoView will remove the need for this patch.

Comment 18

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

> Yes. The first patch did that. Mats and I discussed, and decided that an extra
> parameter would make it more readable code.

The redundancy confused me because I start looking for the reason it's
necessary, and there isn't one. Would you mind taking it out?

Comment 19

[Aaron Leventhal]

Comment on attachment 163858 [details] [diff] [review]
Only require visible scrollbars or an actual area to scroll to when looking for a scroll view in a particular direction

Taking out the redundancy would lead back to this version (minus the caret
stuff). Is this better?

Comment 20

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

Comment on attachment 163858 [details] [diff] [review]
Only require visible scrollbars or an actual area to scroll to when looking for a scroll view in a particular direction

yeah, I prefer this one

Comment 21

[Aaron Leventhal]

Comment on attachment 163858 [details] [diff] [review]
Only require visible scrollbars or an actual area to scroll to when looking for a scroll view in a particular direction

Mats, will you reconsider this one. Roc prefers it. I can tweak the comments to
make it clear.

Comment 22

[Aaron Leventhal]

Comment on attachment 163858 [details] [diff] [review]
Only require visible scrollbars or an actual area to scroll to when looking for a scroll view in a particular direction

Mats, will you reconsider this one. Roc prefers it. I can tweak the comments to
make it clear.

Comment 23

[Mats Palmgren (inactive)]

Comment on attachment 163858 [details] [diff] [review]
Only require visible scrollbars or an actual area to scroll to when looking for a scroll view in a particular direction

review+ for the nsLayoutUtils.cpp change.
(I must have had something bad in my tree when I tested
this the first time. My apologies for that.)

Comment 24

[Aaron Leventhal]

Comment on attachment 163858 [details] [diff] [review]
Only require visible scrollbars or an actual area to scroll to when looking for a scroll view in a particular direction

Seeking a= for non-caret changes.

Comment 25

[Aaron Leventhal]

Comment on attachment 163858 [details] [diff] [review]
Only require visible scrollbars or an actual area to scroll to when looking for a scroll view in a particular direction

Seeking a= for non-caret changes.

Comment 26

[Asa Dotzler [:asa]]

Comment on attachment 163858 [details] [diff] [review]
Only require visible scrollbars or an actual area to scroll to when looking for a scroll view in a particular direction

a=asa for 1.8a5 checkin.

Comment 27

[Aaron Leventhal]

Checking in nsLayoutUtils.cpp;
/cvsroot/mozilla/layout/base/src/nsLayoutUtils.cpp,v  <--  nsLayoutUtils.cpp
new revision: 3.24; previous revision: 3.23
done

Comment 28

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED for me on Windows XP, build 2004-11-20-05, Seamonkey trunk.

Comment 29

[Asa Dotzler [:asa]]

Comment on attachment 163858 [details] [diff] [review]
Only require visible scrollbars or an actual area to scroll to when looking for a scroll view in a particular direction

1.0 has shipped and we're looking to the trunk for future Firefox releases.