Closed Bug 542677 Opened 10 years ago Closed 6 years ago

cursor not shown in textbox with enough text to cause scroll

Categories

(Core :: Web Painting, defect, P2)

x86
Windows 2000
defect

Tracking

()

RESOLVED FIXED
mozilla2.0b10
Tracking Status
blocking2.0 --- .x+

People

(Reporter: tnikkel, Assigned: dbaron)

References

(Depends on 1 open bug, )

Details

(Keywords: regression, testcase, Whiteboard: [softblocker][needs help])

Attachments

(3 files, 2 obsolete files)

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.
Assignee: nobody → roc
blocking2.0: --- → ?
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.
blocking2.0: ? → final+
Priority: -- → P2
I'll take a look.
Assignee: roc → ehsan
Whiteboard: [softblocker]
Attached patch Patch (v1) (obsolete) — Splinter Review
This patch implements option 5 in comment 4.  I will file another bug to fix the underlying bug.
Attachment #504586 - Flags: review?(roc)
Depends on: 626541
That is now bug 626541.
Whiteboard: [softblocker] → [softblocker][has patch][needs review roc]
+      // 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?
Attached patch Patch (v2) (obsolete) — Splinter Review
Attachment #504586 - Attachment is obsolete: true
Attachment #504608 - Flags: review?(roc)
Attachment #504586 - Flags: review?(roc)
I think % will go wrong when x is negative. Use floor() instead.
Whiteboard: [softblocker][has patch][needs review roc] → [softblocker][has patch]
Attached patch Patch (v3)Splinter Review
Attachment #504608 - Attachment is obsolete: true
Attachment #504838 - Flags: review?(roc)
Attachment #504608 - Flags: review?(roc)
Whiteboard: [softblocker][has patch] → [softblocker][needs landing]
http://hg.mozilla.org/mozilla-central/rev/3fc6728e81a8
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [softblocker][needs landing] → [softblocker]
Target Milestone: --- → mozilla2.0b10
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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...
Whiteboard: [softblocker] → [softblocker][needs help]
(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.
Assignee: ehsan → roc
(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
blocking2.0: final+ → .x+
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).
Depends on: 681192
It seems to be working nowadays. Can someone confirm this?
Flags: needinfo?(roc)
Yes, it was fixed by bug 681192.
Status: REOPENED → RESOLVED
Closed: 9 years ago6 years ago
Flags: needinfo?(roc)
Resolution: --- → FIXED
Component: Layout: View Rendering → Layout: Web Painting
You need to log in before you can comment on or make changes to this bug.