791616 - 1 pixel dancing by caret moving in certain element

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Alice0775 White]

Attachment: 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

Comment 1

[bhavana bajaj [:bajaj]]

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.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This is similar to bug 784410 but involves ScrollToShowRect/ScrollContentIntoView.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 1: Add nsIScrollableFrame::GetPerceivedScrollingDirections to consolidate logic for whether UI actions should be allowed to scroll in a given direction

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 2: Add API to ScrollIntoView methods to control whether to scroll in a direction when that direction is not a perceived scrollable direction

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 3: Don't scroll vertically to get the caret into view if that's not a perceived scrollable direction

Comment 7

[Mats Palmgren (inactive)]

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

Comment 8

[Mats Palmgren (inactive)]

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

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Sounds good, I'll do that.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/50d89c05af5b
https://hg.mozilla.org/integration/mozilla-inbound/rev/6211dca38071
https://hg.mozilla.org/integration/mozilla-inbound/rev/a04c322290d7

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Backed out due to the new test failing.
https://hg.mozilla.org/integration/mozilla-inbound/rev/d678fc7cfbef

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I got a couple of the changes for comment #8 wrong. Relanded with those fixed.

https://hg.mozilla.org/integration/mozilla-inbound/rev/46fce5a9890f
https://hg.mozilla.org/integration/mozilla-inbound/rev/de04a102395f
https://hg.mozilla.org/integration/mozilla-inbound/rev/83737cad6889

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/46fce5a9890f
https://hg.mozilla.org/mozilla-central/rev/de04a102395f
https://hg.mozilla.org/mozilla-central/rev/83737cad6889

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 18

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/97953e11eb50
https://hg.mozilla.org/releases/mozilla-aurora/rev/41f730abab15
https://hg.mozilla.org/releases/mozilla-aurora/rev/e183e7a0c34a

Comment 20

[Simona Badau, Desktop QA]

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.