Last Comment Bug 656990 - [10.7] Ensure compatibility with OS X 10.7's arrowless scrollbar
: [10.7] Ensure compatibility with OS X 10.7's arrowless scrollbar
Status: VERIFIED FIXED
:
Product: Core
Classification: Components
Component: Widget: Cocoa (show other bugs)
: unspecified
: All Mac OS X
: -- normal (vote)
: ---
Assigned To: Steven Michaud [:smichaud] (Retired)
:
Mentors:
Depends on:
Blocks: lion-compatibility 636564
  Show dependency treegraph
 
Reported: 2011-05-13 12:42 PDT by Steven Michaud [:smichaud] (Retired)
Modified: 2013-12-27 14:21 PST (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
fixed
fixed


Attachments
Fix (10.92 KB, patch)
2011-05-13 13:06 PDT, Steven Michaud [:smichaud] (Retired)
mstange: review+
christian: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Steven Michaud [:smichaud] (Retired) 2011-05-13 12:42:18 PDT
This bug has been spun off from bug 636564.  It deals with one aspect
of support for OS X 10.7's (Lion's) new scrollbar style -- the new
scrollbars have only a thumb, and no arrow buttons.

Current widgets code doesn't know how to deal with scrollbars that
have no arrow buttons.  As a result, on Lion there's a blank space
where the buttons should be (if they were present).  And the "endcap"
(on the opposite end of the scrollbar from the arrow buttons) also
isn't drawn correctly.

Shortly I'll post a patch that fixes these problems.

As best I can tell, the only platform on which we need to display
arrowless scrollbars is OS X 10.7.  So most of my patch's changes will
be to Cocoa widgets code.
Comment 1 Steven Michaud [:smichaud] (Retired) 2011-05-13 13:06:38 PDT
Created attachment 532326 [details] [diff] [review]
Fix

Here's a patch that fixes this bug's problems in my tests.

It might not be necessary to change the nsILookAndFeel interface's IID
-- you could argue that I didn't actually change the interface.  But
it's probably better to play it safe and change the IID anyway.

A tryserver build will follow in several hours.
Comment 2 Steven Michaud [:smichaud] (Retired) 2011-05-14 09:44:21 PDT
Here's a tryserver build made with my patch from comment #1:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/smichaud@pobox.com-9ef7649cb40e/try-macosx64/firefox-6.0a1.en-US.mac.dmg

Please try it out.
Comment 3 Nomis101 2011-05-14 14:01:44 PDT
(In reply to comment #2)
> Here's a tryserver build made with my patch from comment #1:
> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/smichaud@pobox.com-
> 9ef7649cb40e/try-macosx64/firefox-6.0a1.en-US.mac.dmg
> 
> Please try it out.

With this build I get the normal 10.6 scrollbar on 10.6. On 10.7 I get a scrollbar that looks like the 10.7 scrollbar, but without any of the effects (it doesn't autohide...). And the scrollbar seems a bit more expanded compared to that in Safari and the Finder. The problem with the blank space compared to an unpatched build seems fixed.
Comment 4 Steven Michaud [:smichaud] (Retired) 2011-05-14 14:30:21 PDT
Thanks for testing.

> And the scrollbar seems a bit more expanded compared to that in
> Safari and the Finder.

You mean the scrollbar's width?

If so, yes.  The scrollbar's track (though not its thumb) is slightly
wider in Firefox (even with my patch) than it is in other programs
(like Safari) that support autohiding the scrollbar -- when autohiding
is turned on (the default setting).  But when you turn autohiding off
(globally, in the Appearance panel), other programs' (and Safari's)
scrollbars have the same width as Firefox's.

> The problem with the blank space compared to an unpatched build
> seems fixed.

Glad to hear it.
Comment 5 Nomis101 2011-05-14 14:53:41 PDT
(In reply to comment #4)
> Thanks for testing.
> 
> > And the scrollbar seems a bit more expanded compared to that in
> > Safari and the Finder.
> 
> You mean the scrollbar's width?
Yes.
Comment 6 Steven Michaud [:smichaud] (Retired) 2011-05-16 07:43:59 PDT
Comment on attachment 532326 [details] [diff] [review]
Fix

Markus, you've done a lot of work in this area.  Can you review this patch?
Comment 7 Markus Stange [:mstange] 2011-05-16 08:19:59 PDT
Comment on attachment 532326 [details] [diff] [review]
Fix

Sure, looks good.
Comment 8 Steven Michaud [:smichaud] (Retired) 2011-05-16 10:59:38 PDT
Comment on attachment 532326 [details] [diff] [review]
Fix

Landed on trunk:
http://hg.mozilla.org/mozilla-central/rev/890b0e24fa5e
Comment 9 Asa Dotzler [:asa] 2011-05-16 16:25:09 PDT
this bug number wasn't noted in the checkin comment.
Comment 10 Steven Michaud [:smichaud] (Retired) 2011-05-16 16:27:23 PDT
> this bug number wasn't noted in the checkin comment.

Oh shit!  Sorry.

Too bad we don't have any way to edit checkin comments.
Comment 11 Josh Aas 2011-06-10 14:13:14 PDT
Steven - can you make sure this patch works on aurora and beta and post new patches if necessary?
Comment 12 Josh Aas 2011-06-10 14:19:15 PDT
Actually, we're all set for Aurora already. Just need a beta patch.
Comment 13 Steven Michaud [:smichaud] (Retired) 2011-06-10 14:21:32 PDT
It was my understanding this was only going to be in Firefox 6 -- not in Firefox 5.
Comment 14 Josh Aas 2011-06-10 14:21:48 PDT
I really think we should take this for Firefox 5 even if it is last-minute. I don't want stable Firefox to have obvious visual glitches when Mac OS X 10.7 comes out.

Much of the logic in the patch only applies to 10.7+ (runtime decision).
Comment 15 Steven Michaud [:smichaud] (Retired) 2011-06-10 14:23:32 PDT
OK, I'll write up a patch and seek approval.
Comment 16 Asa Dotzler [:asa] 2011-06-10 14:33:36 PDT
I saw that 10.7 was slated for a July release. Do we know what day in July? If it's late, we'd have a solid beta of Firefox 6 for 10.7 users with this fix, right?
Comment 17 Steven Michaud [:smichaud] (Retired) 2011-06-10 14:35:48 PDT
But there's a problem -- my patch changes the nsILookAndFeel interface's IID, and I'm not sure that'll be allowed this late in the game.

You could argue (as I did in comment #1) that this change isn't really necessary.  But that introduces complexities that are awkward to deal with at such a late stage.

> I saw that 10.7 was slated for a July release. Do we know what day in July?

As far as I know we don't.  Apple can be very cagey about these things.
Comment 18 Josh Aas 2011-06-10 14:37:01 PDT
I don't think we know the exact day in July, but in any case I don't think we should be requiring Firefox users to use beta software in order to avoid bugs like this.
Comment 19 Steven Michaud [:smichaud] (Retired) 2011-06-10 14:45:17 PDT
Comment on attachment 532326 [details] [diff] [review]
Fix

Low risk patch that has been baking on the trunk for quite a while.

It fixes an obvious (and rather ugly) problem in OS X 10.7, which appears likely to come out before FF 6 is released.
Comment 20 Asa Dotzler [:asa] 2011-06-10 14:46:49 PDT
or "wait a couple of weeks" to get the not-beta.  I don't like it either, but Firefox 5 is done (what was essentially our RC build came out yesterday) and I'm having a hard time believing that we'd respin for this change.
Comment 21 christian 2011-06-10 15:11:48 PDT
Comment on attachment 532326 [details] [diff] [review]
Fix

approved for releases/mozilla-beta. We're not sure if we're going to respin again but it makes sense to get it in tree just in case.
Comment 22 Steven Michaud [:smichaud] (Retired) 2011-06-10 15:53:49 PDT
Landed on releases/mozilla-beta:
http://hg.mozilla.org/releases/mozilla-beta/rev/62deda8ae7c6

I won't change status-firefox5, because we're not yet sure this is going to get into FF 5.
Comment 23 christian 2011-06-10 15:59:44 PDT
Thanks!
Comment 24 christian 2011-06-13 14:15:55 PDT
Does this happen on 3.6?
Comment 25 Steven Michaud [:smichaud] (Retired) 2011-06-13 14:27:53 PDT
Yes, it happens in all older versions -- 4, 3.6 and 3.5.
Comment 26 Marcia Knous [:marcia - use ni] 2011-06-14 10:55:40 PDT
Verified fixed on the Beta 6 candidate - buildID=20110613165758.
Comment 27 Mihaela Velimiroviciu (:mihaelav) 2011-08-22 00:27:19 PDT
Considering comment26 setting resolution to VERIFIED-FIXED on Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:7.0) Gecko/20100101 Firefox/7.0
Comment 28 neil@parkwaycc.co.uk 2011-08-31 01:06:55 PDT
(In reply to Steven Michaud from comment #1)
> It might not be necessary to change the nsILookAndFeel interface's IID
> -- you could argue that I didn't actually change the interface.  But
> it's probably better to play it safe and change the IID anyway.
Opinions seem to be divided on this one. (My preference is not to change it.)

Note You need to log in before you can comment on or make changes to this bug.