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

VERIFIED FIXED

Status

()

Core
Widget: Cocoa
VERIFIED FIXED
6 years ago
4 years ago

People

(Reporter: smichaud, Assigned: smichaud)

Tracking

(Blocks: 1 bug)

unspecified
All
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox5+ fixed, firefox6 fixed, firefox7 fixed)

Details

Attachments

(1 attachment)

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.
(Assignee)

Updated

6 years ago
Blocks: 636455
Summary: Ensure compatibility with OS X 10.7's arrowless scrollbar → [10.7] Ensure compatibility with OS X 10.7's arrowless scrollbar
(Assignee)

Comment 1

6 years ago
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.
Assignee: nobody → smichaud
Status: NEW → ASSIGNED
Attachment #532326 - Flags: review?(joshmoz)
(Assignee)

Comment 2

6 years ago
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

6 years ago
(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.
(Assignee)

Comment 4

6 years ago
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

6 years ago
(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.
(Assignee)

Comment 6

6 years ago
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+
(Assignee)

Comment 8

6 years ago
Comment on attachment 532326 [details] [diff] [review]
Fix

Landed on trunk:
http://hg.mozilla.org/mozilla-central/rev/890b0e24fa5e
(Assignee)

Updated

6 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Comment 9

6 years ago
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.

Updated

6 years ago
Blocks: 636564

Comment 11

6 years ago
Steven - can you make sure this patch works on aurora and beta and post new patches if necessary?

Comment 12

6 years ago
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.

Comment 14

6 years ago
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.

Comment 16

6 years ago
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.

Comment 18

6 years ago
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?

Comment 20

6 years ago
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

6 years ago
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.

Comment 23

6 years ago
Thanks!

Comment 24

6 years ago
Does this happen on 3.6?

Updated

6 years ago
tracking-firefox5: ? → +
Yes, it happens in all older versions -- 4, 3.6 and 3.5.
Verified fixed on the Beta 6 candidate - buildID=20110613165758.
status-firefox5: affected → fixed
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.