672474 - Provide an API to retrieve the current document's scrollbar width

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement)

Description

[Raymond Lee [:raymondlee]]

It would be nice to have a way to get the width of browser scrollbars.  

When taking a screenshot of a tab, we want to get the width of vertical and horizontal scrollbars so we can remove them from screenshot.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Panorama needs this, the thumbnail service needs it and here's two more places where we use a quite bad hack to work around the lack of a proper API:

http://mxr.mozilla.org/mozilla-central/source/browser/devtools/webconsole/AutocompletePopup.jsm#366
http://mxr.mozilla.org/mozilla-central/source/browser/devtools/shared/DeveloperToolbar.jsm#765

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Another one, just FTR:

http://mxr.mozilla.org/mozilla-central/source/browser/devtools/commandline/gcli.jsm#9279

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Attachment: add nsIDOMWindowUtils.getScrollbarWidth(aFlushLayout)

Olli, I hope you're the right one to review this?

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 711073 [details] [diff] [review]
add nsIDOMWindowUtils.getScrollbarWidth(aFlushLayout)

Looks ok to me, but scrollbar handling is more roc's stuff.

One nit, nsIPresShell *presShell -> nsIPresShell* presShell

Comment 5

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

Comment on attachment 711073 [details] [diff] [review]
add nsIDOMWindowUtils.getScrollbarWidth(aFlushLayout)

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

The actual code looks OK.

::: dom/tests/mochitest/general/test_domWindowUtils_scrollbarWidth.html
@@ +22,5 @@
> +      is(utils.getScrollbarWidth(false), 0,
> +         "getScrollbarWidth returns zero with a vertical scrollbar w/o flushing");
> +
> +      ok(utils.getScrollbarWidth(true) > 0,
> +         "getScrollbarWidth returns non-zero with a vertical scrollbar with flushing");

I'm afraid this test will fail on B2G/Android (maybe other platforms later) where disappearing scrollbars are used.

Try doing something like an overflow:scroll float element containing a single child of known width. The float element should then get the width of the child plus the width of a scrollbar; you can use that to figure out what a scrollbar width should be.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #5)
> I'm afraid this test will fail on B2G/Android (maybe other platforms later)
> where disappearing scrollbars are used.

Good point.

> Try doing something like an overflow:scroll float element containing a
> single child of known width. The float element should then get the width of
> the child plus the width of a scrollbar; you can use that to figure out what
> a scrollbar width should be.

Ok, will do, thanks!


BTW, is there any reason this patch might not work with XUL documents / windows?   presShell->GetRootScrollFrameAsScrollable() does not seem to return a valid scrollFrame.

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Tim Taubert [:ttaubert] from comment #6)
> BTW, is there any reason this patch might not work with XUL documents /
> windows?   presShell->GetRootScrollFrameAsScrollable() does not seem to
> return a valid scrollFrame.

I have a ChromeWindow instance, to be specific.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

DOMWindowUtils.getScrollbarWidth() does also throw if I try to run the code in the Scratchpad.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

XUL docs don't have scrollable root, at least not by default.

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

There are a couple of API clients that would actually rather need to pass a specific DOMElement like a <xul:richlistbox> and get its scrollbar width, if any. I think it would make sense to add DOMWindowUtils.getScrollbarWidthForElement(nsIDOMElement aElement, bool aFlush)? Is it possible to retrieve an nsIScrollableFrame from a given DOMElement? I think the <xul:scrollbox> is actually injected by XBL...

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #5)
> > +      is(utils.getScrollbarWidth(false), 0,
> > +         "getScrollbarWidth returns zero with a vertical scrollbar w/o flushing");
> > +
> > +      ok(utils.getScrollbarWidth(true) > 0,
> > +         "getScrollbarWidth returns non-zero with a vertical scrollbar with flushing");
> 
> I'm afraid this test will fail on B2G/Android (maybe other platforms later)
> where disappearing scrollbars are used.
> 
> Try doing something like an overflow:scroll float element containing a
> single child of known width. The float element should then get the width of
> the child plus the width of a scrollbar; you can use that to figure out what
> a scrollbar width should be.

Wait, how does that work if I'm trying to get the root frame's scrollbar width? There's no way I can make such a test for a root's scrollbar, right?

Comment 12

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

You can assume all scrollbars have the same width.

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

But wouldn't .getScrollbarWidth() always return 0 if the root doesn't have scrollbars?

Comment 14

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

I'm suggesting using the float trick to figure out if the platform uses disappearing scrollbars or not. Once you know that, you can skip the part of your test that assumes scrollbars have width.

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

Oh, I get it know. Thank you for explaining :)

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

*now

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

Attachment: add nsIDOMWindowUtils.getScrollbarWidth(aFlushLayout)

Added proposed test for floating scrollbars.

Comment 18

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/eba916ef39c9

Comment 19

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/eba916ef39c9

Comment 20

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 713372 [details] [diff] [review]
add nsIDOMWindowUtils.getScrollbarWidth(aFlushLayout)

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

::: dom/base/nsDOMWindowUtils.cpp
@@ +1450,5 @@
> +
> +  nsCOMPtr<nsPIDOMWindow> window = do_QueryReferent(mWindow);
> +  NS_ENSURE_STATE(window);
> +
> +  nsCOMPtr<nsIDocument> doc(do_QueryInterface(window->GetExtantDocument()));

For future reference, this is what GetExtantDoc() is for.

Comment 21

[Tim Taubert [:ttaubert] (inactive)]

Attachment: followup to use GetExtantDoc()

Thanks for the hint.

Comment 22

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 714358 [details] [diff] [review]
followup to use GetExtantDoc()

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

Thanks!

Comment 23

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/02524d3d9d8a

Comment 24

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/02524d3d9d8a