Closed Bug 766937 Opened 12 years ago Closed 12 years ago

Add scrollTopMax/scrollLeftMax properties to elements

Categories

(Core :: DOM: CSS Object Model, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla16

People

(Reporter: kats, Assigned: kats)

References

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 2 obsolete files)

See https://bugzilla.mozilla.org/show_bug.cgi?id=755971#c75 for context. This bug is tracking the addition of scrollMaxX and scrollMaxY properties on elements, so that content can determine how much the element is capable of scrolling. These should always return the largest values scrollLeft/scrollTop will return, and are more or less equal to element.scroll(Width|Height) - element.client(Width|Height), modulo rounding errors.
This needs to be discussed on www-style before it's landed.
Keywords: dev-doc-needed
Verified that this returns the correct value for the d1 div in the test case at https://bugzilla.mozilla.org/attachment.cgi?id=635179&action=edit
Attachment #635352 - Flags: review?(roc)
Comment on attachment 635352 [details] [diff] [review]
Add scrollMaxX and scrollMaxY to elements

Review of attachment 635352 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsGenericElement.cpp
@@ +2201,5 @@
> +nsGenericElement::GetScrollMaxX()
> +{
> +  if (IsSVG()) {
> +    return 0;
> +  }

This IsSVG check seems redundant. Presumably GetScrollFrame() will return null in that case.

@@ +2208,5 @@
> +  if (!sf) {
> +    return 0;
> +  }
> +
> +  return nsPresContext::AppUnitsToIntCSSPixels(sf->GetScrollRange().width);

Actually you want XMost(), for now.

::: dom/interfaces/core/nsIDOMElement.idl
@@ +156,5 @@
>     */
>    readonly attribute long             clientHeight;
>  
> +  /* The maximum offset that the element can be scrolled to
> +     (i.e., the element scrollWidth/height minus the element clientWidth/height) */

I would say "the value that scrollLeft/scrollTop would be clamped to if they were set to arbitrary large values".
(In reply to :Ms2ger from comment #1)
> This needs to be discussed on www-style before it's landed.

Yes, I'll do that. Although I doubt there will be useful feedback.
Good catches, thanks. Updated to address all the comments. I also have a simple test page at http://people.mozilla.org/~kgupta/bug/766937.html that tests both the LTR and RTL cases to verify the XMost() thing is working as expected.
Attachment #635352 - Attachment is obsolete: true
Attachment #635352 - Flags: review?(roc)
Attachment #635732 - Flags: review?(roc)
Comment on attachment 635732 [details] [diff] [review]
Add scrollMaxX and scrollMaxY to elements (v2)

Review of attachment 635732 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, but on reflection I think scrollLeftMax and scrollTopMax are better names.
Updated to be scrollLeftMax and scrollTopMax as requested.
Attachment #635732 - Attachment is obsolete: true
Attachment #635732 - Flags: review?(roc)
Attachment #636415 - Flags: review?(roc)
Should I land this patch, or do you want to wait longer for feedback on www-style?
You should land it. There's no feedback on www-style yet and I don't expect to get any.
https://hg.mozilla.org/mozilla-central/rev/01c8479cf333
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
Depends on: 962249
Summary: Add scrollMaxX/Y properties to elements → Add scrollTopMax/scrollLeftMax properties to elements
You need to log in before you can comment on or make changes to this bug.