scrollTo and getPosition methods of ScrollBoxObjects should use pixels

RESOLVED FIXED in mozilla1.2alpha

Status

()

defect
P3
normal
RESOLVED FIXED
19 years ago
11 years ago

People

(Reporter: rginda, Assigned: mvl)

Tracking

Trunk
mozilla1.2alpha
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

Reporter

Description

19 years ago
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.
Reporter

Comment 1

19 years ago
Reporter

Comment 2

19 years ago
over to evaughan.
Assignee: hyatt → evaughan

Comment 3

19 years ago
->future
Target Milestone: --- → Future
Reporter

Comment 4

19 years ago
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.
Could someone fix this (there is a patch already) ?

Comment 6

18 years ago
rginda, is this still something we should do? 
Assignee: eric → rginda
Reporter

Comment 7

18 years ago
Posted patch updated patch (obsolete) — Splinter Review
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.
Attachment #14406 - Attachment is obsolete: true
Will this be fixed in 0.9.8?
Reporter

Comment 9

18 years ago
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

18 years ago
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?
Reporter

Comment 11

18 years ago
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.
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
Blocks: 70753
Keywords: mozilla1.0

Comment 13

18 years ago
Moving non nsbeta1+ XUL 1.0 bugs to mozilla1.2
Target Milestone: Future → mozilla1.2

Updated

18 years ago
No longer blocks: 70753

Comment 14

17 years ago
*** Bug 128525 has been marked as a duplicate of this bug. ***
Posted patch patch v2 (obsolete) — Splinter Review
Patch update to trunk
Assignee: rginda → mvl
Attachment #62974 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #180823 - Flags: review?(bzbarsky)
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)
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?).
Posted patch patch v2.1 (obsolete) — Splinter Review
Same patch, but with the .idl updated to include some documentation.
Attachment #180823 - Attachment is obsolete: true
Attachment #181070 - Flags: review?(bzbarsky)
Attachment #180823 - Flags: review?(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.
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?
(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.
So, basically, it should work the same way frames scroll, whatever that is ;-)
Posted patch patch v2.5 (obsolete) — Splinter Review
patch updated to review comments.
Attachment #181070 - Attachment is obsolete: true
Attachment #181201 - Flags: review?(bzbarsky)
Attachment #181070 - Flags: review?(bzbarsky)
That doesn't address my review comment on GetPosition() (see comment 19).
Attachment #181201 - Flags: review?(bzbarsky) → review-
Posted patch patch v2.5Splinter Review
sorry, I missed that one. This patch does address it.
Attachment #181201 - Attachment is obsolete: true
Attachment #181549 - Flags: review?(bzbarsky)
Comment on attachment 181549 [details] [diff] [review]
patch v2.5

r+sr=bzbarsky
Attachment #181549 - Flags: superreview+
Attachment #181549 - Flags: review?(bzbarsky)
Attachment #181549 - Flags: review+
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.
Attachment #181549 - Flags: approval1.8b2?
Comment on attachment 181549 [details] [diff] [review]
patch v2.5

a=asa
Attachment #181549 - Flags: approval1.8b2? → approval1.8b2+
patch checked in
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED

Updated

11 years ago
Component: XP Toolkit/Widgets: XUL → XUL
QA Contact: jrgmorrison → xptoolkit.widgets
You need to log in before you can comment on or make changes to this bug.