Last Comment Bug 766937 - Add scrollTopMax/scrollLeftMax properties to elements
: Add scrollTopMax/scrollLeftMax properties to elements
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM: CSS Object Model (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla16
Assigned To: Kartikaya Gupta (email:kats@mozilla.com)
:
Mentors:
Depends on: 962249
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-21 06:43 PDT by Kartikaya Gupta (email:kats@mozilla.com)
Modified: 2014-09-16 04:05 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Add scrollMaxX and scrollMaxY to elements (3.09 KB, patch)
2012-06-21 10:08 PDT, Kartikaya Gupta (email:kats@mozilla.com)
no flags Details | Diff | Review
Add scrollMaxX and scrollMaxY to elements (v2) (3.06 KB, patch)
2012-06-22 07:14 PDT, Kartikaya Gupta (email:kats@mozilla.com)
no flags Details | Diff | Review
Add scrollLeftMax and scrollTopMax to elements (v3) (3.09 KB, patch)
2012-06-25 11:36 PDT, Kartikaya Gupta (email:kats@mozilla.com)
roc: review+
Details | Diff | Review

Description Kartikaya Gupta (email:kats@mozilla.com) 2012-06-21 06:43:11 PDT
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.
Comment 1 :Ms2ger 2012-06-21 06:45:49 PDT
This needs to be discussed on www-style before it's landed.
Comment 2 Kartikaya Gupta (email:kats@mozilla.com) 2012-06-21 10:08:39 PDT
Created attachment 635352 [details] [diff] [review]
Add scrollMaxX and scrollMaxY to elements

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
Comment 3 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-06-21 16:05:16 PDT
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".
Comment 4 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-06-21 16:06:01 PDT
(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.
Comment 5 Kartikaya Gupta (email:kats@mozilla.com) 2012-06-22 07:14:54 PDT
Created attachment 635732 [details] [diff] [review]
Add scrollMaxX and scrollMaxY to elements (v2)

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.
Comment 6 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-06-22 16:44:43 PDT
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.
Comment 7 Kartikaya Gupta (email:kats@mozilla.com) 2012-06-25 11:36:50 PDT
Created attachment 636415 [details] [diff] [review]
Add scrollLeftMax and scrollTopMax to elements (v3)

Updated to be scrollLeftMax and scrollTopMax as requested.
Comment 8 Kartikaya Gupta (email:kats@mozilla.com) 2012-06-26 12:40:49 PDT
Should I land this patch, or do you want to wait longer for feedback on www-style?
Comment 9 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-06-26 21:20:14 PDT
You should land it. There's no feedback on www-style yet and I don't expect to get any.
Comment 10 Kartikaya Gupta (email:kats@mozilla.com) 2012-06-26 22:31:40 PDT
Landed on inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/01c8479cf333
Comment 11 Ed Morley [:emorley] 2012-06-28 01:11:44 PDT
https://hg.mozilla.org/mozilla-central/rev/01c8479cf333

Note You need to log in before you can comment on or make changes to this bug.