299748 - On listbox, Sometimes, We cannot scroll the page to bottom by mouse wheel

Status: RESOLVED FIXED

(Core :: XUL, defect, P2)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

If a listbox is scrolled to most bottom, we can scroll the page to bottom.
But sometimes, it cannot scroll the page.
Because following check is wrong. It is not checking the rounding by pixel.

http://lxr.mozilla.org/seamonkey/source/content/events/src/nsEventStateManager.cpp#1724

1736         passToParent = (aScrollHorizontal ?
1737                         (xPos + portRect.width >= scrolledSize.width) :
1738                         (yPos + portRect.height >= scrolledSize.height));

On Win2k, In https://bugzilla.mozilla.org/query.cgi,
I can reproduce this problem on the "Product", "Target" and "where one or more
of the following changed".

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv1.0

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv1.0

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv1.0

Comment 4

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

I think it would be clearer for CanScroll to take parameters PRBool aHorizontal,
PRBool aForward. Can you rework the patch to do that?

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv2.0

Comment 6

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

Comment on attachment 188425 [details] [diff] [review]
Patch rv2.0

+  nsIDeviceContext  *dev;
+  float	     t2p;
+  float	     p2t;
+
+  mViewManager->GetDeviceContext(dev);
+  t2p = dev->AppUnitsToDevUnits(); 
+  p2t = dev->DevUnitsToAppUnits();
+
+  NS_RELEASE(dev);

Looks good, but make this an nsCOMPtr<nsIDeviceContext>, use getter_Addrefs and
you don't need the NS_RELEASE.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv2.1

The risk is low.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

checked-in.

-> FIXED

Comment 9

[Mats Palmgren (inactive)]

Do we need to bump the IID for the nsIScrollableView interface?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Yes, we do.  Reopening until that happens...  That should probably block 1.8b3.

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Changing the IID

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 188689 [details] [diff] [review]
Changing the IID

Boris:

Could you review it?

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 188689 [details] [diff] [review]
Changing the IID

The risk is NONE.
This is changing only IID of nsIScrollableView.
This is final blocker bug of 1.8b3.

Comment 14

[Chris Beard]

let's get this landed asap for 1.8b3

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

checken-in. thanks.