673349 - Address bug 656990 for 2.1

Status: RESOLVED FIXED

(Camino Graveyard :: HTML Form Controls, defect)

Description

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Incomplete fix

Unfortunately, all of the hunks fail to apply.  Most of them are trivial changes (attached here), but the meat of the patch (nsNativeThemeCocoa.mm and nsLookAndFeel.mm) are completely different, in part due to bug 517412.

I don't know if it's (10.4-)safe to swap wholesale to the newer methods or not; that would certainly make it easier if so. (However, I think this degree of change probably rules out ever getting a branch-safe patch with a NS_ILOOKANDFEEL_MOZILLA_1_9_2_BRANCH accepted, which means we're likely stuck minibranching this for releases only, or adding support to madhatter to apply patches at the start of a build and getting line-numbers all out-of-wack in symbols--we'd probably want to do that only for nightlies if we did, and still minibranch releases.)

So here's the trivial parts, but someone with more knowledge and time than me is going to have to look at the native theme/look-and-feel hunks of https://bugzilla.mozilla.org/attachment.cgi?id=532326&action=diff#a/widget/src/cocoa/nsLookAndFeel.mm_sec1  and see what can/should be done.

Comment 1

[Stuart Morgan]

Since it's non-trivial, let's just ship b1 with this as a known issue, and fix it for 2.1. We need to start the clock on 2.1 final.

Comment 2

[Stuart Morgan]

Actually, belay that. I'll look tomorrow morning and see if I can throw something together.

Comment 3

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #2)
> Actually, belay that. I'll look tomorrow morning and see if I can throw
> something together.

OK. (I may have made the problems seem more non-trivial than they might actually be; I just don't have the knowledge to accurately SWAG it.)

Comment 4

[Stuart Morgan]

Attachment: fix part 2

Here's the rest; codewise it's actually still a trivial change, because all the code that's different between 1.9.2 and 2 is just being indented (screwing up context, and making it look much worse than it really is).

I haven't built this, and I can't test it. Can you make a build and give it to ss to try out?

If it works (i.e., there's no other, earlier scrollbar work this relies on), we should see if they'll take it for realz.

Comment 5

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #4)
> I haven't built this, and I can't test it. Can you make a build and give it
> to ss to try out?

Er, for anyone who wasn't in the channel on Friday afternoon: http://www.ardisson.org/smokey/moz/Camino-2.1a2pre-i386-2011-07-22.dmg

> If it works (i.e., there's no other, earlier scrollbar work this relies on),
> we should see if they'll take it for realz.

We'd still have to do the MOZILLA_1_9_2_BRANCH-interface dance, though, since this changed the interface on a stable branch; not sure how hard that is for this patch.

Comment 6

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Complete patch for Camino minibranch

Just so I don't forget (and so it's easy to find in case of any unforeseen events), here's the complete patch to replace the two parts.

Still waiting to hear from someone on 10.7 about how/if it works.

Comment 7

[Samuel Sidler (:ss)]

Comment on attachment 548108 [details] [diff] [review]
Complete patch for Camino minibranch

Oh, is that what that dmg was that you linked to? I downloaded it but didn't know why. ;)

Anyway, this doesn't change the behavior of scrollbars on 10.7 at all. The end cap still has issues drawing. Specifically:

  * If scrolled all the way to the top, the end cap is present (starting 
    position anyway).
  * Scroll down on a page and the end cap stays present at the top.
  * Scroll all the way to the bottom and the bottom end cap does not appear.
  * De-focus Camino and the end caps are drawn properly and stay that way until 
    the next scroll.

It's not clear to me from bug 656990 what was trying to fix or that it applies to Camino. It implies there are blank spaces where the end caps should be, but that isn't true on Camino (after a defocus/refocus anyway). I imagine that this is what smichaud is trying to describe, but I can't be sure. Regardless, this patch doesn't change the behavior at all for Camino (at least in the build I have).

Comment 8

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Oh, crap; I don't see the patch in my tree, so it could well be that when I backed it out before updating Gecko, I didn't actually non--dry-run the patch before building :-(  I'll rebuild with the patch.

By way of comparison, does Firefox 5 appear to draw the scrollbars more sanely?

Comment 9

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Sam, try http://www.ardisson.org/smokey/moz/Camino-2.1a2pre-i386-2011-07-25.dmg

/me crosses Finders

Comment 10

[Samuel Sidler (:ss)]

Comment on attachment 548108 [details] [diff] [review]
Complete patch for Camino minibranch

Yep. That build indeed fixes the problem.

Comment 11

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Assuming I didn't screw up minibranching and tagging, this is 2.1b1rc1 (say that 5 times fast!).

Resummarizing as a placeholder for 2.1 until we figure out if we can make a patch that'll be branch-acceptable for the Firefox people.

Comment 12

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/d0c0f3cb8ba6 (on CAMINO_2_1_SECBRANCH)

Nothing builds that branch yet, but hopefully tomorrow.  See bug 740620 comment 13 for more.

-->Stuart, since he backported the important parts ;-)

Comment 13

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Tonight's nightly should include this fix.