Closed Bug 791616 Opened 7 years ago Closed 7 years ago

1 pixel dancing by caret moving in certain element

Categories

(Core :: Layout, defect)

15 Branch
x86
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18
Tracking Status
firefox16 - ---
firefox17 - verified
firefox18 - ---

People

(Reporter: alice0775, Assigned: roc)

References

Details

(Keywords: regression, testcase)

Attachments

(5 files)

Attached file 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
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.
This is similar to bug 784410 but involves ScrollToShowRect/ScrollContentIntoView.
Attached patch possible fixSplinter Review
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
Attachment #662117 - Attachment is patch: true
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
Closed: 7 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.
You need to log in before you can comment on or make changes to this bug.