Closed Bug 477732 Opened 14 years ago Closed 14 years ago

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

Categories

(Core :: Web Painting, defect, P2)

All
macOS
defect

Tracking

()

VERIFIED FIXED
mozilla1.9.2a1

People

(Reporter: marcia, Assigned: zwol)

References

Details

(Keywords: regression, verified1.9.1, Whiteboard: [fixed1.9.1b3])

Attachments

(9 files, 1 obsolete file)

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
Summary: Error Console tabs are not laying out correctly → Error Console tabs are not laying out correctly on PPC mac
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.
Next time I can get on that PPC machine I will check. Right now mochitests are running on it.
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
Blocks: 455364
Hardware: PowerPC → All
No longer blocks: 455364
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.
(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.)
(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.
Assignee: nobody → zweinberg
Status: NEW → ASSIGNED
Summary: Error Console tabs are not laying out correctly on PPC mac → Error Console tabs are not laying out correctly on Mac, border image endcaps are squashed
Blocks: 455364
Component: Error Console → Layout: View Rendering
Product: Toolkit → Core
QA Contact: error.console → layout.view-rendering
Flags: in-testsuite?
Keywords: regression
This patch fixes the same issue in the preferences toolbar, too.
Attachment #362582 - Flags: review?(dao)
Attachment #362582 - Flags: review?(dao) → review+
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.
Why? The CSS fix is definitely the right thing to do even if the underlying bug is fixed.
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.
I don't. We can get a testcase here and make this bug block. Why do we need to annoy users with it?
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.
Duplicate of this bug: 478704
Summary: Error Console tabs are not laying out correctly on Mac, border image endcaps are squashed → Toolbar buttons (in Error Console, Add-ons Manager, Page Info Dialog) are not laying out correctly on Mac, border image endcaps are squashed
Should the CSS fix be in Firefox:Theme?
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
Attached patch renderer fix (obsolete) — Splinter Review
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.
Attachment #362762 - Flags: review?(roc)
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*.
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.
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.)
> 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.
Attachment #362762 - Attachment is obsolete: true
Attachment #363137 - Flags: superreview+
Attachment #363137 - Flags: review+
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.
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.
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.
Attached file testcase 1
Attached file testcase 2
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.
The reftests pass for me on Mac.
Keywords: checkin-needed
Whiteboard: [needs landing]
http://hg.mozilla.org/mozilla-central/rev/e8b358051c45
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing] → [needs 191 landing]
Target Milestone: --- → mozilla1.9.2a1
Attached image updated screenshot
Reopening because this still doesn't seem quite right. The buttons now include small vertical lines.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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.
Status: REOPENED → RESOLVED
Closed: 14 years ago14 years ago
Resolution: --- → FIXED
Flags: in-testsuite? → in-testsuite+
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.
Status: RESOLVED → VERIFIED
Keywords: checkin-needed
Whiteboard: [needs 191 landing] → [needs 191 landing - only after bug 455634]
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
Attachment #363137 - Attachment description: revised renderer fix: not sensitive to bug 479156 anymore → revised renderer fix: not sensitive to bug 479156 anymore [Checkin: Comment 30 & 35]
Comment on attachment 362582 [details] [diff] [review]
css fix
[Checkin: Comment 33 & 36]


http://hg.mozilla.org/releases/mozilla-1.9.1/rev/db41a8845772
Attachment #362582 - Attachment description: css fix → css fix [Checkin: Comment 33 & 36]
Keywords: fixed1.9.1
Whiteboard: [needs 191 landing - only after bug 455634] → [fixed1.9.1b3]
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.
Component: Layout: View Rendering → Layout: Web Painting
You need to log in before you can comment on or make changes to this bug.