52140 - scrollTo and getPosition methods of ScrollBoxObjects should use pixels

Status: RESOLVED FIXED

(Core :: XUL, defect, P3)

Description

[Robert Ginda]

Unless of course, there is some internal API client that expects twips.
Scripters expect to operate in pixels though.

If you can r= this patch, I'll check it in.

Comment 1

[Robert Ginda]

Attachment: sbo.diff; nsScrollBoxObject.cpp changes

Comment 2

[Robert Ginda]

over to evaughan.

Comment 3

[Peter Trudelle]

->future

Comment 4

[Robert Ginda]

oops, my initial comment was incorrect.  it should have read, "_If_ there is
some internal API client that expects twips...".  It now appears as though there
isn't.

I agree this isn't nsbeta3+, but the patch is here, and it works.  I hope this
can go in post beta3, rather than ship a broken API when we have a fix in hand.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Could someone fix this (there is a patch already) ?

Comment 6

[John Morrison]

rginda, is this still something we should do? 

Comment 7

[Robert Ginda]

Attachment: updated patch

Yeah, this should still happen, otherwise scrollTo and getScrollIndex are
useless to script.

Here is an updated patch.  Someone should write up a testcase and verify that
we don't have any call sites to update though.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Will this be fixed in 0.9.8?

Comment 9

[Robert Ginda]

Smaug, have you tested the patch?  Can you write a testcase?  Can you verify
that there are no call sites that depend on twips?

Any one of these would help this patch land.

cc'ing hyatt, who may know abou the callsites.

Comment 10

[Peter Wilson]

Is this going to happen? Soon? You now have a script client for this code who 
can not use scrollTo because I can't determine twip values. Bug #128525 may also 
be related to confusion of twips and pixels. 
I notice that Chatzilla has one <scrollbox>. This is currently the only use in 
Mozilla. 
(I am writing a folding XUL editor and need to scroll and tab about the page at 
will.) 
Question: If I use a vbox instead of a scrollbox I get scrollbars etc. but I can 
find no way to scroll from the script - is this possible?

Comment 11

[Robert Ginda]

Please test this patch.  Apply it, run the smoke tests, and report on your
findings.  If you want to see it land, you're going to have to pitch in.  I've
got quite a but on my 1.0 plate, and not much use for this in that timeframe. 
If this is important to you, please help by testing.

Comment 12

[Brendan Eich [:brendan]]

This is going in for 1.0, if I have to test it myself.  I don't think I have to,
prove me wrong you XUL fans!

/be

Comment 13

[Joe Hewitt (gone)]

Moving non nsbeta1+ XUL 1.0 bugs to mozilla1.2

Comment 14

[Bill Mason]

*** Bug 128525 has been marked as a duplicate of this bug. ***

Comment 15

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: patch v2

Patch update to trunk

Comment 16

[Michiel van Leeuwen (email: mvl+moz@)]

Forgot to mention that this patch also impements scrollBy()
I couldn't find any internal users of nsIScrollboxObject, so i assume the patch
is safe. (long popupmenus still scroll like they should)

Comment 17

[Boris Zbarsky [:bzbarsky]]

How about some documentation on the api explaining what coordinates it uses? 
Don't forget to specify what you mean by "pixels" if it's using pixels (device
pixels, or CSS pixels?).

Comment 18

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: patch v2.1

Same patch, but with the .idl updated to include some documentation.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 181070 [details] [diff] [review]
patch v2.1

>Index: layout/xul/base/public/nsIScrollBoxObject.idl
>+   * Scroll to the given coordinates, in device pixels.
>+   * (0,0) is top-left.

top-left of what?  Also specify that the coordinates increase down and to the
right?	And these should be CSS pixels.

>   void scrollTo(in long x, in long y);

What happens if negative x and y are passed in?  Can we ever have space to
scroll to that has negative coords?

>+   * Scroll the amount of device pixels to the left and down.

CSS pixels.  And it would be to the right and down, no?

What happens if the numbers are too big or too small?  Do we just ignore the
call, or scroll as far as we can and then stop?

>+  /**
>+   * Get the current scrollposition in device pixels.

CSS pixels.  And "scroll position" is two words.

>Index: layout/xul/base/src/nsScrollBoxObject.cpp

> NS_IMETHODIMP nsScrollBoxObject::ScrollTo(PRInt32 x, PRInt32 y)

>+  float pixelsToTwips = 0.0;
>+  pixelsToTwips = mPresShell->GetPresContext()->PixelsToTwips();

Why bother with assigning 0.0?	Just assign the number we want.

>+  x += dx;
>+  y += dy;
>+  return ScrollTo(x, y);

Why not just:  ScrollTo(x + dx, y + dy) ?  Looks simpler, I think...

> NS_IMETHODIMP nsScrollBoxObject::GetPosition(PRInt32 *x, PRInt32 *y)

>+  nsresult rv = scrollableView->GetScrollPosition(*x,*y);

I know this code was already there, but it's assuming that nscoord* and
PRInt32* are the same thing.  That's not really a very good assumption.  Please
pass nscoord* to GetScrollPosition (use temporaries).

>+  float twipsToPixels = 0.0;
>+  twipsToPixels = mPresShell->GetPresContext()->TwipsToPixels();

Again, no need to assign 0.0.

Comment 20

[Boris Zbarsky [:bzbarsky]]

roc or neil, do you happen to know the answers to my questions about the api? 
Specifically, how out-of-bounds values are treated and what the coord system
origin is, exactly?

Comment 21

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

(In reply to comment #19)
> (From update of attachment 181070 [details] [diff] [review] [edit])
> >Index: layout/xul/base/public/nsIScrollBoxObject.idl
> >+   * Scroll to the given coordinates, in device pixels.
> >+   * (0,0) is top-left.
> 
> top-left of what?  Also specify that the coordinates increase down and to the
> right?	And these should be CSS pixels.

(0,0) means "put the top left corner of the scrolled element's padding-box at
the top left corner of the scrollport (which is its inner-border-box)".

> >   void scrollTo(in long x, in long y);
> 
> What happens if negative x and y are passed in?  Can we ever have space to
> scroll to that has negative coords?

We can have overflow to the left of or above the top left corner of the
element's padding-box, but we currently don't support scrolling it into view.
Negative values will be clamped to (0,0). This restriction could be lifted in
the future.

> >+   * Scroll the amount of device pixels to the left and down.
> 
> CSS pixels.  And it would be to the right and down, no?

Yeah

> What happens if the numbers are too big or too small?  Do we just ignore the
> call, or scroll as far as we can and then stop?

I believe we will clamp. nsIScrollableView clamps.

Comment 22

[[e-mail]]

So, basically, it should work the same way frames scroll, whatever that is ;-)

Comment 23

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: patch v2.5

patch updated to review comments.

Comment 24

[Boris Zbarsky [:bzbarsky]]

That doesn't address my review comment on GetPosition() (see comment 19).

Comment 25

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: patch v2.5

sorry, I missed that one. This patch does address it.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 181549 [details] [diff] [review]
patch v2.5

r+sr=bzbarsky

Comment 27

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 181549 [details] [diff] [review]
patch v2.5

this patch makes scrolling scrollboxes from javascript	possible in some useful
way. Would be nice to have in the next release.

Comment 28

[Asa Dotzler [:asa]]

Comment on attachment 181549 [details] [diff] [review]
patch v2.5

a=asa

Comment 29

[Michiel van Leeuwen (email: mvl+moz@)]

patch checked in