868316 - New scrollbars are not exactly the right dimensions

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Victor Porof [:vporof][:vp]]

At least on OS X 10.8, the new scrollbars are not exactly the right dimensions compared to all other apps, both in the small and large (drag) modes. I attached a few screenshots for comparison:

1. In small mode, the scrollbar is a bit thicker.
2. In drag mode, the scrollbar misses a bit of padding on the left.

Comment 1

[Victor Porof [:vporof][:vp]]

Attachment: small mode, firefox

Comment 2

[Victor Porof [:vporof][:vp]]

Attachment: small mode, other app

Comment 3

[Victor Porof [:vporof][:vp]]

Attachment: drag mode, firefox

Comment 4

[Victor Porof [:vporof][:vp]]

Attachment: drag mode, other app

Comment 5

[Josiah Bruner [:jsbruner]]

Attachment: Dimmension issues / Corrections

Created a mockup demonstrating the proper dimensions/current dimensions. Confirmed.

Comment 6

[Josiah Bruner [:jsbruner]]

Attachment: Dimmension issues / Corrections (Update 1)

Whoops. Got a wrong size.

Comment 7

[Josiah Bruner [:jsbruner]]

Attachment: Dimmension issues / Corrections (Update 2)

:( Err, a few mistakes actually. Had zoom on apparently before. Here are the "actual" correct values.

Sorry for the spam guys!

Comment 8

[Josiah Bruner [:jsbruner]]

Comment on attachment 745271 [details]
Dimmension issues / Corrections (Update 2)

Dimensions are completely off right now, as I realized the images provided where zoomed-in. I'll create a new mockup with correct values shortly...

Comment 9

[Josiah Bruner [:jsbruner]]

Attachment: Dimmension issues / Corrections (Update 3)

Here we go.

Comment 10

[Josiah Bruner [:jsbruner]]

I seem to have encountered an interesting problem. Inside nativescrollbar.css I altered some rules to the following:

 scrollbar[orient="vertical"] {
    -moz-margin-start: -16px;
    min-width: 3px !important;
    width: 3px !important;
    max-width: 3px !important;
  }

  scrollbar[orient="horizontal"] {
    margin-top: -16px;
    min-height: 3px !important;
    height: 3px !important;
    max-height: 3px !important;
  }


/* ..... thumb ..... */

thumb {
  -moz-appearance: scrollbarthumb-horizontal;
    min-height: 3px !important;
    height: 3px !important;
    max-height: 3px !important;
}

thumb[orient="vertical"] {
  -moz-appearance: scrollbarthumb-vertical;
    min-width: 3px !important;
    width: 3px !important;
    max-width: 3px !important;
}

The 3px is for testing purposes. However, the width stays at 8px. Now, setting the width to say 20px does cause the proper change. I can not figure out why the size can't become lower than 8px. I have "!important" rules that should override any parent rules and DOMi isn't showing any other rules that could override this.

Markus, do you happen to know why the width can't become less than 8px?

Comment 11

[Markus Stange [:mstange]]

Probably something in nsNativeThemeCocoa::GetWidgetBorder or nsNativeThemeCocoa::GetMinimumWidgetSize - those things override anything you set in the CSS.

In any case, I think the fix for this bug will be somewhere in nsNativeThemeCocoa - either for the values in the functions I just referred to, or in the drawing code in nsNativeThemeCocoa::DrawWidgetBackground. The CSS stuff should already be correct.

Note also that you'll need to test any changes on both 10.7 and 10.8 because the places that CUIDraw actually draws the scrollbars in have changed between the OS X versions.

Comment 12

[Josiah Bruner [:jsbruner]]

Attachment: Fixes all issues.

Patch fixes all dimensional issues. Flagging Stephen Pohl for review.

Comment 13

[Markus Stange [:mstange]]

Comment on attachment 749318 [details] [diff] [review]
Fixes all issues.

So in both cases (hovered and unhovered) the thumb is 1px too wide at the inner edge. If you instead increase the top or left scrollbar border in GetWidgetBorder, does that have the same result?

Comment 14

[Stephen A Pohl [:spohl]]

Comment on attachment 749318 [details] [diff] [review]
Fixes all issues.

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

I agree with Markus. In nsNativeThemeCocoa::GetWidgetBorder, you may want to change:

      if (nsLookAndFeel::GetInt(
            nsLookAndFeel::eIntID_UseOverlayScrollbars) != 0) {
        if (isHorizontal) {
          aResult->SizeTo(1, 2, 1, 1);
        } else {
          aResult->SizeTo(2, 1, 1, 1);
        }
      }

to something like:

      if (nsLookAndFeel::GetInt(
            nsLookAndFeel::eIntID_UseOverlayScrollbars) != 0) {
        if (isHorizontal) {
          aResult->SizeTo(2, 1, 1, 1);
        } else {
          aResult->SizeTo(1, 1, 1, 2);
        }
      }

This may be all you need, but you will want to verify the pixel measurements on 10.7 and 10.8.

Comment 15

[Josiah Bruner [:jsbruner]]

Attachment: Fixes all issues.

Ah, this is a better way to do this...

I have verified this works on ML, will get someone to test on Lion next.

Comment 16

[Josiah Bruner [:jsbruner]]

Comment on attachment 749390 [details] [diff] [review]
Fixes all issues.

># HG changeset patch
># User JosiahOne <[e-mail]>
># Date 1368555637 14400
># Node ID dcdf0cf1c85989da60fd0ca3f373b1322995aad4
># Parent  07a6c0f47fcbefde720771e2919d7e9bc9ba6f6d
>Bug 868316 - Adjust scrollbar dimmensions to their OS value
>
>diff --git a/widget/cocoa/nsNativeThemeCocoa.mm b/widget/cocoa/nsNativeThemeCocoa.mm
>--- a/widget/cocoa/nsNativeThemeCocoa.mm
>+++ b/widget/cocoa/nsNativeThemeCocoa.mm
>@@ -2606,19 +2606,19 @@
>           else
>             aResult->SizeTo(endcapSize, 0, 0, 0);
>         }
>       }
> 
>       if (nsLookAndFeel::GetInt(
>             nsLookAndFeel::eIntID_UseOverlayScrollbars) != 0) {
>         if (isHorizontal) {
>-          aResult->SizeTo(1, 2, 1, 1);
>+          aResult->SizeTo(2, 1, 1, 1);
>         } else {
>-          aResult->SizeTo(2, 1, 1, 1);
>+          aResult->SizeTo(2, 1, 1, 2);
>         }
>       }
> 
>       break;
>     }
> 
>     case NS_THEME_STATUSBAR:
>       aResult->SizeTo(1, 0, 0, 0);

Comment 17

[Josiah Bruner [:jsbruner]]

Comment on attachment 749390 [details] [diff] [review]
Fixes all issues.

Got a test on Lion and it looks good. It was a little hard to test though, so if anyone finds something wrong, we can file a new bug for Lion only.

Comment 18

[Markus Stange [:mstange]]

Comment on attachment 749390 [details] [diff] [review]
Fixes all issues.

Oh, now I know why these values are off! Since I wrote these patches, bug 843719 changed the ordering - from (left top right bottom) to (top right bottom left)!

Josiah, have you checked whether the 2px at the top in the vertical case matches the native appearance? I'd expect a 1 there, to be symmetric with the 1 at the bottom.

Comment 19

[Josiah Bruner [:jsbruner]]

Attachment: Fixes all issues.

Ah, that makes more sense. Yes, 1px on top is correct.

Fixed.

Flagging for review.

Comment 20

[Stephen A Pohl [:spohl]]

Comment on attachment 749434 [details] [diff] [review]
Fixes all issues.

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

Looks good to me, but I can't r+ the patch. Sending it over to smichaud.

Comment 21

[Markus Stange [:mstange]]

Comment on attachment 749434 [details] [diff] [review]
Fixes all issues.

I can.

Comment 22

[Josiah Bruner [:jsbruner]]

Attachment: New Dimmensions (After the fix)

Shows the new dimensions and how the correctly line up with Chrome.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f33e89758cc4

Comment 24

[Ryan VanderMeulen [:RyanVM]]

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

Comment 25

[Jared Wein [:jaws] (please needinfo? me)]

Josiah, are you planning on putting up a patch for aurora23? This seems like a low-risk change that we could uplift.

Comment 26

[Josiah Bruner [:jsbruner]]

(In reply to Jared Wein [:jaws] from comment #25)
> Josiah, are you planning on putting up a patch for aurora23? This seems like
> a low-risk change that we could uplift.

Yes, that is a swell idea. First though, let's wait a few days to make sure there aren't any issues I missed concerning Lion.

Comment 27

[Josiah Bruner [:jsbruner]]

Comment on attachment 749434 [details] [diff] [review]
Fixes all issues.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 636564

User impact if declined: The new scroll bars will arrive to users without proper dimensions, causing OS-inconsistency.

Testing completed (on m-c, etc.): Already landed on m-c (https://hg.mozilla.org/integration/mozilla-inbound/rev/f33e89758cc4) and successfully built locally and on Try: https://tbpl.mozilla.org/?tree=Try&rev=09f95e211607

Risk to taking this patch (and alternatives if risky): Very low risk, worst that could happen is the scroll bar dimensions will be slightly off, but those can be fixed quickly on Aurora.

String or IDL/UUID changes made by this patch: None

Comment 28

[Josiah Bruner [:jsbruner]]

Requesting checkin for Aurora.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9c8ea6216122

Comment 30

[Ioana (away)]

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

I verified this fix on Firefox 23 beta 7. It looks fine to me, but since we're talking about 1-2 pixels differences, I might be missing stuff with the naked eye. If anyone else has the time to take a look and give a second opinion, please do so.

Comment 31

[Victor Porof [:vporof][:vp]]

I can confirm that they're the right dimensions now.

Comment 32

[Ioana (away)]

Verified as fixed on Firefox 24 beta 1:
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:24.0) Gecko/20100101 Firefox/24.0