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.
over to evaughan.
Assignee: hyatt → evaughan
Target Milestone: --- → Future
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) ?
rginda, is this still something we should do?
Assignee: eric → rginda
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?
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.
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?
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
Moving non nsbeta1+ XUL 1.0 bugs to mozilla1.2
Target Milestone: Future → mozilla1.2
*** Bug 128525 has been marked as a duplicate of this bug. ***
Patch update to trunk
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?).
Same patch, but with the .idl updated to include some documentation.
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] ) > >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 ;-)
patch updated to review comments.
That doesn't address my review comment on GetPosition() (see comment 19).
Attachment #181201 - Flags: review?(bzbarsky) → review-
sorry, I missed that one. This patch does address it.
Comment on attachment 181549 [details] [diff] [review] patch v2.5 r+sr=bzbarsky
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
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.