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.

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.

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

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

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 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?)

> 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 ?

(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.

(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.

(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.

(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 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.

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

> 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.

(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.

(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.

> 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 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 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 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 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 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 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 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 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.

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

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

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.