714005 - [10.7] Text field part of the location bar covers the inner shadow, and other drawing problems

Status: RESOLVED INCOMPLETE

(Camino Graveyard :: Location Bar & Autocomplete, defect)

Description

[philippe (part-time)]

Attachment: screenshot (camino)

The Lion location bar has an inner shadow; Camino's textfield part, starting at the http:// has a non-transparent white background that covers the inner shadow. The inner shadow is still visible behind the favicon (that compartment is apparently transparent).

For https:// url's the inner is not visible at all.

Comment 1

[philippe (part-time)]

Attachment: screenshot (Safari)

Comment 2

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

Is this due to http://mxr.mozilla.org/camino/source/camino/src/browser/AutoCompleteTextField.mm#543 or something we're doing to the field editor, or something else entirely?

I can't tell what controls the background color-drawing of the PageProxyIcon view such that it behaves properly here, since it changes color, too, but is excluded from AutoCompleteTextField's drawing by the |drawingRectForBounds:| math?  (The LocationBarPartitionView, by contrast, asks for its superview's bg color in |drawRect:|.)

Comment 3

[Samuel Sidler (:ss)]

Wow, and once you see it, you notice that the far, bottom right part of the location bar gets cut off slightly because of this instead of being a smooth round-rect. (And now I just noticed that secure sites don't have the beautiful shadow *and* aren't rounded at all but are instead quite square. hmph.)

Comment 4

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

So after Sam's follow-up comment, it sounds like the location bar has at least 3 drawing problems on 10.7:

1) A clipping issue on the right side, where the drawn-in rect overlaps/extends beyond certain parts of the overall rounded-rect location bar (only for normal sites, or does it happen with the PartitionVeiw--feed icon and/or lock--showing, too?),

2) A transparency issue with normal sites, where we end up drawing a white rectangle in the middle of the rounded-rect's interior shadow

3) A transparency issue with SSL sites, where our yellowish color entirely replaces the interior of the rounded-rect, so there is no shadow(?)

10.7 folks, does that sound right?  If not, can we get some screenshots of the whole bar, or at least left and right sides, in three states (normal, feed, secure)?

(I know the perfect Cocoa dev for this kind of bug ;-) but, alas, I believe he no longer has a tree :-( )

Comment 5

[philippe (part-time)]

Attachment: screenshot 3 states of the location bar

(In reply to Smokey Ardisson (back-ish; no bugmail - do not email) from comment #4)

yes to 1 and 2

> 3) A transparency issue with SSL sites, where our yellowish color entirely
> replaces the interior of the rounded-rect, so there is no shadow(?)

The whole widget is replaced actually. It looks less 'bad', kinda. From the look of it, it is constructed (in code ?) to match the 10.6 and older location bar widget, but with the added yellow background. It has no rounded corners, and a small old style inset shadow at the top.

Screenshot with 2.1 release on a default profile

Comment 6

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

(In reply to philippe from comment #5)
> yes to 1 and 2

From the screenshot, it looks like LBPV has a larger height than the interior of the new rounded-rect, so that a bit of LBPV's bottom overlaps into the rest of the toolbar (in addition to making the top right rounded-rect corner look strange).

> > 3) A transparency issue with SSL sites, where our yellowish color entirely
> > replaces the interior of the rounded-rect, so there is no shadow(?)
> 
> The whole widget is replaced actually. It looks less 'bad', kinda. From the
> look of it, it is constructed (in code ?) to match the 10.6 and older
> location bar widget, but with the added yellow background. It has no rounded
> corners, and a small old style inset shadow at the top.

Since the rounded-rect widget has a smaller height than the pre-10.7 one, I suspect our background-color drawing is using the full rect of the older one and is drawing "on top" of the rounded-rect one.  That, or Apple decided if someone requested a background color, to fall back to drawing the older widget.  Dunno.

New question, though: since the rounded-rect version has a smaller height, are the text and site icon properly centered (and not clipped)?


> 
> Screenshot with 2.1 release on a default profile

Comment 7

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

Oh, bleh; I bet it's not smaller, but just positioned at a different place. On closer inspection, it looks like there's an empty space above the LBPV between it and the top interior edge of the rounded-rect, so we're just drawing LBPV 1-2px too low on 10.7.

(My kingdom for a Pixie for these rapidly-ageing eyes!)

The question about alignment still stands, though.

Comment 8

[philippe (part-time)]

(In reply to Smokey Ardisson (back-ish; no bugmail - do not email) from comment #6)

> New question, though: since the rounded-rect version has a smaller height,
> are the text and site icon properly centered (and not clipped)?

Yes, text and favicon are positioned correctly, matching Safari pixel for pixel. Feeds and Security lock icons are also positioned correctly.

Comment 9

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

Flagging this for 2.1.1 so it doesn't get lost, though I don't expect we'll have a fix for it by 2.1.1 :-(

Comment 10

[Wevah (Nate Weaver)]

URL field is 1px too short on 10.7.

Comment 11

[Dan Weber]

Attachment: Lion toolbar fixes

This annoyed me so much, I had to do something about it. Most of these issues are easy to fix, but Lion will revert to the old-style text field if setBackgroundColor is called. What do we think about just not setting the background to yellow in Lion or higher?

Regarding smokey's Comment 4:

> 1) A clipping issue on the right side, where the drawn-in rect
> overlaps/extends beyond certain parts of the overall rounded-rect
> location bar (only for normal sites, or does it happen with the
> PartitionVeiw--feed icon and/or lock--showing, too?),

This can be fixed by creating a clipping path that has two rounded corners on the top right and bottom right.

> 2) A transparency issue with normal sites, where we end up drawing
> a white rectangle in the middle of the rounded-rect's interior shadow

This took me a few days to track down, but it can easily be fixed by adding the following code to the AutoCompleteTextCell subclass:

- (void)setDrawsBackground:(BOOL)flag {
  [super setDrawsBackground:NO];
}

> 3) A transparency issue with SSL sites, where our yellowish color
> entirely replaces the interior of the rounded-rect, so there is no
> shadow(?) (In reply to Wevah from comment #10)

Not really fixable in any easy way because the text field gets converted to the pre-Lion style when setBackgroundColor is called. I've commented out the line for now.

> URL field is 1px too short on 10.7.

This can be addressed by adding a bit of code that makes the text field (or superview, rather) 1 pixel higher when Lion is detected.

Comment 12

[Stuart Morgan]

> What do we think about just not setting the background to yellow in Lion or higher?

We've talked about not doing yellow at all any more, as the browser consensus has moved away from it, but it's not realistic to do that change (even just for Lion) in a 2.1.x release.

We could revisit for post-2.1, but at this point we don't actually know if there will be a post-2.1, so having that discussion now is probably not useful.

Comment 13

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

(In reply to Dan Weber from comment #11)

> This annoyed me so much, I had to do something about it.

:D

Thanks, Dan!  I had given up on poking more accessible people on 10.7 with mad Cocoa skillz who would have been able to fix it.

> Most of these
> issues are easy to fix, but Lion will revert to the old-style text field if
> setBackgroundColor is called. What do we think about just not setting the
> background to yellow in Lion or higher?

I don't think we should make changes to the yellow color "arbitrarily"; it's our longest-standing consistent piece of security UI (since Nov 2004), and changing (dropping) it in a security update, just on one OS version, seems much worse than dropping it entirely before a major release, which we agreed not to do (see bug 402650; the agreement was to reconsider what to do with it when we implement EV).

I know that's not the answer you wanted to hear.

Are the non-SSL-site issues all still fixable even if we keep the yellow background for SSL sites?  (I.e., does switching from yellow back to white/clear cause additional problems that the rest of the current patch doesn't address?)

As philippe notes, when AppKit falls back to the old style, the result doesn't look "bad" like all the mis-matches on non-SSL sites do (and conceivably we could add a clipping rect to get back the rounded corners in that case, even if we couldn't restore the shadow, right?), it just looks "different" switching between two styles.  IMO, falling back to that look on SSL sites is better than no yellow background on SSL sites.

I guess I want to know what the other options are, and their feasibility and drawbacks.

Thanks again for digging into this, Dan; I'd like to get a good fix in the nightlies as soon as we can, since I know the current situation on 10.7 is ugly as sin.

Comment 14

[philippe (part-time)]

(In reply to Dan Weber from comment #11)
> Created attachment 604428 [details] [diff] [review]
> Lion toolbar fixes

:D + :D 
:-)

Look great !

One minor little thing I noticed here though: while loading a long long url for a page that has feeds and is ssl: while loading the url briefly overlaps the feed/lock icon compartment. As soon as the page is loaded, that overlap is gone. The url must be longer than the length of the location bar.

sample url: https://bugzilla.mozilla.org/buglist.cgi?query_format=advanced;product=Camino;bugidtype=include;chfieldfrom=-1d;chfieldto=Now;chfield=[Bug%20creation];order=Bug%20Number;list_id=2567070

> What do we think about just not setting the
> background to yellow in Lion or higher?

I'm personally not religious about that indicator anymore, but I agree with Smokey and Stuart. As I noted, the pre-Lion style doesn't look bad and there are no weird display issues. It is just a bit inconsistent.

Comment 15

[philippe (part-time)]

(In reply to philippe from comment #14)

> One minor little thing I noticed here though: while loading a long long url
> for a page that has feeds and is ssl: while loading the url briefly overlaps
> the feed/lock icon compartment. As soon as the page is loaded, that overlap
> is gone. The url must be longer than the length of the location bar.

This appears to be an artifact of building with the 10.5sdk (build on 10.6, running on 10.7).
Build with the same sdk, Console logs the following (for every new window, I think):
3/10/12 10:58:08.268 PM Camino: <ExtendedSplitView: 0xc0cfbe0>: the delegate <BrowserWindowController: 0x16c198f0> was sent -splitView:resizeSubviewsWithOldSize: and left the subview frames in an inconsistent state:
3/10/12 10:58:08.268 PM Camino: Split view bounds: {{0, 0}, {350, 23}}
3/10/12 10:58:08.269 PM Camino:     Subview frame: {{0, 0}, {214, 22}}
3/10/12 10:58:08.269 PM Camino:     Subview frame: {{223, 0}, {127, 22}}

Comment 16

[Dan Weber]

Attachment: Lion toolbar fixes v2

In this patch I kept the secure background color by overriding the cell's drawInteriorWithFrame. It's not perfect because it draws a round rect over the text field's interior shadow, but in practice it looks OK. This might be our best bet--better than reverting to the old style, I think.

The patch might be a bit rough right now but I'm willing to see it to the end.

Comment 17

[Eiichi]

Attachment: Screenshots

(In reply to Dan Weber from comment #16)
> Created attachment 604747 [details] [diff] [review]
> Lion toolbar fixes v2

Fantastic! Thanks, Dan.
Screenshots of my private build with Dan's patch (opened with 'NSUseLeopardWindowValues NO')

Comment 18

[philippe (part-time)]

Attachment: screenshot(s) default toolbar

Me likes :-)
The SSL location bar looks nice.

Comment 19

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

So this is very bizarre: it looks like the isLionOrHigher checks are being completely ignored here on 10.5 !?

See the screenshot pairs at http://www.ardisson.org/smokey/moz/locbar/ (all are from Pixie, except for the whole-toolbar shots).

Note 1) the location bar appears to be 1px taller or the "contents" are shifted up 1px (see the 1 extra px of white at the bottom of the bar, and note the top focus ring appears clipped) and 2) there are rounded corners on the left side of the yellow SSL and the right side of the PartitionView.

I noticed 1) right away, with the naked eye, but 2 was only noticeable with Pixie.

I suppose 1) could be an artifact of switching to drawInteriorWithFrame:, but I don't understand 2) at all.

I don't understand the drawing code to begin with ;) so I can't say more on that, but 

>+// Returns YES if the secure images has been displayed, NO otherwise.
>+- (BOOL)isSecureImageDisplayed;

"images have" or "image has" ;-)

The 10.7 screenshots do look really good to the naked eye :-)

Comment 20

[philippe (part-time)]

(In reply to Smokey Ardisson (not following bugs - do not email) from comment #19)
> So this is very bizarre: it looks like the isLionOrHigher checks are being
> completely ignored here on 10.5 !?

I see the same issues on 10.6 :-(.

Comment 21

[Stuart Morgan]

Oh crap, I wonder if I misremembered the issue with MAC_OS_X_VERSION_10_6 on older SDKs. It may be that it was undefined but silently treating undefined as 0, which makes the condition always true.

Try putting gibberish that doesn't compile in one branch of the #if/#else and see which is actually being compiled for you.

If it is being treated as 0, then we need to just take out that short-circuit for Lion-targeted builds.

Comment 22

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

(In reply to Stuart Morgan from comment #21)
> Oh crap, I wonder if I misremembered the issue with MAC_OS_X_VERSION_10_6 on
> older SDKs. It may be that it was undefined but silently treating undefined
> as 0, which makes the condition always true.
> 
> Try putting gibberish that doesn't compile in one branch of the #if/#else
> and see which is actually being compiled for you.
> 
> If it is being treated as 0, then we need to just take out that
> short-circuit for Lion-targeted builds.

Yup, that's definitely the problem.  I guess the color change was subtle enough that we didn't notice it after bug 717476.

However, with that fixed, I notice one new problem with Dan's patch: the "url-fade" gradient doesn't look like it's being composited correctly on the yellow color:

http://www.ardisson.org/smokey/moz/locbar/gradient-orig.png
http://www.ardisson.org/smokey/moz/locbar/gradient-patch.png

It's much more visible in real life, but easier to screenshot in Pixie :P

This just started when I fixed the NSWorkspace method; it's not visible in the partiton-* screenshots from last night.

I'm guessing it's caused by switching what's doing the drawing and not changing 

> NSColor *currentBackgroundColor = [(NSTextField *)[self superview] backgroundColor];

in LBPV, but I don't know for sure.  Nor know why it's not equally wrong on 10.7, although from philippe's screenshot, it looks like there's *no* "url-fade" gradient at all on 10.7.

Comment 23

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

(In reply to comment #22)
> I'm guessing it's caused by switching what's doing the drawing and not
> changing 
> 
> > NSColor *currentBackgroundColor = [(NSTextField *)[self superview] backgroundColor];
> 
> in LBPV, but I don't know for sure.

Yeah, currentBackgroundColor is always 1.0, 1.0, 1.0.

Comment 24

[philippe (part-time)]

(In reply to Smokey Ardisson (not following bugs - do not email) from comment #22)
>  Nor know why it's not equally wrong on
> 10.7, although from philippe's screenshot, it looks like there's *no*
> "url-fade" gradient at all on 10.7.

There is indeed no gradient at all (I had to use Pixie to make sure, may eyes have trouble discerning it.)

Also, right when I loaded this bug, the yellow background was not painted, except for that small part where the gradient should appear. I couldn't reproduce that, though.

Comment 25

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

(In reply to philippe from comment #24)
> Also, right when I loaded this bug, the yellow background was not painted,
> except for that small part where the gradient should appear. I couldn't
> reproduce that, though.

I've also seem some random drawing "glitches" (once, the url drew over the LBPV for a few seconds), but they've always corrected themselves, and I've also never been able to reproduce any of them.

Comment 26

[philippe (part-time)]

(In reply to philippe from comment #24)

> Also, right when I loaded this bug, the yellow background was not painted,
> except for that small part where the gradient should appear. I couldn't
> reproduce that, though.

I think I have a way to reproduce this consistently: click the link to a https:// page from an external app (in my case: Mail.app).

Comment 27

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

I filed bug 735831 on removing the broken short-circuit, since other features depending on it aren't behaving as desired right now; we can and should fix that check separately and first.

Comment 28

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

Dan, are you working on the fade-gradient and any reproducible drawing glitches?  Now that I've fixed isLionOrHigher, the rest of the behavior seems to be where we want it, so if those two items get fixed, then behavior-wise I'd say the patch is ready for review.

Comment 29

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

(In reply to philippe from comment #26)
> (In reply to philippe from comment #24)
> 
> > Also, right when I loaded this bug, the yellow background was not painted,
> > except for that small part where the gradient should appear. I couldn't
> > reproduce that, though.
> 
> I think I have a way to reproduce this consistently: click the link to a
> https:// page from an external app (in my case: Mail.app).

Yeah, I also see the "text drawn over LBPV" bug when getting a URL via AppleEvent (so long as the URL is not already loaded, of couse):

tell app "Camino" to open location "long-bugzilla-url"

I was also able to reproduce the "the yellow background was not painted, except for that small part where the gradient should appear" thing philippe reported, which I had never seen before, using this method (though the yellow was also drawn around the site icon in my case, just not any of the rest of the "text" part of the field).

Comment 30

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

Attachment: Dan's "Lion toolbar fixes v2" patch, unbitrotted

This Dan's attachment 604747 [details] [diff] [review] "Lion toolbar fixes v2" patch unbitrotted from the MPL2 upgrade; I had it in my tree when upgrading (Hg merging abilities are pretty good, apparently!) and had to unbitrot it to get it *out* of my tree after the upgrade, so I'm attaching it here for the benefit of anyone who needs it.

Comment 31

[Dan Weber]

(In reply to Smokey Ardisson (not following bugs - do not email) from comment #28)
> Dan, are you working on the fade-gradient and any reproducible drawing
> glitches?  Now that I've fixed isLionOrHigher, the rest of the behavior
> seems to be where we want it, so if those two items get fixed, then
> behavior-wise I'd say the patch is ready for review.

I'll be looking into these issues this week. The fade gradient is easy to fix. The other issue (about redrawing the yellow background) I haven't had a chance to reproduce yet, but I'll look into what philippe said in Comment 26.

Comment 32

[philippe (part-time)]

(In reply to Dan Weber from comment #31)
> The other issue (about redrawing the yellow background) I haven't had a
> chance to reproduce yet, but I'll look into what philippe said in Comment 26.

Another way to repro the issue on my side at least: Cmd-Opt F and search with Google. I haven't seen it on 10.6, I think, but on 10.7 the painting issue happens regularly (but I haven't figured out a pattern).

Comment 33

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

Dan, any updates here?