Closed Bug 656990 Opened 13 years ago Closed 13 years ago

[10.7] Ensure compatibility with OS X 10.7's arrowless scrollbar

Categories

(Core :: Widget: Cocoa, defect)

Product:
Core
Component:
Widget: Cocoa
Platform:
All
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Tracking Flags:
Tracking Status
firefox5 + fixed
firefox6 --- fixed
firefox7 --- fixed

People

(Reporter: smichaud, Assigned: smichaud)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Fix
10.92 KB, patch
mstange
: review+
christian
: approval-mozilla-beta+
Details | Diff | Splinter Review 
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.
Blocks: lion-compatibility
Summary: Ensure compatibility with OS X 10.7's arrowless scrollbar → [10.7] Ensure compatibility with OS X 10.7's arrowless scrollbar
Attached patch Fix — — Splinter Review 
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.
Assignee: nobody → smichaud
Status: NEW → ASSIGNED
Attachment #532326 - Flags: review?(joshmoz)
(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.
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.
(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 on attachment 532326 [details] [diff] [review]
Fix

Markus, you've done a lot of work in this area.  Can you review this patch?
Attachment #532326 - Flags: review?(joshmoz) → review?(mstange)
Comment on attachment 532326 [details] [diff] [review]
Fix

Sure, looks good.
Attachment #532326 - Flags: review?(mstange) → review+
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
this bug number wasn't noted in the checkin comment.
> 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.
Blocks: 636564
Steven - can you make sure this patch works on aurora and beta and post new patches if necessary?
Actually, we're all set for Aurora already. Just need a beta patch.
It was my understanding this was only going to be in Firefox 6 -- not in Firefox 5.
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).
status-firefox5: --- → affected
status-firefox6: --- → fixed
status-firefox7: --- → fixed
tracking-firefox5: --- → ?
OK, I'll write up a patch and seek approval.
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?
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.
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 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.
Attachment #532326 - Flags: approval-mozilla-beta?
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 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.
Attachment #532326 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.
Thanks!
Does this happen on 3.6?
Yes, it happens in all older versions -- 4, 3.6 and 3.5.
Verified fixed on the Beta 6 candidate - buildID=20110613165758.
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
Status: RESOLVED → VERIFIED
(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.)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: