Closed
Bug 868316
Opened 12 years ago
Closed 12 years ago
New scrollbars are not exactly the right dimensions
Categories
(Core :: Widget: Cocoa, defect)
Tracking
()
VERIFIED
FIXED
mozilla24
Tracking | Status | |
---|---|---|
firefox22 | --- | unaffected |
firefox23 | + | verified |
firefox24 | --- | verified |
People
(Reporter: vporof, Assigned: jsbruner)
References
Details
(Whiteboard: [lion-scrollbars+])
Attachments
(3 files, 9 obsolete files)
205.98 KB,
image/jpeg
|
Details | |
946 bytes,
patch
|
mstange
:
review+
spohl
:
feedback+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
231.25 KB,
image/jpeg
|
Details |
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•12 years ago
|
||
Reporter | ||
Comment 2•12 years ago
|
||
Reporter | ||
Comment 3•12 years ago
|
||
Reporter | ||
Comment 4•12 years ago
|
||
Updated•12 years ago
|
Assignee | ||
Comment 5•12 years ago
|
||
Created a mockup demonstrating the proper dimensions/current dimensions. Confirmed.
Assignee: nobody → josiah
Status: NEW → ASSIGNED
Assignee | ||
Updated•12 years ago
|
Attachment #745160 -
Attachment is obsolete: true
Assignee | ||
Comment 7•12 years ago
|
||
:( 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
Assignee | ||
Comment 8•12 years ago
|
||
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
Updated•12 years ago
|
Status: ASSIGNED → NEW
status-firefox23:
affected → ---
Hardware: All → x86
Version: 23 Branch → unspecified
Updated•12 years ago
|
Status: NEW → ASSIGNED
status-firefox23:
--- → affected
Hardware: x86 → All
Version: unspecified → 23 Branch
Assignee | ||
Comment 10•12 years ago
|
||
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•12 years ago
|
||
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.
Assignee | ||
Comment 12•12 years ago
|
||
Patch fixes all dimensional issues. Flagging Stephen Pohl for review.
Attachment #749318 -
Flags: review?(spohl)
Comment 13•12 years ago
|
||
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•12 years ago
|
||
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+
Assignee | ||
Comment 15•12 years ago
|
||
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
Assignee | ||
Comment 16•12 years ago
|
||
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)
Assignee | ||
Comment 17•12 years ago
|
||
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 18•12 years ago
|
||
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.
Assignee | ||
Comment 19•12 years ago
|
||
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 20•12 years ago
|
||
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 21•12 years ago
|
||
Comment on attachment 749434 [details] [diff] [review] Fixes all issues. I can.
Attachment #749434 -
Flags: review?(smichaud) → review+
Assignee | ||
Comment 22•12 years ago
|
||
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
Assignee | ||
Updated•12 years ago
|
Keywords: checkin-needed
Comment 23•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/f33e89758cc4
Keywords: checkin-needed
Comment 24•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f33e89758cc4
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Comment 25•12 years ago
|
||
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)
Assignee | ||
Comment 26•12 years ago
|
||
(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)
Updated•12 years ago
|
Keywords: branch-patch-needed
Assignee | ||
Comment 27•12 years ago
|
||
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?
Updated•12 years ago
|
Keywords: branch-patch-needed
![]() |
||
Updated•12 years ago
|
tracking-firefox23:
--- → ?
Whiteboard: [lion-scrollbars+]
Updated•12 years ago
|
Updated•12 years ago
|
Attachment #749434 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 29•12 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/9c8ea6216122
Keywords: checkin-needed
Comment 30•11 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.
Comment 32•11 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
You need to log in
before you can comment on or make changes to this bug.
Description
•