463204 - Emoticons are clipped after checkin for Bug 458487

Status: VERIFIED FIXED

(Core :: Layout: Images, Video, and HTML Frames, defect)

Description

[Tobias Fischer]

As mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=458487#c45 the graphical Emoticons are clipped. This is an regression from the latest Patch for Bug 458487 changeset http://hg.mozilla.org/mozilla-central/rev/49dd935efff3

Original Reporter (see Bug 458487 Comment 45) has tested this with an backout of this changeset.

Comment 1

[John]

Also, if you look at Bug 456330 and its testcase, the background image is completely missing (it was distorted before, but now it doesn't even render)

Comment 2

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

I need a testcase for this bug. I can't find these exact icons in our repository.

Comment 3

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

It's possible that the fix for bug 456330 will fix this, though.

Comment 4

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

I found the icons here:
http://mxr.mozilla.org/comm-central/source/suite/themes/classic/communicator/icons/smileys/
But they display OK for me, at least standing alone. So I really do need a testcase.

Comment 5

[Hartmut Figge]

(In reply to comment #4)

> I found the icons here:
> http://mxr.mozilla.org/comm-central/source/suite/themes/classic/communicator/icons/smileys/

The icons in the picure 'clipped emoticons' can be found in suite/themes/modern/communicator/icons/smileys/

> But they display OK for me, at least standing alone. So I really do need a
> testcase.

You could write a PM to yourself, plain text, with some smileys and look at the mail with a nightly of SM2, choosing modern. E.g.

testline ;) :) ;) :)

Comment 6

[Hartmut Figge]

(In reply to comment #3)

> It's possible that the fix for bug 456330 will fix this, though.

I'm just building a new SM with this fix. We will see. ;)

Comment 7

[Hartmut Figge]

(In reply to comment #3)

> It's possible that the fix for bug 456330 will fix this, though.

No, even with this patch the smileys are still clipped

Comment 8

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

(In reply to comment #5)
> > But they display OK for me, at least standing alone. So I really do need a
> > testcase.
> 
> You could write a PM to yourself, plain text, with some smileys and look at the
> mail with a nightly of SM2, choosing modern. E.g.
> 
> testline ;) :) ;) :)

I could, but I'd need to pull and build Seamonkey from scratch, which would take a while, and we'll need a small testcase for debugging and making reftests anyway ... so it would be helpful if you could extract an HTML test that shows the problem.

Comment 9

[Hartmut Figge]

I don't know, how, because the mentionend icons are rendered perfectly in the browser. No clipping can be seen there. ;)

By the way, not only modern is affected in SM2 but also classic.

Comment 10

[Stefan [:stefanh]]

You'll just need one smiley, then the css from
suite/themes/modern/communicator/smileys.css or suite/themes/classic/communicator/smileys.css should be a pointer. What we do here is that we display the smiley as a background-image in a <span/>.

Comment 11

[Stefan [:stefanh]]

> By the way, not only modern is affected in SM2 but also classic.

It's the same icons (and css) in both themes.

Comment 12

[Hartmut Figge]

But i don't speak HTML or CSS, so it would surely be better, if someone else creates this testcase. Otherwise it would take some time. ;)

Comment 13

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

Attachment: NOT a testcase

This simple test works for me. So we need more details about what's in the generated HTML markup.

Comment 14

[Hartmut Figge]

Attachment: perhaps a testcase

The 'NOT a testcase' leads to the one attached. *g* It shows the error in a SM2 with 'fix v4', but not in one, in wich this fix is backed out.

The additional lines come from smileys.css.

Comment 15

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

It looks fine on Mac at normal zoom level. At some zoom levels it does look clipped. I'll dig into that, thanks.

Comment 16

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

Attachment: a testcase

This testcase clearly shows a bug on Mac. Note that the image is 19x19 so the CSS should be setting width:19px NOT width:18px!

Comment 17

[Hartmut Figge]

(In reply to comment #16)

> Created an attachment (id=346590) [details]
> a testcase

I can confirm, that a SM2 with 'fix v4' shows the error and one without does not.

Comment 18

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

Attachment: fix

I had to tighten the requirements on the anchor point in
https://wiki.mozilla.org/Gecko:Image_Snapping_and_Rendering

We need to ensure that when the anchor point is snapped, it moves in the same direction as when we snapped the fill rect. I'm doing this by first transforming it the same way we transformed the fill rect by snapping.

Comment 19

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

This should block, since it's a significant regression.

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 346605 [details] [diff] [review]
fix

r+sr=dbaron

Comment 21

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

Attachment: fix v2

That version of the patch actually had a bug in the anchorPoint.y calculation, we were scaling by the widths when we should have been scaling by heights. I found this because we failed the 433640-1.html reftest.

But it turns out that I had to change that test for us to pass it with this patch. In particular, drawing a centered image in an element of width 32.5px, with this patch we snap the fill-rect to 33px and anchor the center of the image to a snapped offset of 17px from the left/top. In other words, when the element grows from 32px to 32.5px we add a row of pixels to the left/top, not the right/bottom as the current code does. This seems reasonable so I've updated the test in this patch.

Comment 22

[Robert Kaiser]

Do we have reftests that would catch such a type of failure in the future? Should the testcase in the bug here be turned into one?

Comment 23

[Stefan [:stefanh]]

The patch includes reftests.

Comment 24

[Robert Kaiser]

Oh, right, the reftest is there. good. :)

Comment 25

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

Pushed 8adf110592e9

Comment 26

[Tobias Fischer]

Marking this as verified=fixed as I am something like an inofficial (hobby?) SeaMonkey QA. 
Have tested this on Windows with:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b2pre) Gecko/20081106 Mnenhy/0.7.5.20005 SeaMonkey/2.0a2pre 
and on Mac too with the latest Tinderbox-Build. 

Thanks to roc for help working out the testcase and fixing this.