Emoticons are clipped after checkin for Bug 458487

VERIFIED FIXED in mozilla1.9.1b2

Status

()

Core
Layout: Images
VERIFIED FIXED
10 years ago
10 years ago

People

(Reporter: Tobias Fischer, Assigned: roc)

Tracking

({regression, verified1.9.1})

Trunk
mozilla1.9.1b2
regression, verified1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

10 years ago
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.

Updated

10 years ago
Blocks: 458487
No longer depends on: 458487

Comment 1

10 years ago
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)
I need a testcase for this bug. I can't find these exact icons in our repository.
It's possible that the fix for bug 456330 will fix this, though.
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

10 years ago
(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

10 years ago
(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

10 years ago
(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
(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

10 years ago
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

10 years ago
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

10 years ago
> 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

10 years ago
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. ;)
Created attachment 346561 [details]
NOT a testcase

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

Comment 14

10 years ago
Created attachment 346576 [details]
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.
It looks fine on Mac at normal zoom level. At some zoom levels it does look clipped. I'll dig into that, thanks.
Created attachment 346590 [details]
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!
Assignee: nobody → roc

Comment 17

10 years ago
(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.
Created attachment 346605 [details] [diff] [review]
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.
Attachment #346605 - Flags: superreview?(dbaron)
Attachment #346605 - Flags: review?(dbaron)
This should block, since it's a significant regression.
Flags: blocking1.9.1?
Whiteboard: [needs review]
Comment on attachment 346605 [details] [diff] [review]
fix

r+sr=dbaron
Attachment #346605 - Flags: superreview?(dbaron)
Attachment #346605 - Flags: superreview+
Attachment #346605 - Flags: review?(dbaron)
Attachment #346605 - Flags: review+
Created attachment 346636 [details] [diff] [review]
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.
Attachment #346605 - Attachment is obsolete: true
Attachment #346636 - Flags: superreview+
Attachment #346636 - Flags: review+
Whiteboard: [needs review] → [needs landing]

Comment 22

10 years ago
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

10 years ago
The patch includes reftests.

Comment 24

10 years ago
Oh, right, the reftest is there. good. :)
Pushed 8adf110592e9
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs landing]
Hardware: PC → All
Target Milestone: --- → mozilla1.9.1b2
(Reporter)

Comment 26

10 years ago
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.
Status: RESOLVED → VERIFIED
Depends on: 463301
Flags: blocking1.9.1? → blocking1.9.1+
Keywords: fixed1.9.1

Updated

10 years ago
Keywords: verified1.9.1
Keywords: fixed1.9.1
You need to log in before you can comment on or make changes to this bug.