New scrollbars are not exactly the right dimensions

VERIFIED FIXED in Firefox 23

Status

()

Core
Widget: Cocoa
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: vporof, Assigned: JosiahOne)

Tracking

23 Branch
mozilla24
All
Mac OS X
Points:
---

Firefox Tracking Flags

(firefox22 unaffected, firefox23+ verified, firefox24 verified)

Details

(Whiteboard: [lion-scrollbars+])

Attachments

(3 attachments, 9 obsolete attachments)

205.98 KB, image/jpeg
Details
946 bytes, patch
mstange
: review+
spohl
: feedback+
Details | Diff | Splinter Review
231.25 KB, image/jpeg
Details
(Reporter)

Description

5 years ago
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.
(Reporter)

Comment 1

5 years ago
Created attachment 745021 [details]
small mode, firefox
(Reporter)

Comment 2

5 years ago
Created attachment 745022 [details]
small mode, other app
(Reporter)

Comment 3

5 years ago
Created attachment 745024 [details]
drag mode, firefox
(Reporter)

Comment 4

5 years ago
Created attachment 745025 [details]
drag mode, other app
(Reporter)

Updated

5 years ago
Blocks: 636564
status-firefox23: --- → affected
Hardware: x86 → All
Version: unspecified → 23 Branch
Created attachment 745160 [details]
Dimmension issues / Corrections

Created a mockup demonstrating the proper dimensions/current dimensions. Confirmed.
Assignee: nobody → josiah
Status: NEW → ASSIGNED
Created attachment 745266 [details]
Dimmension issues / Corrections (Update 1)

Whoops. Got a wrong size.
Attachment #745160 - Attachment is obsolete: true
Created attachment 745271 [details]
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!
Attachment #745266 - Attachment is obsolete: true
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...
Attachment #745271 - Attachment is obsolete: true
Created attachment 745858 [details]
Dimmension issues / Corrections (Update 3)

Here we go.
Status: ASSIGNED → NEW
status-firefox23: affected → ---
Hardware: All → x86
Version: 23 Branch → unspecified
Status: NEW → ASSIGNED
status-firefox23: --- → affected
Hardware: x86 → All
Version: unspecified → 23 Branch
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?
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.
Created attachment 749318 [details] [diff] [review]
Fixes all issues.

Patch fixes all dimensional issues. Flagging Stephen Pohl for review.
Attachment #749318 - Flags: review?(spohl)
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 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.
Attachment #749318 - Flags: review?(spohl) → feedback+
Created attachment 749390 [details] [diff] [review]
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.
Attachment #749318 - Attachment is obsolete: true
Comment on attachment 749390 [details] [diff] [review]
Fixes all issues.

># HG changeset patch
># User JosiahOne <josiah@programmer.net>
># 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);
Attachment #749390 - Flags: review?(spohl)
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.
Attachment #749390 - Attachment description: Fixes all issues. (Not tested on Lion) → Fixes all issues.
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.
Created attachment 749434 [details] [diff] [review]
Fixes all issues.

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

Fixed.

Flagging for review.
Attachment #749390 - Attachment is obsolete: true
Attachment #749390 - Flags: review?(spohl)
Attachment #749434 - Flags: review?(spohl.mozilla.bugs)
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.
Attachment #749434 - Flags: review?(spohl.mozilla.bugs)
Attachment #749434 - Flags: review?(smichaud)
Attachment #749434 - Flags: feedback+
Comment on attachment 749434 [details] [diff] [review]
Fixes all issues.

I can.
Attachment #749434 - Flags: review?(smichaud) → review+
Created attachment 749458 [details]
New Dimmensions (After the fix)

Shows the new dimensions and how the correctly line up with Chrome.
Attachment #745021 - Attachment is obsolete: true
Attachment #745022 - Attachment is obsolete: true
Attachment #745024 - Attachment is obsolete: true
Attachment #745025 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f33e89758cc4
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Josiah, are you planning on putting up a patch for aurora23? This seems like a low-risk change that we could uplift.
Flags: needinfo?(josiah)
(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.
Flags: needinfo?(josiah)
Keywords: branch-patch-needed
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
Attachment #749434 - Flags: approval-mozilla-aurora?
Keywords: branch-patch-needed
tracking-firefox23: --- → ?
Whiteboard: [lion-scrollbars+]
status-firefox22: --- → unaffected
status-firefox24: --- → fixed
tracking-firefox23: ? → +
Attachment #749434 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Requesting checkin for Aurora.
Keywords: checkin-needed
https://hg.mozilla.org/releases/mozilla-aurora/rev/9c8ea6216122
status-firefox23: affected → fixed
Keywords: checkin-needed
Keywords: verifyme

Comment 30

5 years ago
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.
status-firefox23: fixed → verified
(Reporter)

Comment 31

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

Comment 32

5 years ago
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
Status: RESOLVED → VERIFIED
status-firefox24: fixed → verified

Updated

5 years ago
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.