477732 - Toolbar buttons (in Error Console, Add-ons Manager, Page Info Dialog) are not laying out correctly on Mac, border image endcaps are squashed

Status: VERIFIED FIXED

(Core :: Web Painting, defect, P2)

Description

[Marcia Knous [:marcia]]

Attachment: Screenshot of issue on PPC mac

Seen while running  Mozilla/5.0 (Macintosh; U; PPC Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090207 Minefield/3.2a1pre

STR:
1. Open the Error Console
2. Observe the attached screenshot

Comment 1

[philippe (part-time)]

Isn't that fixed in more recent builds ? I saw that issue on a Intel/OS X10.5.6 Minefield build, but it is gone now (with build 20090209). I strongly suspect that this was caused by a checkin in bug 455364 (border-image), since then backed out.

Comment 2

[Marcia Knous [:marcia]]

Next time I can get on that PPC machine I will check. Right now mochitests are running on it.

Comment 3

[philippe (part-time)]

Now that the patch in bug 455364 landed again, those tabs (in error console, in add-on manager) are broken again. The main-window tabs from the Grapple theme are affected as well.

fails
http://hg.mozilla.org/mozilla-central/rev/506199089c3c

works
http://hg.mozilla.org/mozilla-central/rev/8a3394e165a9

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=8a3394e165a9&tochange=506199089c3c

Comment 4

[Zack Weinberg (:zwol)]

The Grapple theme isn't in mozilla-central AFAICT, but I can reproduce the problem with a testcase based on the Pinstripe error console.  It is indeed a bug in the border-image renderer, but it's provoked by a CSS construct in the theme that I consider ill-advised, and it is very easily worked around there.  Quoting from toolkit/themes/pinstripe/global/console/console.css:

  border-width: 0 6px;
  -moz-border-image: url("chrome://global/skin/toolbar/roundrectbutton.png") 
       0 6 repeat !important;

The border-box is carefully set up to be exactly the same height as the image. This is supposed to divide the image into three columns, not scale them at all, and then paint them over the border-box, repeating the middle column (which happens to be one pixel wide) as many times as necessary.  Since the border-box is the same height as the image, it shouldn't matter that the images are being repeated vertically.

The renderer bug is making it squash the left and right strips into 6x6 squares and then repeat those squares vertically to fill up the space.  The workaround is to change all themes with constructs like this to use "repeat stretch" instead of just "repeat".  I recommend that this be done *anyway*, because that insulates you from ugly glitches if the border-box somehow winds up not the same height as the image.

I would prefer to fix the renderer by means of a follow-up patch in this bug, but I won't be working on it until Tuesday because of the holiday in the USA, so if you want to back out the patch in bug 455364 I will cope.

Comment 5

[philippe (part-time)]

(In reply to comment #4)
> The Grapple theme isn't in mozilla-central AFAICT, but I can reproduce the
> problem with a testcase based on the Pinstripe error console.

To be clear, the default theme is affected. Grapple is a little bit worse off, as it uses border-image for it the main-window tabs.

[...]
> The renderer bug is making it squash the left and right strips into 6x6 squares
> and then repeat those squares vertically to fill up the space.  The workaround
> is to change all themes with constructs like this to use "repeat stretch"
> instead of just "repeat".  I recommend that this be done *anyway*, because that
> insulates you from ugly glitches if the border-box somehow winds up not the
> same height as the image.

yep, "repeat stretch" 'fixes' the issue.

(adding markus to the CC, as he has handles those tab images.)

Comment 6

[Zack Weinberg (:zwol)]

(In reply to comment #5)
> (In reply to comment #4)
> > The Grapple theme isn't in mozilla-central AFAICT, but I can reproduce the
> > problem with a testcase based on the Pinstripe error console.
> 
> To be clear, the default theme is affected. Grapple is a little bit worse off,
> as it uses border-image for it the main-window tabs.

I think maybe only the default Mac theme (that's what Pinstripe is, right?) -- I don't actually have a Mac, see.

Assigning to self for the renderer fix.

Comment 7

[Markus Stange [:mstange]]

Attachment: css fix [Checkin: Comment 33 & 36]

This patch fixes the same issue in the preferences toolbar, too.

Comment 8

[Dão Gottwald [:dao]]

Comment on attachment 362582 [details] [diff] [review]
css fix
[Checkin: Comment 33 & 36]

I think this shouldn't land until the underlying bug is fixed.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Why? The CSS fix is definitely the right thing to do even if the underlying bug is fixed.

Comment 10

[Dão Gottwald [:dao]]

Yes, that's why I'd land it even if the underlying bug is fixed. Until then, I consider it a good thing that the rendering bug doesn't only exist theoretically but is exposed by a real case.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't. We can get a testcase here and make this bug block. Why do we need to annoy users with it?

Comment 12

[Dão Gottwald [:dao]]

Users who are annoyed by distorted button borders in the error console probably shouldn't use nightlies. Other than that, yes, somebody could create a testcase and mark this bug blocking.

Comment 14

[Mike Beltzner [:beltzner, not reading bugmail]]

Should the CSS fix be in Firefox:Theme?

Comment 15

[Zack Weinberg (:zwol)]

Attachment: renderer fix

Pretty straightforward - with 'repeat', side images are supposed to be scaled to the width of that side and *proportionately* along that side.  We were scaling them in both directions to the width of that side.  The patch looks bigger than it is because I moved some calculations relevant only to one iteration of the loop into that iteration (since there is already an if-else chain in there).

Lightly tested - border-image reftests only.

Comment 16

[Zack Weinberg (:zwol)]

Attachment: screenshot of side-scaling-1h(-ref).html - shows remaining problem

There's a problem still, unfortunately.  Regardless of repeat/scale, at some zoom levels and box sizes, there are seams in the overall image!  This is just what using the nsLayoutUtils::Draw*Image helper functions was supposed to *prevent*.

Comment 17

[Zack Weinberg (:zwol)]

I shoulda said that the image I just attached was a screenshot of the browser at the default zoom level, but blown up to 200% with no interpolation in the Gimp to make the problem more obvious.

Comment 18

[Zack Weinberg (:zwol)]

The "remaining problem" described in the above two comments is now bug 479156 and need not block the fix in this bug, IMO.  (It appears to be a lower level issue.)

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I pushed the patch:
http://hg.mozilla.org/mozilla-central/rev/a2f0465c9dc3
but had to back it out due to a Mac test failure:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1235032456.1235036209.21331.gz

Comment 20

[Zack Weinberg (:zwol)]

Attachment: revised renderer fix: not sensitive to bug 479156 anymore [Checkin: Comment 30 & 35]

> Mac test failure:
> http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1235032456.1235036209.21331.gz

This is because of bug 479156.  I had some extra styling in the tests to make them detect that bug, but it didn't actually work on Linux and I forgot it was there.  Since we're not treating that bug as a blocker for this one I've taken the extra styling out; the tests should now pass on Mac.

Comment 21

[Zack Weinberg (:zwol)]

I'm reluctant to tag this for checkin again without proof that the fix works.  Would one of the Mac users on this bug be willing to test the patch?  You only need to build with the patch applied and run the border-image reftests.

Comment 22

[Marcia Knous [:marcia]]

Zack: We can make a tryserver build, but would need help with where I could find the border-image reftests - maybe I could get someone on the team to help me with that part.

Comment 23

[Zack Weinberg (:zwol)]

I'm afraid a tryserver build is no use at all in this context, because the relevant reftests are added by the patch.  This needs someone who can build it for themselves, on a mac.

Comment 24

[Dão Gottwald [:dao]]

Attachment: testcase 1

Comment 25

[Dão Gottwald [:dao]]

Attachment: testcase 1 reference

Comment 26

[Dão Gottwald [:dao]]

Attachment: testcase 2

Comment 27

[Dão Gottwald [:dao]]

Attachment: testcase 2 reference

Comment 28

[Zack Weinberg (:zwol)]

Thanks for pulling out the relevant tests, Dão, but eyeball comparison may not be good enough to confirm that the failure mentioned in comment 19 is truly gone.  If testing by hand I recommend taking screen shots and using a graphics editor to compare pixel-by-pixel.  Note also that you may see seaming - unless it only appears in test but not reference or vice versa, that's not this bug, that's bug 479156.

Comment 29

[Markus Stange [:mstange]]

The reftests pass for me on Mac.

Comment 30

[Karl Tomlinson (:karlt)]

http://hg.mozilla.org/mozilla-central/rev/e8b358051c45

Comment 31

[Dave Townsend [:mossop]]

Attachment: updated screenshot

Reopening because this still doesn't seem quite right. The buttons now include small vertical lines.

Comment 32

[philippe (part-time)]

(In reply to comment #31)
> Created an attachment (id=363429) [details]
> updated screenshot
> 
> Reopening because this still doesn't seem quite right. The buttons now include
> small vertical lines.

That is covered by bug 479156.

Comment 33

[Dão Gottwald [:dao]]

Comment on attachment 362582 [details] [diff] [review]
css fix
[Checkin: Comment 33 & 36]

http://hg.mozilla.org/mozilla-central/rev/0337c236d55d

Comment 34

[Marcia Knous [:marcia]]

Verified fixed using  Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090223 Minefield/3.2a1pre. Bug 479156 is on file for the issue with the vertical lines.

Comment 35

[Serge Gautherie (:sgautherie)]

Comment on attachment 363137 [details] [diff] [review]
revised renderer fix: not sensitive to bug 479156 anymore
[Checkin: Comment 30 & 35]


http://hg.mozilla.org/releases/mozilla-1.9.1/rev/8c6bc98c4976

Comment 36

[Serge Gautherie (:sgautherie)]

Comment on attachment 362582 [details] [diff] [review]
css fix
[Checkin: Comment 33 & 36]


http://hg.mozilla.org/releases/mozilla-1.9.1/rev/db41a8845772

Comment 37

[Marcia Knous [:marcia]]

Verified fixed on the 1.9.1 branch using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090225 Shiretoko/3.1b3pre. Bug 479156 is on file for the issue with the vertical lines.