868416 - Scrollbar size should not be altered by page-zoom

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Anthony Ricaud (:rik)]

Attachment: Screenshot of the issue

If you're using a zoom level greater than 100% on a page, the scrollbars are larger than they should be. See attached screenshot.

Comment 1

[Joris Muller]

I have the same issue. Here, the details :

User Agent: Build identifier: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:23.0) Gecko/20130508 Firefox/23.0

Steps to reproduce: Open any webpage and zoomm in or zoom out ("Cmd"-"+" or "Cmd"-"-"). This bug appear with the integration of Lion's (MacOS X.7) Overlay Scrollbars.

Actual results:
All the scrollbars of the page(vertical, horizontal ones, of the navigator window and in a text box for example)  gain width when zoom in and loose width when zoom out.

Expected results:
Constant width of the scroll bar.

Comment 2

[(no longer active)]

Attachment: WIP

This is the beginning of a patch, which mostly works fine, except that the zoomed scrollbar's background is a bit too short.  For example, for the vertical scrollbar, the background does not span the entire height of the document.  Stephen, do you have any idea what else I need to handle?

Comment 3

[Stephen A Pohl [:spohl]]

Comment on attachment 751277 [details] [diff] [review]
WIP

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

Thank you for taking this on!

I can't reproduce the background being too short (or I'm just not seeing it). Is it possible that you're in a scenario with both vertical and horizontal scrollbars? In that case, the bottom right corner is 'empty' in order for the scrollbars to not overlap. This matches what Safari is doing.

What I did notice however is that the thumb is still affected by the zoom level. If the page is zoomed in and scrolled to the bottom, the thumb is only half way down the page. The opposite is the case when the page is zoomed out.

When scrolling in and out, the width of the scrollbars change minimally, but noticeably. A rounding error maybe, resulting in different widths being returned?

The only other thing I noticed is that your patch doesn't currently work with HiDPI. The scrollbars are barely visible (1px?) in HiDPI mode.

Could you use GetMinimumWidgetSize to determine the width and height of the scrollbar instead of determining the zoomRatio and risking rounding errors?

Comment 4

[Markus Stange [:mstange]]

I wonder what bug 636564 changed, this should have been fixed by bug 800668...
Maybe on of the changes in nsGfxScrollFrameInner::LayoutScrollbars caused this. There is a CSSPixelsToAppUnits(15) in there, which could potentially be affected by page zoom.

Comment 5

[(no longer active)]

Changing this code in GetMinimumWidgetSize doesn't fix the bug...

Comment 6

[(no longer active)]

OK, the problem is that when the margins specified here <https://hg.mozilla.org/mozilla-central/rev/0c513ba74137#l22.23> deflate the scrollbar rects in nsGfxScrollFrameInner::LayoutScrollbars, we get huge rectangles in zoomed mode because those values are specified in px.

Unassigning as I'm not sure what the best way to solve that is.

Comment 8

[Markus Stange [:mstange]]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #6)
> OK, the problem is that when the margins specified here
> <https://hg.mozilla.org/mozilla-central/rev/0c513ba74137#l22.23> deflate the
> scrollbar rects

Oh, I see. Hmm, so -moz-margin-start: -16px should really be -moz-margin-start: -15devpx... or more generally, "used width"...

Or maybe we should use the system metric "use overlay scrollbars" to make nsGfxScrollFrameInner apply the correct negative margins automatically, and take them out of the CSS. Roc, any ideas?

Comment 9

[Richard Marti (:Paenglab)]

Is bug 401213 affected by the same reason as this bug?

Comment 10

[Stephen A Pohl [:spohl]]

Attachment: Wip

This patch seems to fix this bug. It may be too narrow for what we need to do and we may want to address it more generally (see comment 8).

Requesting feedback to get roc's view on this patch and comment 8.

Comment 11

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

(In reply to Markus Stange from comment #8)
> Or maybe we should use the system metric "use overlay scrollbars" to make
> nsGfxScrollFrameInner apply the correct negative margins automatically, and
> take them out of the CSS. Roc, any ideas?

That sounds like the best option. So something like Stephen's patch, but for overlay scrollbars remove the existing CSS margin and just add a negative margin of the width we know it should have.

Comment 13

[Stephen A Pohl [:spohl]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #11)
> That sounds like the best option. So something like Stephen's patch, but for
> overlay scrollbars remove the existing CSS margin and just add a negative
> margin of the width we know it should have.

Could you help me understand your feedback a bit better? I might be repeating obvious things here, please bear with me:

The negative margin for overlay scrollbars should change depending on the zoom level. In the case of the vertical scrollbar, margin.left should be >= -960. -960 is the -16px times 60 app units per CSS pixel at 0 zoom level. Getting the regular margin first and then scaling as my patch is doing (for overlay scrollbars only) seemed reasonable since this would continue to be correct even if the -16px margin in the CSS changed in the future.

Are you saying that instead of dividing -960 back to -16px and then scaling based on the zoom level we should query the theme for the -16px directly and then scale? Or are you suggesting we generate this negative margin somewhere/somehow else?

Comment 14

[Markus Stange [:mstange]]

(In reply to Stephen Pohl [:spohl] from comment #13)
> Or are you suggesting we generate this negative margin
> somewhere/somehow else?

My suggestion was to override any margin specified in the CSS and hardcode it to "equal to the negative used width" - but only when the overlay scrollbars pref is set, of course.
In LayoutScrollbars can get the size of the scrollbar using scrollbarbox->GetPrefSize. In the other places where you apply the margin, you can probably just set the width or height of the resulting box to zero instead.

Comment 15

[Stephen A Pohl [:spohl]]

Attachment: Patch

Incorporated Markus' feedback. I kept this patch strictly limited to LayoutScrollbars for now. It seems to be all we need to fix the bug here.

Confirmed to be working with HiDPI on and off as well as positive and negative zoom.

Comment 16

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

Comment on attachment 752926 [details] [diff] [review]
Patch

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

::: layout/generic/nsGfxScrollFrame.cpp
@@ +3716,5 @@
>        nsMargin margin;
>        mVScrollbarBox->GetMargin(margin);
> +      if (LookAndFeel::GetInt(LookAndFeel::eIntID_UseOverlayScrollbars) != 0) {
> +        nscoord left = -1 * mVScrollbarBox->GetPrefSize(aState).width;
> +        margin.left = std::max(margin.left, left);

I think you should just be setting margin here, not overwriting it. Then we can remove the negative margin from the stylesheet.

Comment 17

[Stephen A Pohl [:spohl]]

Attachment: Patch

Removed margin for overlay scrollbars from stylesheet and defaulting to (0, 0, 0, 0) margin in LayoutScrollbars. This seems to work properly everywhere I tested it.

Comment 18

[Stephen A Pohl [:spohl]]

Comment on attachment 753351 [details] [diff] [review]
Patch

Argh, I focused my attention on the size of the scrollbars and missed the fact that we started to draw a white background for the scrollbars.

Comment 19

[Stephen A Pohl [:spohl]]

Attachment: Patch

This seems to work properly now. Flagging mstange for feedback to make sure that I incorporated all of his comments from our conversation on IRC.

Comment 20

[Markus Stange [:mstange]]

Comment on attachment 753459 [details] [diff] [review]
Patch

This is what I had in mind, yes.

Roc: To clarify, should we ignore any CSS margins on the scrollbar completely?

>diff --git a/layout/generic/nsGfxScrollFrame.cpp b/layout/generic/nsGfxScrollFrame.cpp
>--- a/layout/generic/nsGfxScrollFrame.cpp
>+++ b/layout/generic/nsGfxScrollFrame.cpp
>@@ -219,34 +219,32 @@ static void
> GetScrollbarMetrics(nsBoxLayoutState& aState, nsIFrame* aBox, nsSize* aMin,
>                     nsSize* aPref, bool aVertical)
> {
>   NS_ASSERTION(aState.GetRenderingContext(),
>                "Must have rendering context in layout state for size "
>                "computations");
> 
>   if (aMin) {
>-    *aMin = aBox->GetMinSize(aState);
>-    nsBox::AddMargin(aBox, *aMin);
>-    if (aMin->width < 0) {
>-      aMin->width = 0;
>+    if (LookAndFeel::GetInt(LookAndFeel::eIntID_UseOverlayScrollbars) == 0) {
>+      *aMin = aBox->GetMinSize(aState);
>+      nsBox::AddMargin(aBox, *aMin);

If we want to be consistent with ignoring CSS margins, this line should be removed, too.

>     }
>-    if (aMin->height < 0) {
>-      aMin->height = 0;
>+    else {
>+      *aMin = nsSize(0, 0);

maybe simpler as aMin->SizeTo(0, 0);, your choice

>+        nsMargin margin(0, 0, 0, 0);
>+        vRect.Deflate(margin);

This does exactly nothing. :)

>+      }
>+      else {

} else {

>+        vRect.x -= mVScrollbarBox->GetPrefSize(aState).width;
>+        vRect.width += mVScrollbarBox->GetPrefSize(aState).width;

I think I would prefer:
nscoord prefWidth = mVScrollbarBox->GetPrefSize(aState).width;
vRect.Inflate(nsMargin(0, 0, 0, prefWidth));

Comment 21

[Stephen A Pohl [:spohl]]

Comment on attachment 753459 [details] [diff] [review]
Patch

Addressing Markus' feedback before asking for review again. :-)

Comment 22

[Stephen A Pohl [:spohl]]

Attachment: Patch

Addressed Markus' feedback. This patch assumes that we want to ignore margins for scrollbars that are specified in CSS.

Markus, always feel free to chime in if I missed anything. Thanks!

Comment 23

[Markus Stange [:mstange]]

Comment on attachment 753477 [details] [diff] [review]
Patch

>+        nscoord prefWidth = mVScrollbarBox->GetPrefSize(aState).width;
>+        vRect.Inflate(nsMargin(0, 0, 0, prefWidth));

I just realized that this might not work in right-to-left mode. The CSS handled this by using -moz-margin-start instead of margin-left, but now it may need to be handled explicitly.
The only place where I've been able to observe a scrollbar on the left is in the locationbar autocomplete dropdown after setting intl.uidirection.en to rtl in about:config.

Oh no, and now I've noticed still another problem: Removing the CSS margin breaks the overlay effect for XUL trees, which don't have an nsGfxScrollbarFrame to layout their scrollbars.

Comment 24

[Stephen A Pohl [:spohl]]

... and we're running into a bunch of failures on try, on all platforms:
https://tbpl.mozilla.org/?tree=Try&rev=8f9725c03201

I guess the question I have for roc is: Should we continue down this path, or go back to a patch similar to the one reviewed in comment 16?

Comment 25

[Markus Stange [:mstange]]

(In reply to Stephen Pohl [:spohl] from comment #24)
> ... and we're running into a bunch of failures on try, on all platforms:

So devtools use negative margins on scrollbars (floating-scrollbars.css) to achieve floating scrollbars even on platforms where the pref is not set. Good to know.

Comment 26

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

How about for non-overlay scrollbars you continue doing what we're already doing, and for overlay scrollbars, treat the preferred/min width as 0 instead of calling GetPrefSize/GetMinSize, and remove the negative margins from the stylesheet?

Comment 27

[Stephen A Pohl [:spohl]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #26)
> How about for non-overlay scrollbars you continue doing what we're already
> doing, and for overlay scrollbars, treat the preferred/min width as 0
> instead of calling GetPrefSize/GetMinSize, and remove the negative margins
> from the stylesheet?

Wouldn't this still leave us with the problems mentioned in comment 23? I'm not worried about the RTL support, since we can be explicit there. But what about the overlay effect for XUL trees?

Comment 28

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

I'm not sure what to do about XUL trees. Hmm.

Maybe we should modify computation of scrollbar frames' margins during reflow to override the style system's margins with a negative margin equal to the frame's width/height. Say in nsCSSOffsetState::ComputeMargin.

Comment 29

[Markus Stange [:mstange]]

Or maybe we should just keep the negative margins in the CSS as an additional measure for trees. Trees are usually not shown in any zoomable context, so this bug doesn't really apply to them. So, to sum up:
 - Start with the current patch
 - Add the old margin handling back in, but make it only apply when the overlay
   pref isn't set, so that devtools scrollbars on Linux/Windows work again
 - Add the old CSS back in so that XUL trees work again

How does that sound?

Comment 30

[Stephen A Pohl [:spohl]]

Attachment: Wip that computes margins in nsCSSOffsetState::ComputeMargin

This is a work in progress that computes margins in nsCSSOffsetState::ComputeMargin without margins specified in CSS. One of the problems I ran into with this approach is that during reflow, the scrollable frame is reflown before the scrollbars (initiated in PresShell::ProcessReflowCommands). This means that the new margins were computed only after the scrollbars had been laid out (resulting in zoomed scrollbars). The way I currently work around this is by initiating a reflow of the scrollbars during the reflow of the scrollable frame in GetScrollbarMetrics.

One thing that still doesn't work is switching between regular and overlay scrollbars (bug 868498), and I haven't made this work for XUL trees yet.

Comment 31

[Stephen A Pohl [:spohl]]

(In reply to Markus Stange from comment #29)
> Or maybe we should just keep the negative margins in the CSS as an
> additional measure for trees. Trees are usually not shown in any zoomable
> context, so this bug doesn't really apply to them. So, to sum up:
>  - Start with the current patch
>  - Add the old margin handling back in, but make it only apply when the
> overlay
>    pref isn't set, so that devtools scrollbars on Linux/Windows work again
>  - Add the old CSS back in so that XUL trees work again
> 
> How does that sound?

Based on the fact that the wip patch in comment 30 is quickly becoming messy (forcing reflows of scrollbars during reflow of scrollable frame, etc.), I like this idea. On the other hand, there may be clean ways to work around the problems I'm facing in the wip patch.

roc, would you mind telling me what path I should proceed on? Thanks!

Comment 32

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

(In reply to Markus Stange from comment #29)
> Or maybe we should just keep the negative margins in the CSS as an
> additional measure for trees. Trees are usually not shown in any zoomable
> context, so this bug doesn't really apply to them. So, to sum up:
>  - Start with the current patch
>  - Add the old margin handling back in, but make it only apply when the
> overlay
>    pref isn't set, so that devtools scrollbars on Linux/Windows work again
>  - Add the old CSS back in so that XUL trees work again
> 
> How does that sound?

This won't work on High-DPI systems where 1 CSS pixel != 1 device pixel by default.

Comment 33

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

(In reply to Stephen Pohl [:spohl] from comment #30)
> Created attachment 759499 [details] [diff] [review]
> Wip that computes margins in nsCSSOffsetState::ComputeMargin
> 
> This is a work in progress that computes margins in
> nsCSSOffsetState::ComputeMargin without margins specified in CSS. One of the
> problems I ran into with this approach is that during reflow, the scrollable
> frame is reflown before the scrollbars (initiated in
> PresShell::ProcessReflowCommands). This means that the new margins were
> computed only after the scrollbars had been laid out (resulting in zoomed
> scrollbars). The way I currently work around this is by initiating a reflow
> of the scrollbars during the reflow of the scrollable frame in
> GetScrollbarMetrics.
> 
> One thing that still doesn't work is switching between regular and overlay
> scrollbars (bug 868498), and I haven't made this work for XUL trees yet.

For the overlay scrollbars case, can't we just ask the theme directly for the width instead of reflowing the scrollbar to find out what it is?

Comment 34

[Markus Stange [:mstange]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #32)
> (In reply to Markus Stange from comment #29)
> > Or maybe we should just keep the negative margins in the CSS as an
> > additional measure for trees. Trees are usually not shown in any zoomable
> > context, so this bug doesn't really apply to them. So, to sum up:
> >  - Start with the current patch
> >  - Add the old margin handling back in, but make it only apply when the
> > overlay
> >    pref isn't set, so that devtools scrollbars on Linux/Windows work again
> >  - Add the old CSS back in so that XUL trees work again
> > 
> > How does that sound?
> 
> This won't work on High-DPI systems where 1 CSS pixel != 1 device pixel by
> default.

But in that case, at least on Mac the native theme scales up the scrollbar size so that it corresponds to CSS pixels again. Overlay scrollbars return a width of 16devpx in LoDPI mode and 32devpx in HiDPI mode. So it should still work.

Comment 35

[Markus Stange [:mstange]]

(In reply to Markus Stange from comment #34)
> But in that case, at least on Mac the native theme scales up the scrollbar
> size so that it corresponds to CSS pixels

or rather, to "screen points", which usually correspond to CSS pixels when the page is not zoomed.

Comment 36

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

(In reply to Markus Stange from comment #34)
> But in that case, at least on Mac the native theme scales up the scrollbar
> size so that it corresponds to CSS pixels again. Overlay scrollbars return a
> width of 16devpx in LoDPI mode and 32devpx in HiDPI mode. So it should still
> work.

I don't think we should rely on that.

Comment 37

[Markus Stange [:mstange]]

Fair enough.

Comment 38

[Stephen A Pohl [:spohl]]

Attachment: Patch

This patch eliminates the margins from the CSS completely. Works with XUL trees and RTL.

This patch also works around the flickering of scrollbars in the page info dialog (bug 870917).

Comment 39

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

Comment on attachment 766922 [details] [diff] [review]
Patch

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

::: layout/generic/nsGfxScrollFrame.cpp
@@ +69,5 @@
> +/*static*/ nsMargin
> +nsGfxScrollFrameUtility::GetMargin(nsIFrame* aBox,
> +                                   nsSize* aSize,
> +                                   bool aVertical,
> +                                   bool aScrollbarOnRight)

This could just be a static method defined right here, couldn't it?

::: layout/generic/nsGfxScrollFrame.h
@@ +49,5 @@
> +{
> +public:
> +  static nsMargin GetMargin(nsIFrame* aBox, nsSize* aSize, bool aVertical,
> +                            bool aScrollbarOnRight);
> +};

Why does this need to be exposed in nsGfxScrollFrame.h?

::: layout/xul/base/src/nsBox.cpp
@@ +382,5 @@
> +        else {
> +          aMargin.left = -1 * mRect.Width();
> +        }
> +      }
> +      return NS_OK;

Can't nsScrollbarFrame override GetMargin and check LookAndFeel directly?

::: layout/xul/base/src/nsScrollbarFrame.cpp
@@ +50,5 @@
>    mState |= NS_FRAME_REFLOW_ROOT;
> +
> +  if (LookAndFeel::GetInt(LookAndFeel::eIntID_UseOverlayScrollbars) != 0) {
> +    aContent->SetAttr(kNameSpaceID_None, nsGkAtoms::overlay_scrollbars,
> +                      NS_LITERAL_STRING("1"), false);

It is not a good idea to mutate the DOM like this.

Why do we even need this attribute? We can just check LookAndFeel directly instead?

Comment 40

[Stephen A Pohl [:spohl]]

Attachment: Patch

Thanks :roc! I agree with all of your points. Your feedback should be addressed in this patch.

Comment 41

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

Comment on attachment 767556 [details] [diff] [review]
Patch

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

::: layout/generic/nsGfxScrollFrame.cpp
@@ +87,5 @@
> +    }
> +    else {
> +      margin.top = -1 * aSize->height;
> +    }
> +  }

It's not clear to me why the nsGfxScrollFrame changes are needed. aBox->GetMargin should return the correct negative margin for nsScrollbarFrames now, shouldn't it?

::: layout/xul/base/src/nsScrollbarFrame.cpp
@@ +171,5 @@
> +      else {
> +        aMargin.left = -1 * mRect.Width();
> +      }
> +    }
> +    return NS_OK;

Making the margin depend on mRect is bad.

I think in this case you should call GetPrefSize and negate its dimensions.

Comment 42

[Stephen A Pohl [:spohl]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #41)
> ::: layout/xul/base/src/nsScrollbarFrame.cpp
> @@ +171,5 @@
> > +      else {
> > +        aMargin.left = -1 * mRect.Width();
> > +      }
> > +    }
> > +    return NS_OK;
> 
> Making the margin depend on mRect is bad.
> 
> I think in this case you should call GetPrefSize and negate its dimensions.

Calling GetPrefSize requires an nsBoxLayoutState. I could pass this to GetMargin and therefore overload it, but then I wouldn't be overriding nsBox::GetMargin. We also call GetMargin in the GetMinSize case (see GetScrollbarMetrics in nsGfxScrollFrame.cpp), so this would either be a special case or a new function. I'd have to call GetMinSize instead of GetPrefSize and negate its dimensions. Is this what I should do?

> ::: layout/generic/nsGfxScrollFrame.cpp
> @@ +87,5 @@
> > +    }
> > +    else {
> > +      margin.top = -1 * aSize->height;
> > +    }
> > +  }
> 
> It's not clear to me why the nsGfxScrollFrame changes are needed.
> aBox->GetMargin should return the correct negative margin for
> nsScrollbarFrames now, shouldn't it?

With an overloaded GetMargin that accepts an nsBoxLayoutState parameter, yes it would.

Comment 43

[Stephen A Pohl [:spohl]]

(In reply to Stephen Pohl [:spohl] from comment #42)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #41)
> > ::: layout/generic/nsGfxScrollFrame.cpp
> > @@ +87,5 @@
> > > +    }
> > > +    else {
> > > +      margin.top = -1 * aSize->height;
> > > +    }
> > > +  }
> > 
> > It's not clear to me why the nsGfxScrollFrame changes are needed.
> > aBox->GetMargin should return the correct negative margin for
> > nsScrollbarFrames now, shouldn't it?
> 
> With an overloaded GetMargin that accepts an nsBoxLayoutState parameter, yes
> it would.

... but it would not work for XUL trees anymore (which requires GetMargin to be overridden). I'd appreciate some guidance as to how to move forward here. Maybe the solution in comment 40 is acceptable based on this?

Comment 44

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

(In reply to Stephen Pohl [:spohl] from comment #42)
> Calling GetPrefSize requires an nsBoxLayoutState. I could pass this to
> GetMargin and therefore overload it, but then I wouldn't be overriding
> nsBox::GetMargin. We also call GetMargin in the GetMinSize case (see
> GetScrollbarMetrics in nsGfxScrollFrame.cpp), so this would either be a
> special case or a new function. I'd have to call GetMinSize instead of
> GetPrefSize and negate its dimensions. Is this what I should do?

OK then, have nsScrollbarFrame::GetMargin call into the theme to get the scrollbar size and use that.

Comment 45

[Stephen A Pohl [:spohl]]

Attachment: Patch

Addressed feedback and made scrollbars appear in the correct place in RTL.

Comment 46

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

Comment on attachment 768134 [details] [diff] [review]
Patch

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

Hurrah!

::: layout/xul/base/src/nsScrollbarFrame.cpp
@@ +167,5 @@
> +    if (theme) {
> +      nsIntSize size;
> +      bool isOverridable;
> +      nsRefPtr<nsRenderingContext> rc =
> +        PresContext()->PresShell()->GetReferenceRenderingContext();

Factor out calls to PresContext() into a local variable.

@@ +171,5 @@
> +        PresContext()->PresShell()->GetReferenceRenderingContext();
> +      theme->GetMinimumWidgetSize(rc, this, NS_THEME_SCROLLBAR, &size,
> +                                  &isOverridable);
> +      if (IsHorizontal()) {
> +        aMargin.top = -1 * PresContext()->DevPixelsToAppUnits(size.height);

Just -presContext->DevPixelsToAppUnits(size.height) (and likewise below).

Comment 47

[Stephen A Pohl [:spohl]]

Attachment: Patch

Thanks roc! Addressed review feedback. Carrying over r+.

Comment 48

[Stephen A Pohl [:spohl]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/32e423bcd381

Comment 49

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/32e423bcd381

Comment 50

[Stephen A Pohl [:spohl]]

Comment on attachment 768137 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 636564
User impact if declined: Overlay scrollbars change size when the web page is zoomed in/out on Mac OSX.
Testing completed (on m-c, etc.): Has been on trunk for a few days. I've also verified that scrollbars keep the correct size in recent nightlies.
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none

Comment 51

[Stephen A Pohl [:spohl]]

Requesting checkin for aurora.

Comment 52

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e4f20ca64ee3

Does this need to go on beta too? Please request approval if so or set status-firefox23 to wontfix if not.

Comment 53

[Stephen A Pohl [:spohl]]

Comment on attachment 768137 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 636564
User impact if declined: Overlay scrollbars change size when the web page is zoomed in/out on Mac OSX.
Testing completed (on m-c, etc.): Has been on trunk for a few days. I've also verified that scrollbars keep the correct size in recent nightlies.
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none

Comment 54

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/76df955550e3

Comment 55

[Stephen A Pohl [:spohl]]

Thank you, RyanVM!

Comment 56

[Simona Badau, Desktop QA]

I tried to reproduce this issue several times on Mac OS X 10.7 and on Mac OS X 10.8 on a 05/08 Nightly 23 but I can't. I don't see any difference in the size of the scrollbars when normal and zoomed.

If anyone else in QA can reproduce this bug, please verify the fix.

Comment 58

[Tiziana Sellitto [:tiziana]]

I don't see anymore the problem Verified Fixed with 

Beta:
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:23.0) Gecko/20100101 Firefox/23.0
BuildID: 20130711122148

Aurora:
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:24.0) Gecko/20130711 Firefox/24.0
buildID: 20130717004002

Nightly:
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:25.0) Gecko/20130715 Firefox/25.0
buildID:20130717030207