111034 - XUL needs clientwidth/height or scrollheight/width properties

Status: RESOLVED FIXED

(Core :: XUL, enhancement)

Description

[rvj]

I understand via the DOM newgroup that  the current viewport properties have 
been written for html only.

I would argue that there is just as much need (if not more) to make this 
functionality available for XUL applications because document browsing is most 
likely to be controlled via the user agent (rather than the document)

As a minimum could there be support for the following?

window.frames[xulwindowname].document.scrollHeight
window.frames[xulwindowname].document.scrollWidth

Comment 1

[Boris Zbarsky [:bzbarsky]]

xul

Comment 2

[rvj]

where xulwindowname just means any valid  reference to a xul 
iframe/browser/editor element

Comment 3

[Fabian Guisset]

CC'ing jst.
1) Even HTML documents don't have scroll* properties available, only HTML
elements and windows.
2) window.frames[xulwindowname].scrollX and window.frames[xulwindowname].scrollY
probably do what you want.

Is it scroll* properties for XUL *elements* (like box, I assume?) you are
requesting?

Comment 4

[rvj]

I guess I am beginning to get a bit punch drunk with sytax. 

What ideally I want  to do is compare the window.innerWidth(innerHeight) with 
the window.scrollWidth(scrollHeight) and perhaps other properties to determine 
if a particular scrollbar is  displayed.

Since  window.scrollWidth is not a supported property  then I presume I need to 
fetch the scrollwidth(or clientWidth) of the document within it.?

Im not sure if this approach will work  but surely it ought to be possible to 
could return the document's width (not including  scrollbars)

PS

1. Im not sure what window.frames[xulwindowname].scrollX and window.frames
[xulwindowname].scrollY are supposed to do. Can you explain? Im aware of the 
now depreciated scroll method but that has been replaced by ScrollBy.


2. MORE importantly Im trying to find out the height of document for a given 
window. Now with htm documents this is simple.  e.g. window.frames
[x].document.height.

But with xml this is inappropriate - I believe you have to use computed style 
to get the current document height based on the root node.


Now what I had hoped is that a single method/property could be used to return 
the current document height irrespective of file type. Is there such a property 
or could scrollheight be used to claculate the correct depth


 

Comment 5

[rvj]

you can mark this bug invalid if you like 

Ive finally found a truely awful work around for detecting scrollbar 
visibility !!!

Comment 6

[Mike Potter]

If you've got a way to scroll XUL elements, can you please post your solution
somewhere? 

Comment 7

[Alex Vincent [:WeirdAl]]

This bug, as filed, probably is invalid.

I'm wondering, though... can there be some standard way to expose a box's 
scrollbars to the DOM, in all incarnations?  If there is a bug already filed 
for this, I'm not sure where.

Comment 8

[Neil Deakin]

Attachment: support offset/client/scroll properties in xul

This patch just moves the offsetXXX/clientXXX/scrollXXX properties into GenericElement. The offset/scroll/clientWidth/clientHeight properties are useful, but I can't figure out what clientLeft/clientTop are supposed to be values of.

Most of this patch is just moving code and the addition of two testcases. The only other code change of note is in GetOffsetRect so that popup frames are treated as offset parents.

Comment 9

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

This makes these properties apply to SVG elements. Do you know what they're going to return for SVG elements? Does it make sense?

Comment 10

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

Comment on attachment 266392 [details] [diff] [review]
support offset/client/scroll properties in xul

clearing request until question is addressed

Comment 11

[Neil Deakin]

Roc, there's a spec for this at http://dev.w3.org/csswg/cssom-view/ which indicates that these properties are html only using an HTMLElementView interface. Maybe I should put these properties in a new interface and just have XUL elements implement this interface as well.

Comment 12

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

I don't think we really want offset* for anything but HTML. That stuff is evil and must be stamped out.

Seems to me client* and scroll* are reasonably well defined and should apply to all elements. I'll mention that to Anne and see if we can get the spec updated.

Comment 13

[Neil Deakin]

Well, I mainly want to be able to remove the need for box objects. getBoundingClientRect has the problem that it returns floats which means you have to round them in many cases, and doesn't provide a means of retrieving the width/height.

So replacing elem.boxObject.width with much longer code doesn't seem logical.

Note that elem.clientWidth returns the padding width of a element (inside the scroll bars) making it not a suitable replacement.

Comment 14

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

> which means you have to round them in many cases

In what cases?

> So replacing elem.boxObject.width with much longer code doesn't seem logical.

Then we should add height and width properties to ClientRect.

Comment 15

[Neil Deakin]

Attachment: updated patch and includes tests for clientXXX, offsetXXX and scrollXXX properties

Comment 16

[Myk Melez [:myk] [@mykmelez]]

Is there any reason the second patch hasn't had review requested on it yet?

Comment 17

[Neil Deakin]

Comment on attachment 319807 [details] [diff] [review]
updated patch and includes tests for clientXXX, offsetXXX and scrollXXX properties

Yes, this can be reviewed.

Comment 18

[Neil Deakin]

The patch also adds the width and height properties to ClientRect which are currently in the spec at http://dev.w3.org/csswg/cssom-view/

Comment 19

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

The spec isn't clear bout what scroll* and client* should return for SVG elements, but I think they don't "have an associated CSS layout box" so they should all return zero. I think currently we return nonsense. Otherwise the patch is great, so just fix that and we're done.

Comment 20

[Neil Deakin]

Attachment: set clientXXX and scrollXXX properties to 0 for svg elements, also clean up unused code in the test

Comment 21

[Neil Deakin]

Attachment: changes

OK, I needed to make some changes because it didn't seem to work on Windows anymore, but did a while ago.

Comment 22

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

+static nsIFrame*
+GetStyledFrameFor(nsGenericElement* aElement)
+{
+  nsIFrame *frame = aElement->GetPrimaryFrame(Flush_Layout);
+
+  return (frame && frame->GetType() == nsGkAtoms::tableOuterFrame) ?
+    frame->GetFirstChild(nsnull) : frame;
+}

Can you share this with the equivalent code in nsComputedDOMStyle::GetPropertyCSSValue and possibly elsewhere? At least the part other than the Flush should be easy to share via an nsLayoutUtils method. You can share with nsGenericHTMLElement by making it a method on nsGenericElement.

+  aRect.x = aRect.y = 0;
+  aRect.Empty();

I'd just write aRect = nsRect();

Comment 23

[Neil Deakin]

(In reply to comment #22)
> +static nsIFrame*
> +GetStyledFrameFor(nsGenericElement* aElement)
> +{
> +  nsIFrame *frame = aElement->GetPrimaryFrame(Flush_Layout);
> +
> +  return (frame && frame->GetType() == nsGkAtoms::tableOuterFrame) ?
> +    frame->GetFirstChild(nsnull) : frame;
> +}
> 
> Can you share this with the equivalent code in
> nsComputedDOMStyle::GetPropertyCSSValue and possibly elsewhere?

I'm not clear on what should be shared here. GetStyledFrameFor? Just the last line? I haven't changed the code, just moved it.

I will however share GetStyledFrameFor between nsGenericElement and nsGenericHTMLElement

Comment 24

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

the return expression could be shared more widely, but it's OK to just share between nsGenericElement and nsGenericHTMLElement so we're not adding more duplication.

Comment 25

[Neil Deakin]

Attachment: fix issues

Comment 26

[(not currently active) Ted Mielczarek]

I backed this out because it broke the Microformat unit tests:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1218813548.1218817694.2804.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1218814005.1218818328.4596.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1218814948.1218817939.3435.gz

Comment 27

[Neil Deakin]

The microformat code is trying to do:

var box = microformatNodes[i].getBoundingClientRect();
box.width = box.right - box.left;
box.height = box.bottom - box.top;

which fails now because width and height are already defined.

Is some extra magic needed to support this, or should Microformat.js just be changed?

Comment 28

[Neil Deakin]

Attachment: This would be the microformat fix

We can make this change anyway to fix the test.

Comment 29

[Mike Kaply [:mkaply]]

Comment on attachment 333977 [details] [diff] [review]
This would be the microformat fix

changing the test is fine. I only did that as a convenience so I would have width/height for both the getBoundingClientRect case and the getBoxObjectFor case.

Comment 30

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

Should we make width and height replaceable properties?

Comment 31

[Neil Deakin]

Checked this in

Comment 32

[Eric Shepherd [:sheppy]]

Docs have been updated:

http://developer.mozilla.org/En/DOM/Element.getClientRects
http://developer.mozilla.org/En/DOM/Element.getBoundingClientRect

Comment 33

[Dão Gottwald [:dao]]

(client|scroll)(Height|Width) should probably also show up here: http://developer.mozilla.org/En/XUL/Property