Enter enough text into a single line textbox to cause it to scroll, hit the end key, the textbox isn't scrolled enough to show the cursor. Regression range indicates the day bug 526394 landed.

This testcase shows the bug. We should be able to scroll the black div into view, but we can't. The basic issue is that here the logical horizontal scroll range is from 0 to 0.6px; the scrollframe code requires the horizontal scroll position to be an integer, so it can only be 0. We should probably snap the scroll range bounds to nearest integers before clamping.

Snapping the scroll range bounds to nearest integer pixels fixes the caret issue, but it can introduce new issues as shown in the latest testcase. We can end up making the scroll range too large, so that there's a visible gap between the trailing edge of the scrolled content and the border of the scrollframe. This is a bit of a quandry. I'll need to think about it some more.

I can't think of a way to solve this without just letting scroll positions take fractional values. Maybe we should just do that. The reason we wanted to snap scroll positions to device pixels is to ensure that any scroll operation scrolls by a multiple of device pixels, which is required for blitting to be correct. But we could say that scroll positions have to be either a multiple of device pixels or on the edge of the scroll range, and when the scroll position changes by an amount that's not a multiple of device pixels, we repaint everything instead of blitting. In practice that would mean when the user scrolls down to the end of a window, we repaint the entire window when they reach the end. Or we could not try so hard ... in order of decreasing ambition: 1) do what I said in the last paragraph, so it's always possible to scroll to a fractional offset to see content at the edge 2) as 1), but don't do it for the viewport of the toplevel content window 3) as 1), but only do it for scrolling in the horizontal direction 4) as 1), but only do it for scrolling in the horizontal direction and not in the viewport of the toplevel content window 5) just hack caret so that we always position the caret in from the edge of the scrollframe so it's visible no matter what David, any thoughts?

One way of thinking about the problem here is that drawing generally snaps to device pixel boundaries in the output device, but when snapping the scroll position we can't really do that, because the scroll position is relative to the scrollframe.

I'll take a look.

This patch implements option 5 in comment 4. I will file another bug to fix the underlying bug.

That is now bug 626541.

+ // Make sure that the caret falls on a device pixel boundary so that + // it doesn't fall outside of the view. + aRect->x = NS_ROUND_BORDER_TO_PIXELS(aRect->x, caretMetrics.mAppPerDev); I think we want to round down here.

(In reply to comment #9) > + // Make sure that the caret falls on a device pixel boundary so that > + // it doesn't fall outside of the view. > + aRect->x = NS_ROUND_BORDER_TO_PIXELS(aRect->x, caretMetrics.mAppPerDev); > > I think we want to round down here. Oh, that's right. Do we have an existing macro which I can use, or should I add my own?

Write your own I guess

I think % will go wrong when x is negative. Use floor() instead.

The test went orange on Windows 7 <http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1295466863.1295470671.32539.gz>, so I backed it out: http://hg.mozilla.org/mozilla-central/rev/cb7ba24ca60d I'm building locally to determine what's going wrong.

This is why the test fails. We end up with aRect->x equal to 24069 and overflow equal to 60, which means that aRect->x ends up being 24009, and then 24000, after being rounded. Now, apparently that's not enough, as the caret ends up too close to the edge again. Changing the rounding code to aRect->x = (floor(double(aRect->x) / caretMetrics.mAppPerDev) - 1) * caretMetrics.mAppPerDev; Makes the caret show up correctly. Now, I'm thinking, if aRect->x is in the first half of a mAppPerDev-sized interval, should I consider moving it down by one pixel? Or something like that?

(In reply to comment #17) > This is why the test fails. We end up with aRect->x equal to 24069 and > overflow equal to 60, which means that aRect->x ends up being 24009, and then > 24000, after being rounded. > > Now, apparently that's not enough, as the caret ends up too close to the edge > again. Changing the rounding code to Presumably aRect->XMost() is 24129 and the edge of the frame is at 24069. Hmm. Come to think of it, I don't know why the old code doesn't work. The caret should be one device pixel wide, so after placing its XMost() at the right edge of the scroll area (in appunits), both caret edges and the scroll area edge should snap by the same amount in the same direction, so the caret should be visible.

(In reply to comment #18) > (In reply to comment #17) > > This is why the test fails. We end up with aRect->x equal to 24069 and > > overflow equal to 60, which means that aRect->x ends up being 24009, and then > > 24000, after being rounded. > > > > Now, apparently that's not enough, as the caret ends up too close to the edge > > again. Changing the rounding code to > > Presumably aRect->XMost() is 24129 and the edge of the frame is at 24069. Yes. > Hmm. Come to think of it, I don't know why the old code doesn't work. The caret > should be one device pixel wide, so after placing its XMost() at the right edge > of the scroll area (in appunits), both caret edges and the scroll area edge > should snap by the same amount in the same direction, so the caret should be > visible. How can I check the result of snapping for each of them?

they're snapped via gfxContext::Rectangle. You'll have to find the invocations that draw the scrollport border, and the invocation that draws the caret.

Here is a summary of how the caret is not drawn. The x position is 24000, and the nsDisplayCaret's mToReferenceFrame.x is -15360, which puts the relative x coordinate at 8640. The visible region is {x: 660, y: 6180, width: 7980, height: 960}, which means that the caret rectangle is outside of the visible region, and is therefore clipped and not drawn. So, hunting down the gfxContext::Rectangle invocation for painting the caret is pointless, as it's done from the FillRect call inside nsCaret::PaintCaret, which doesn't get called in this case at all. I'm not sure how to proceed from here...

(In reply to comment #21) > Here is a summary of how the caret is not drawn. The x position is 24000, and > the nsDisplayCaret's mToReferenceFrame.x is -15360, which puts the relative x > coordinate at 8640. The visible region is {x: 660, y: 6180, width: 7980, > height: 960}, which means that the caret rectangle is outside of the visible > region, and is therefore clipped and not drawn. You mean the display list visible region passed into nsDisplayItem::ComputeVisiblity?

(In reply to comment #22) > (In reply to comment #21) > > Here is a summary of how the caret is not drawn. The x position is 24000, and > > the nsDisplayCaret's mToReferenceFrame.x is -15360, which puts the relative x > > coordinate at 8640. The visible region is {x: 660, y: 6180, width: 7980, > > height: 960}, which means that the caret rectangle is outside of the visible > > region, and is therefore clipped and not drawn. > > You mean the display list visible region passed into > nsDisplayItem::ComputeVisiblity? Yes.

OK that just means the last call to DrawCaret didn't put the caret in the right position. Back to comment #17, in GetGeometryForFrame, what are the values going the calculation of 'overflow'? Seems to me from comment #23, scrolled->GetVisualOverflowRectRelativeToSelf().width should be 7980.

(In reply to comment #24) > OK that just means the last call to DrawCaret didn't put the caret in the right > position. > > Back to comment #17, in GetGeometryForFrame, what are the values going the > calculation of 'overflow'? Seems to me from comment #23, > scrolled->GetVisualOverflowRectRelativeToSelf().width should be 7980. No, caretInScroll.XMost() is 24189, and scrolled->GetVisualOverflowRectRelativeToSelf().width is 24129.

roc: do you have any suggestions on how I should proceed here? At the lack of a better option, I think I should go with option 1 in comment 4, although that would be a rather big project this late in the cycle :/

I think I need to debug this to understand what's going on.

(In reply to comment #3) > Snapping the scroll range bounds to nearest integer pixels fixes the caret > issue, but it can introduce new issues as shown in the latest testcase. We can > end up making the scroll range too large, so that there's a visible gap between > the trailing edge of the scrolled content and the border of the scrollframe. > > This is a bit of a quandry. I'll need to think about it some more. Perhaps this would work if we also ensured that scroll positions themselves were snapped to device pixels. In other words, if a scroll frame starts at 100.3px and ends at 199.7px, we'd have a scrollable width of 100px (as we do now) and we'd make the initial scroll position be -0.3px instead of 0 (which your patch did not do, from the sound of it). That said, I'm having trouble thinking of anything I'd want to do here that would be worth the risk at this point in the cycle.

(In reply to comment #28) > Perhaps this would work if we also ensured that scroll positions themselves > were snapped to device pixels. In other words, if a scroll frame starts at > 100.3px and ends at 199.7px, we'd have a scrollable width of 100px (as we do > now) er, as we did *with your patch* (it's 99px now) > and we'd make the initial scroll position be -0.3px instead of 0 (which > your patch did not do, from the sound of it).

** PRODUCT DRIVERS PLEASE NOTE ** This bug is one of 7 automatically changed from blocking2.0:final+ to blocking2.0:.x during the endgame of Firefox 4 for the following reasons: - it was marked as a soft blocking issue without a requirement for beta coverage

The work in bug 681192, if completed, should fix this. In that bug we allow subpixel scroll offsets (and change the scrolling code so that whenever we scroll, we try to pick a destination offset that's an integer number of layer pixels offset from the current scroll position, but sometimes we'll fail).

It seems to be working nowadays. Can someone confirm this?

Yes, it was fixed by bug 681192.