Closed Bug 364612 Opened 13 years ago Closed Last year

Replace nsIScrollBoxObject API with methods on elements

Categories

(Core :: XUL, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED DUPLICATE of bug 1454358

People

(Reporter: enndeakin, Unassigned)

References

Details

Attachments

(1 file, 1 obsolete file)

 
Attached patch Patch (obsolete) — Splinter Review
This is one in a series of patches to move the box object APIs into content node APIs. In this bug, the functions of nsIScrollBoxObject.

- move scrollLeft/Top/etc into generic element so they can be shared between all element types
- move the scroll box object functions there too using similar method names as the window scrolling functions
- this patch obsoletes nsIScrollBoxObject. A follow up bug will remove the calls to it in toolkit.
 
While this patch looks fairly big, it is mostly moving code from nsGenericHTMLElement to nsGenericElement, and deleting most of the nsIScrollBoxObject code.

HTML test case: http://www.xulplanet.com/ndeakin/tests/xts/scripts/scroll-api.html
XUL test case:
http://www.xulplanet.com/ndeakin/tests/xts/scripts/scroll-api.xul

Documentation of APIs: http://wiki.mozilla.org/XUL:Specs:Scrolling

Let me know if the API looks suitable.

Also, I chose two different reviewers due to the code touching two modules.
Attachment #249362 - Flags: superreview?(roc)
Attachment #249362 - Flags: review?(jst)
This is going to break all the XUL apps out there that use nsIBoxObject, right? It appears you are not doing this (from the wiki page):
> The simplest way to make any element have a scrollable
> API is to just make nsBoxObject implement
> nsIScrollBoxObject, and put in the appropriate classinfo
> changes.
I agree that deprecating boxobject APIs in favour of direct content element APIs is the right way to go. I just wonder whether we should provide backward compatibility, and if not, if this is the right time to make such a change, and if so, if we have some way of coordinating with the XUL community on this.

Also, I think nsIDOMNSElement.idl is missing.

This is going to add scrollTo*/scrollBy* APIs to *all* elements including HTML, right? That's a pretty big Web-affecting API extension. Do we know the situation w.r.t. other browsers on these APIs? Have they implemented any of these APIs (with these names, or other names)? What do they think of these extensions? Are we going to push them through WHAT-WG or some other forum?
> The simplest way to make any element have a scrollable
> API is to just make nsBoxObject implement
> nsIScrollBoxObject, and put in the appropriate classinfo
> changes.

No, I'm not planning on doing that. I'll update the wiki. I'm not changing or removing any box object APIs for 1.9.

(In reply to comment #3)
> I agree that deprecating boxobject APIs in favour of direct content element
> APIs is the right way to go. I just wonder whether we should provide backward
> compatibility, and if not, if this is the right time to make such a change, and
> if so, if we have some way of coordinating with the XUL community on this.
> 
> Also, I think nsIDOMNSElement.idl is missing.
> 

OK, I must have missed it. It looks similar to api on the wiki page.
 
> This is going to add scrollTo*/scrollBy* APIs to *all* elements including HTML,
> right? That's a pretty big Web-affecting API extension. Do we know the
> situation w.r.t. other browsers on these APIs? Have they implemented any of
> these APIs (with these names, or other names)? What do they think of these
> extensions? Are we going to push them through WHAT-WG or some other forum?
> 

I could just put the methods I added into XULElement instead.
I'm not opposed to adding APIs to HTML elements. I just want to make sure that we do it "the right way".
In fact, I am very much in favour of taking features from XUL and making them work for all Web content (or just dropping them if something equivalent already exists for regular Web content).
OK, I'll write up a better more detailed spec and post it to the whatwg mailing list. The existing scroll features (scrollLeft, scrollTop, etc) are implemented in most browsers yet there is no spec for them, so it would be useful to have this written up anyway.
Here's the spec that I posted:

http://xulplanet.com/ndeakin/xul/specs/scrollspec.html

There may be limited work going on already for the scroll* properties in the css working group:
http://dev.w3.org/cvsweb/~checkout~/csswg/cssom/Overview.src.html
Why do scrollToIndex/scrollByIndex have to be defined in terms of CSS boxes? That's rather nasty.
Also, it's possible to actually calculate the vertical positions of lines of content in arbitrary HTML+CSS. I wonder if we should at least allow that behaviour in scrollToLine/scrollByLine.
Regarding scrollToIndex/scrollByIndex, if we can't come up with something reasonable for a spec, maybe we should make them XUL-only and deprecate them.
Yes, I think that scrollToIndex/scrollByIndex should be XUL only, so they should be defined on XULElement. They need to work based on the visible display and order of their contents. 

scrollByIndex is needed for the tabbrowser navigation arrows. I don't think that scrollToIndex is particularly useful though, in fact, ScrollBoxObject doesn't even implement it, so I could just remove it.

Can you explain the comment about calculating line positions? scrollByLine(1) should be the same as pressing the scrollbar down button.
Then I think you should just say that scrollByLine scrolls by the same amount as pressing the up or down arrow N times, and leave it at that.

I can imagine having a smarter algorithm for scrolling line by line, e.g., one that looks at the CSS line-height property, or looks at the actual heights of the line boxes. We don't want to prevent browsers from using such an algorithm.

If scrollByIndex is only in XUL (which sounds good to me) then perhaps we could eventually specify it in terms of XUL children and ordinals instead of the existing definition. But it doesn't matter right now.
Hmm, if we implemented a smarter algorithm for scrollByLines, maybe we wouldn't need scrollByIndex. Oh well, still doesn't matter right now.
Couldn't scrollByIndex be implemented as a method in the binding of the tabbrowser, so it can be removed totally?

"An implementation may optionally define a scrolling mechanism for other replaced elements. If so, the properties and methods below may function in a similar manner as described using the replaced element's scrollable content."
So it may become possible that a website could control the scroll position of external site, when using <iframe src="http://slashdot.org"></iframe> for example? Not sure if that's a bad thing.
scrollByIndex would need to be implemented in <scrollbox>/<arrowscrollbox>, as it is used for other things (large menus for instance) I'm not sure how feasible it would be to implement in JS.

I did think of the potential security issue with <iframe> so I didn't define it specifically. I can do some tests to see what browsers do currently.

Could we redefine scrollByIndex to just a) find the first visible child element and then b) scroll to the top of the (Nth) previous/next child element, without breaking things? That would be a much simpler spec. I believe it could be implemented in Javascript using getBoundingClientRect.
(which isn't checked in yet, but will be soon hopefully)
> scrollByLine(1) should be the same as pressing the scrollbar down button.

Actually, I don't want to define it that way, as some browsers do the smooth scrolling effect when the button is pressed. scrollToLine should just scroll one line.

> I believe it could be implemented in Javascript using getBoundingClientRect.

I suppose it could be, although that would mean that a <scrollbox> would be needed just to be able to call it, rather than being available for all elements.
(In reply to comment #20)
> > scrollByLine(1) should be the same as pressing the scrollbar down button.
> 
> Actually, I don't want to define it that way, as some browsers do the smooth
> scrolling effect when the button is pressed. scrollToLine should just scroll
> one line.

OK, so define it to scroll "the same amount" as pressing the scrollbar down button.

> > I believe it could be implemented in Javascript using getBoundingClientRect.
> 
> I suppose it could be, although that would mean that a <scrollbox> would be
> needed just to be able to call it, rather than being available for all
> elements.

If the definition is nasty then I don't want it to be available for all elements.

By 'all elements', I meant all XUL elements.

I'm not clear how the method proposed in comment 18 differs from what I have in the scrolling spec.
It doesn't mention CSS boxes and is therefore much easier to implement.
I used 'CSS boxes' as they have a standard definition. Does 'getBoundingClientRect' have any kind of definition? I can define the scrollToIndex api to be defined in terms of it though instead:

scrollToIndex(x, edge)
- get the rectangles retrieved from calling getBoundingClientRect on all children of the element
- sort the list of rectangles in the order of left/top depending on orientation (or right/bottom for rtl)
- scroll so that the rectangle at index x in the list is along the top edge of the container
(In reply to comment #24)
> I used 'CSS boxes' as they have a standard definition.

Yeah, but "the order they appear" not so well specified. Other than that, yes, they're well specified, but what you suggest is not easy to implement (consider elements with "display:none", reordering of boxes due to XUL ordinals, elements that generate multiple in-flow boxes, etc).

> Does
> 'getBoundingClientRect' have any kind of definition?

For elements generating regular CSS boxes, it's the union of all border-boxes, relative to the top-left of the initial containing block, i.e., the top-left of the viewport, as if everything scrollable was scrolled to the default position.

> scrollToIndex(x, edge)
> - get the rectangles retrieved from calling getBoundingClientRect on all
> children of the element
> - sort the list of rectangles in the order of left/top depending on orientation
> (or right/bottom for rtl)

What's the orientation for non-XUL boxes?

> - scroll so that the rectangle at index x in the list is along the top edge of
> the container

is it really important to scroll to box #x in the sorted list? Can't we just scroll to element #x, and assume that if you're using this API, you will ensure that element order corresponds to visual order?
(In reply to comment #25)
> What's the orientation for non-XUL boxes?

vertical

> is it really important to scroll to box #x in the sorted list? Can't we just
> scroll to element #x, and assume that if you're using this API, you will ensure
> that element order corresponds to visual order?
> 

It's more important for scrollByIndex which should scroll to the next item in visible order. This would be used, for example, for the tab scroll arrows. As I said earlier, I don't think scrollToIndex is as useful, but it should scroll using the same ordering criteria.
Status: NEW → ASSIGNED
I think we should go with comment #24 with the modification that if an element has an empty getBoundingClientRect then it should be ignored.
roc, do you think I should define these apis for xul only?

Also what about the case where several elements have the same position? I think these should be skipped too as otherwise scrolling might not occur.
Yes, the scroll*Index APIs should be XUL only since I think it's premature to push them on the Web.

I guess the algorithm for scrollByIndex should be:
-- Generate the sorted list of nonempty boxes
-- Find the last box that contains the current scroll offset
-- Move forward N boxes
-- While the current box's top edge is not below the current scroll offset, advance to the next box
-- If you run out of boxes, don't scroll at all
Depends on: 174397
Attachment #249362 - Flags: superreview?(roc)
Attachment #249362 - Flags: review?(jst)
Depends on: 430445
Attached patch updated patchSplinter Review
Implements some scrolling APIs for XUL elements.
Attachment #249362 - Attachment is obsolete: true
Duplicate of this bug: 293030
Blocks: 660561
Element now contains some scrolling functions, such that much of this is no longer needed. There are a few scroll-related functions not available such as the ensureVisible type functions and scrollByLine which have no equivalent.
Assignee: enndeakin → nobody
Status: ASSIGNED → NEW
Filed bug 1454358 to see if we can delete ScrollBoxObject entirely.
Depends on: 1454358
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → DUPLICATE
Duplicate of bug: 1454358
You need to log in before you can comment on or make changes to this bug.