1 pixel dancing by caret moving in certain element

RESOLVED FIXED in Firefox 17

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: alice0775, Assigned: roc)

Tracking

({regression, testcase})

15 Branch
mozilla18
x86
Windows 7
regression, testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox16-, firefox17- verified, firefox18-)

Details

Attachments

(5 attachments)

(Reporter)

Description

6 years ago
Created attachment 661703 [details]
testcase

Test 1 pixel dancing by caret moving.

Bug 784410 does not fix this problem.

Regression window(m-i)
Good:
http://hg.mozilla.org/mozilla-central/rev/6fe7dd2f8f57
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/15.0 Firefox/15.0a1 ID:20120510021321
Bad:
http://hg.mozilla.org/mozilla-central/rev/b7b6565d12a0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/15.0 Firefox/15.0a1 ID:20120510050721
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=6fe7dd2f8f57&tochange=b7b6565d12a0


Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/b5304fd23df9
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/15.0 Firefox/15.0a1 ID:20120509222721
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/67091352b7d2
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/15.0 Firefox/15.0a1 ID:20120509223521
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=b5304fd23df9&tochange=67091352b7d2

Last good: c3d3bfb3b68d
First bad: 9d9a3edaa0b9

Triggered by
9d9a3edaa0b9	Robert O'Callahan — Bug 681192. Part 11: Don't snap scrollrange endpoints to device pixels anymore. r=matspal
Keywords: testcase
Not tracking this for now, as this is already there on 15.0 and we have not heard any user complaints yet & seems to be a very minor issue. 

Please renominate, if you strongly feel this should be tracked.
tracking-firefox16: ? → -
tracking-firefox17: ? → -
tracking-firefox18: ? → -
This is similar to bug 784410 but involves ScrollToShowRect/ScrollContentIntoView.
Created attachment 662042 [details] [diff] [review]
possible fix

This patch fixes it, but I'm not sure it's really the way to go. I'll think about it a little more.
Assignee: nobody → roc
Created attachment 662116 [details] [diff] [review]
Part 1: Add nsIScrollableFrame::GetPerceivedScrollingDirections to consolidate logic for whether UI actions should be allowed to scroll in a given direction
Attachment #662116 - Flags: review?(matspal)
Created attachment 662117 [details] [diff] [review]
Part 2: Add API to ScrollIntoView methods to control whether to scroll in a direction when that direction is not a perceived scrollable direction
Attachment #662117 - Flags: review?(matspal)
Created attachment 662118 [details] [diff] [review]
Part 3: Don't scroll vertically to get the caret into view if that's not a perceived scrollable direction
Attachment #662118 - Flags: review?(matspal)

Updated

6 years ago
Attachment #662117 - Attachment is patch: true

Updated

6 years ago
Attachment #662116 - Flags: review?(matspal) → review+
Comment on attachment 662117 [details] [diff] [review]
Part 2: Add API to ScrollIntoView methods to control whether to scroll in a direction when that direction is not a perceived scrollable direction

> +  nsRect scrollRange = aScrollFrame->GetScrollRange();

scrollRange is never used
Attachment #662117 - Flags: review?(matspal) → review+
Comment on attachment 662118 [details] [diff] [review]
Part 3: Don't scroll vertically to get the caret into view if that's not a perceived scrollable direction

+    // Don't scroll vertically to get the caret into view, if it doesn't look
+    // like that is a scrollable direction for the container (i.e. if there
+    // is no scrollbar showing, or the allowed scroll range is very small).
+    aVertical.mScrollEvenWhenNotPerceivedScrollableDirection = false;

I had to read this a few times to understand it.  There seems to be
double negation in both the comment and assignment.  Compare this
with using positive terms:

  // Scroll vertically to get the caret into view, but only if the container
  // is perceived scrollable in that direction (i.e. there is a visible
  // vertical scrollbar or the scroll range is at least one device pixel).
  aVertical.mOnlyIfPerceivedScrollableDirection = true;

Either way is fine, but I prefer the latter logic.  r=mats
Attachment #662118 - Flags: review?(matspal) → review+
Sounds good, I'll do that.
https://hg.mozilla.org/mozilla-central/rev/46fce5a9890f
https://hg.mozilla.org/mozilla-central/rev/de04a102395f
https://hg.mozilla.org/mozilla-central/rev/83737cad6889
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Comment on attachment 662116 [details] [diff] [review]
Part 1: Add nsIScrollableFrame::GetPerceivedScrollingDirections to consolidate logic for whether UI actions should be allowed to scroll in a given direction

Review of attachment 662116 [details] [diff] [review]:
-----------------------------------------------------------------

This is a somewhat annoying user-visible regression. I think we should take it on Aurora but not beta.
Attachment #662116 - Flags: approval-mozilla-aurora?
Comment on attachment 662117 [details] [diff] [review]
Part 2: Add API to ScrollIntoView methods to control whether to scroll in a direction when that direction is not a perceived scrollable direction

Review of attachment 662117 [details] [diff] [review]:
-----------------------------------------------------------------

This is a somewhat annoying user-visible regression. I think we should take it on Aurora but not beta.
Comment on attachment 662118 [details] [diff] [review]
Part 3: Don't scroll vertically to get the caret into view if that's not a perceived scrollable direction

Review of attachment 662118 [details] [diff] [review]:
-----------------------------------------------------------------

This is a somewhat annoying user-visible regression. I think we should take it on Aurora but not beta.
Attachment #662118 - Flags: approval-mozilla-aurora?
Comment on attachment 662117 [details] [diff] [review]
Part 2: Add API to ScrollIntoView methods to control whether to scroll in a direction when that direction is not a perceived scrollable direction

Review of attachment 662117 [details] [diff] [review]:
-----------------------------------------------------------------

This is a somewhat annoying user-visible regression. I think we should take it on Aurora but not beta.
Attachment #662117 - Flags: approval-mozilla-aurora?
Attachment #662116 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #662117 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 662118 [details] [diff] [review]
Part 3: Don't scroll vertically to get the caret into view if that's not a perceived scrollable direction

Approving for Aurora since we'll get some good bake time before 17 ships.
Attachment #662118 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Mozilla/5.0 (Windows NT 6.1; rv:17.0) Gecko/17.0 Firefox/17.0

Verified on Firefox 17 beta 6 that the 1 pixel dancing by caret moving is gone. Verified by using the attached test case from the Description.
status-firefox17: fixed → verified
You need to log in before you can comment on or make changes to this bug.