Closed Bug 175787 Opened 22 years ago Closed 18 years ago

Resize large images to fit in treeview instead of cropping

Categories

(Core :: Widget, defect)

Product:
Core
Component:
Widget
Type:
defect
Priority:
Not set
Severity:
trivial

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.9alpha1

People

(Reporter: micke, Assigned: takeshi2)

References

(
URL
)

Details

Attachments

(7 files, 9 obsolete files)

Workaround Extension: Fit Bookmark Icon 0.6 (MD5:08b7d9cfef4d7f222cc8dfe61307c9c8)
5.45 KB, application/x-xpinstall
Details
a new screenshot showing that this bug still appears on Bon Echo's Places functionality
70.26 KB, image/jpeg
Details
v5 patch
15.06 KB, patch
neil
: review+
neil
: superreview+
Details | Diff | Splinter Review
Screenshot of the bug in "all tabs" menu
46.61 KB, image/jpeg
Details
screen shot of the all tabs menu
30.28 KB, image/png
Details
Screenshot from the bug (correct picture)
86.64 KB, image/png
Details
Screenshot showing cropped favicons on bookmarks sidebar on the branch (correct picture)
86.64 KB, image/png
Details 
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.2b) Gecko/20021020 Phoenix/0.3
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.2b) Gecko/20021020 Phoenix/0.3

Favicons are displayed next to the bookmarks in the bookmark sidebar. However,
large favicons, such as the one used at http://www.trolltech.com/ won't be
resized to fit the allocated space, but gets cropped instead.
The favicon does look good in the location field, and on the tabs though!

Reproducible: Always

Steps to Reproduce:
1. Go to http://www.trolltech.com/
2. Make a bookmark to the page.
3. Open the bookmarks sidebar, and observe the upper left quarter of the QT logo.

Actual Results:  
I observed the upper left quarter of the QT logo :-)

Expected Results:  
Display the entire logo.
Comfirmed
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.2b) Gecko/20021020 Phoenix/0.3

Bookmark sidebar does not correctly display icon, BUT bookmark menu does
correctly display icon.
Changing status to Confirmed. (I can reproduce this on Mozilla/5.0 (X11; U;
Linux i686; en-US; rv:1.2b) Gecko/20021019 Phoenix/0.3).
Status: UNCONFIRMED → NEW
Ever confirmed: true
-> hyatt
Assignee: chanial → hyatt
*** Bug 184313 has been marked as a duplicate of this bug. ***
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.4a) Gecko/20030401

I also encounter this bug on Windows XP, however it shows the entire top half of
the icon, not just the top left corner.  I don't see a favicon on Trolltech
right now, but http://www.megatokyo.com/ is another site that has a large
favicon which causes this bug.
*** Bug 202064 has been marked as a duplicate of this bug. ***
Using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.4a) Gecko/20030401

I do not see icons for some sites like trolltech, but when I do get a bookmarks
icon, I only see the top. It actually appears as if the icon is being scaled
*up* (it's very pixellated) even though it would probably fit in the bookmarks
sidebar if it was the proper size.  The icon does appear correctly in the
location bar and in the tab when using tabbed browsing.

*** Bug 207368 has been marked as a duplicate of this bug. ***
*** Bug 208931 has been marked as a duplicate of this bug. ***
OS: Linux → All
Hardware: PC → All
Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; da-DK; rv:1.4; MultiZilla
v1.4.0.4A) Gecko/20030624 - 1.4rc3

Other sites with this problem:
1) http://www.pc-help.org/obscure.htm
2) http://www.smiley-faces.com/php/index.php?lan=en
3) http://macslash.org/

On Mac OS X 1) 2) seems to display behaviour accoding to comment 5, while 3)
seems to act according to comment 6
Target Milestone: --- → After Firebird 1.0
taking QA contact, sorry about the bugspam
QA Contact: asa → mconnor
WFM on all sites. I see nothing wrong with any of the favicons on the sites
mentioned here. Can one of you reporters reproduce this?

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.5b) Gecko/20030728 Mozilla
Firebird/0.6.1
I can still reproduce this.
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.5b) Gecko/20030729 Mozilla
Firebird/0.6.1

Another example is http://www.sueddeutsche.de/
and http://europa.eu.int/

Note that this bug only refers to the bookmarks sidebar, not to the url bar.
Blocks: 222104
*** Bug 222866 has been marked as a duplicate of this bug. ***
Re comment 12, this is a suite bug, not a firebird bug.

This is still a bug btw, in mozilla 1.6. Would be nice
to get rid of this pretty visual bug for mozilla 1.7,
since we're now an end user product.
Oops sorry. I see now this is posted as a firebird bug. Maybe this bug should be
marked as WFM, or reassigned to the suite product instead? And open a new bug
for the suite.
Re: comment 16 and 17
What build id are you using?
I am using 20040114-trunk/MozJF (sorry, a little old...) and it still have this
problem.
Mozilla/5.0 (Windows; U; Win 9x 4.90; ja-JP; rv:1.7a) Gecko/20040114
Firebird/0.8.0+ (MozJF)


Same problem for suite is bug 199209, I think.
->tweaking summary to lower dupe count...
Summary: Large favicons get cropped rather than resized in bookmark sidebar. → Large favicons get cropped rather than resized in bookmark sidebar and manager
*** Bug 241716 has been marked as a duplicate of this bug. ***
Bug still seen in

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a2) Gecko/20040711
Firefox/0.9.0+
Assignee: hyatt → vladimir
*** Bug 260997 has been marked as a duplicate of this bug. ***
*** Bug 258375 has been marked as a duplicate of this bug. ***
Bug still exists in Firefox 1.0.

Mozilla/5.0 (Windows; U; Windows NT 5.1; de-DE; rv:1.7.5) Gecko/20041107
Firefox/1.0 
(mozilla.org en-US + language-pack)
Assignee: vladimir → vladimir+bm
Attached patch nsTreeBodyFrame.cpp patch (obsolete) — — Splinter Review 
Quick hack.

This patch tells DrawImage() the natural size of the image when its source URI
is "data:...".

Tested on 2005-01-15 CVS / Linux.
Details for patch 172800:

Because tree view has no ability to resize images, I added it in patch.
One important thing is that you should not resize all the images, for example
chrome://global/skin/icons/folder-item.png.
I want to minimize the effect, so I decided to check whether the format of
source URI is "data:...".
This hack will work fine until anyone want to use data: for his theme (or any
image in tree view).  This is I think a reasonable assumption.

Another way is to resize icons before you store them into the bookmark, which
will require considerable work. Is there any existing function to do that?
Attached patch Patch improved not to rely on "data:" URI (obsolete) — — Splinter Review 
Again, currently the tree widget has no ability to resize its icons.  My patch
adds this ability without interfering with existing non-resizable icons (which
use -moz-image-region).

Tested on current CVS / Linux.
Attachment #172800 - Attachment is obsolete: true
Attachment #173946 - Flags: review?(jan)
Attachment #173946 - Flags: review?(jan) → review?(bryner)
Blocks: majorbugs
No longer blocks: majorbugs
Assignee: vladimir+bm → takeshi2
Hmm. Did anyone have a look at this patch already? It would just add 8 lines of
code. Maybe this could be considered for 1.5?
Flags: blocking-aviary1.5?
Comment on attachment 173946 [details] [diff] [review]
Patch improved not to rely on "data:" URI

Vlad, could you take a look at the patch please?
Attachment #173946 - Flags: review?(bryner) → review?(vladimir)
You should probably get Neil or someone more in tune with the tree code to take
a look since it changes the behaviour of the XUL <tree>... this isn't a generic
bookmarks change.  (This also looks like it's a patch against the aviary branch,
not against trunk.)

Might want to change the summary to be something like "Resize large images to
fit in treeview instead of cropping".
Component: Bookmarks → Widget
Flags: review?(vladimir)
Product: Firefox → Core
Summary: Large favicons get cropped rather than resized in bookmark sidebar and manager → Resize large images to fit in treeview instead of cropping
Target Milestone: Future → ---
Version: unspecified → Trunk
Attachment #173946 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #173946 - Flags: review?(neil.parkwaycc.co.uk)
(In reply to comment #30)
> bookmarks change.  (This also looks like it's a patch against the aviary branch,
> not against trunk.)

nsTreeBodyFrame has changed significantly since this patch was attached.
Masayuki, could you attach a more up-to-date patch?
Comment on attachment 173946 [details] [diff] [review]
Patch improved not to rely on "data:" URI

Earlier in PaintImage the source image is cropped to the size of the rect, so I
would suggest that prevention is better than cure ;-)
Attachment #173946 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #173946 - Flags: superreview-
Attachment #173946 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #173946 - Flags: review-
> Masayuki, could you attach a more up-to-date patch?

I'm contacting to Nishimura-san. If he cannot work for this now, I will work on
this.
Flags: blocking-aviary1.5? → blocking-aviary1.5-
(In reply to comment #32)
Please correct me if I am wrong.

> Earlier in PaintImage the source image is cropped to the size of the rect, 

?
If you mean the code around l. 2657
http://lxr.mozilla.org/mozilla/source/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp#2657
>     if (imageSize.height > imageRect.height)
>       imageSize.height = imageRect.height;
>     if (imageSize.width > imageRect.width)
>       imageSize.width = imageRect.width;
, this is not for that purpose. It only restricts drawing size on screen within
the cell size. This operation is necessary even when resizing.

NOTE of PaintImage:
imageRect:
    Cell rectangle on screen
imageSize.x, .y:
    Offset on source image (by -moz-image-region).
imageSize.width, .height:
    Image size on screen
I.e. there is no variable which directly holds "size on source image".
I think it is confusing that usages of parts of imageSize are different from
each other.
Attached patch Another option (obsolete) — — Splinter Review 
I propose a patch to make things clearer.
This clearly splits variables into  "source image rectangle" and "drawing
rectangle on screen".
Attachment #192363 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #192363 - Flags: review?(neil.parkwaycc.co.uk)
Comment on attachment 192363 [details] [diff] [review]
Another option

(In reply to comment #34)
>(In reply to comment #32)
>Please correct me if I am wrong.
I don't know if you are wrong, so I am just cancelling instead of denying.

I believe imageRect is the available area on the screen, while imageSize is
originally the image size, but as the latter gets cropped to the height and
width of imageRect is causes this bug. Instead what I believe should happen is
that if the size is less than the rect then the rect should be reduced and
centered on the size, otherwise allowing the image to scale as necessary.
Attachment #192363 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #192363 - Flags: review?(neil.parkwaycc.co.uk)
Flags: blocking-aviary2.0?
Neil, if you cannot decide if this new patch is the wrong or the right way to
do, can you suggest another reviewer who might help here out.

If this patch doesn't make it into the tree before firefox 1.5, it will never
do, because Vlad plans a complete rewrite of the bookmarks code for the otcho
(2.0) release.
(In reply to comment #36)
Thanks for your reply. I understood your comments.

Let me explain. There are 3 characters:
* cell rectangle on screen
* image size on screen
* rectangle on source image
The first is, as you said, represented by imageRect.
The second is another restriction of drawing area. For example, following CSS
lines define this.
http://lxr.mozilla.org/mozilla/source/browser/components/bookmarks/content/bookmarks.css#13
> treechildren::-moz-tree-image(Name) {
>   width:16px;
>   height:16px;
This is represented by a part of imageSize. I thnik you seem to mis-recognize
this as a part of imageRect.
The third determines which image to draw. Normally this is whole source image.
It can be cropped by -moz-image-region.

Last two things are essentially orthogonal but current code operates them on one
variable imageSize.
As mentioned before, my patch adds the calculation of "rectangle on source
image" and leaves the calculation of "image size on screen" unchanged because
both calculations are necessary.

> I believe imageRect is the available area on the screen, while 
> imageSize is originally the image size, but as the latter gets 
> cropped to the height and width of imageRect is causes this bug.

No. Cropping you mention is related to the first and second things, which has
nothing to do with the third thing.
(Indeed, imageSize, which implies "image size on screen", is already cropped in
GetImageSize as a result of above CSS.)


> if the size is less than the rect then the rect should be reduced 
> and centered on the size, otherwise allowing the image to scale as 
> necessary.

This sentence contains another design choice. For example of bookmarks, if one
site has an 8x8-sized site icon, doesn't it scale to 16x16? I think we should
follow a demand from CSS.
*** Bug 304798 has been marked as a duplicate of this bug. ***
*** Bug 312348 has been marked as a duplicate of this bug. ***
*** Bug 317175 has been marked as a duplicate of this bug. ***
As mentioned above, there are two restrictions to draw icons. This patch resizes the image only when image size is specified.
This is more intuitive, I think.

All my three patches (simplest patch, "make things clearer" patch and this patch) are tested on current trunk and 1.8 branch. (though only one slight modification in second patch is necessary on trunk: "-moz-apperance" to "-moz-appearance")
After installing this extension (Fit Bookmark Icon), it resizes and slims a bookmark icon to fit when it is registered.
Tested on 1.0.7, 1.5 and 2005-11-11-06-trunk.
NOTE:
- This works only on ICON format icons, does not work on GIF or other icons.
- This does not affect already existing icons. If you prefer to change them, please bookmark the same page again and then reload that page for each bookmark.
Blocks: 199209
This is a bit of platform uplift that will be far more visible once we're using favicons for history as well as bookmarks.  Takeshi, this is a core issue that the bookmarks rewrite isn't going to fix, can you request review on whatever patch you consider the best approach.
Flags: blocking-aviary2? → blocking1.8.1+
Blocks: 164421
I see the same problem here: http://www.absoluflash.com/
I am under Firefox 1.5 and Windows Me.
At least from my own usage of the patch I can tell that it does its job without adding any flaws. This would be a 1.5.0.1 build for others to test as well: http://www.pryan.org/mozilla/firefox/amano/Fx-1.5.0.1-O1-SVG-MNG-Exp.exe
Hmm, well it seems I was always struggling as non-cairo gtk[2] toolkit builds can't stretch image regions correctly (they reset the origin to 0,0). And the problem on my windows builds is of course the 12-minute layout link times :-(
*** Bug 331242 has been marked as a duplicate of this bug. ***
I believe I have a complete fix for the bug which involves your patch dated 2005-11-30 03:07 PDT, as well as changes to places.css and a modification to nsTreeBodyFrame::GetImageSize.

The change to places.css specifies the height and width for favicon images displayed in the places tree.

The change to nsTreeBodyFrame::GetImageSize will adjust the destination width/height to maintain the image's aspect ratio in the case where the css specifies width or height, but not both. (This gives the same appearance as when you have an html img tag with a specified width or height, but not both).

I have emailed Takeshi to ask him if I can assign this bug to myself.

I will attach my patch as soon as I figure out how to do so. I'm new at all of this. :)
See comment #49.
This patch does not include the changes I want to make to places.css. I will look for (or file) a separate bug for that.
Attachment #218085 - Flags: superreview?(bryner)
Attachment #218085 - Flags: review?(neil)
Comment on attachment 218085 [details] [diff] [review]
v1.5 patch - incorporates previous patch from 2005-11-30 03:07 PDT

Looks good in general, just a few comments:

>--- nsTreeBodyFrame.cpp	26 Mar 2006 21:30:36 -0000	1.270
>+++ nsTreeBodyFrame.cpp	11 Apr 2006 21:38:03 -0000
>@@ -1838,75 +1838,93 @@ nsRect nsTreeBodyFrame::GetImageSize(PRI
>+  PRBool needWidth = PR_TRUE;
>+  PRBool needHeight = PR_TRUE;
>+  nsSize size;

nsSize's default constructor doesn't set the members to 0.  There's no way for an uninitialized field of |size| to be used, right? (it looks like you always fill in the fields referenced by needWidth/Height if necessary).


>+      if (needWidth && !needHeight) {
>+        // Height was explicitly specified, but width was not explicitly specified.
>+        // Set width in order to maintain aspect ratio.
>+        size.width = (nscoord)((float)imageSize.width * size.height / imageSize.height);

I checked this against the equivalent computation for HTML image scaling (nsImageFrame::GetDesiredSize).  That doesn't use floating point math as far as I can tell, and it seems like we might want to avoid it if we can, since it can introduce round-off errors.

>@@ -2802,87 +2820,115 @@ nsTreeBodyFrame::PaintTwisty(PRInt32    
> void
> nsTreeBodyFrame::PaintImage(PRInt32              aRowIndex,
>                             nsTreeColumn*        aColumn,
>-                            const nsRect&        aImageRect,
>-                            nsPresContext*      aPresContext,
>-                            nsIRenderingContext& aRenderingContext,
>-                            const nsRect&        aDirtyRect,
>-                            nscoord&             aRemainingWidth,
>-                            nscoord&             aCurrX)
>+   const nsRect&        aImageRect,
>+   nsPresContext*      aPresContext,
>+   nsIRenderingContext& aRenderingContext,
>+   const nsRect&        aDirtyRect,
>+   nscoord&             aRemainingWidth,
>+   nscoord&             aCurrX)

Probably best to leave the indentation as it was.

>-  if (imageSize.height > imageRect.height)
>+  // Get the image for drawing.
>+  PRBool useImageRegion = PR_TRUE;
>+  nsCOMPtr<imgIContainer> image; 
>+  GetImage(aRowIndex, aColumn, PR_FALSE, imageContext, useImageRegion, getter_AddRefs(image));
>+  nsMargin bp(0,0,0,0);
>+  GetBorderPadding(imageContext, bp);
>+
>+  nsRect srcRect;
>+  if (!useImageRegion && image) {

We should just be able to return early if there's no image, right?  I think that would make the logic a little easier to follow.


>+    float p2t = aPresContext->PixelsToTwips();
>+    nscoord width, height;
>+    image->GetWidth(&width);
>+    image->GetHeight(&height);
>+    srcRect.width = NSIntPixelsToTwips(width, p2t);
>+    srcRect.height = NSIntPixelsToTwips(height, p2t);
>+    srcRect.x = srcRect.y = 0;

These 4 lines could be combined into a single call to srcRect.SetRect().  Also, you might want to comment here about what's going on, something like

// Use the entire image as the source, unless an image region was specified


>+  if (imageSize.height > imageRect.height) {
>+    if (useImageRegion)
>+      srcRect.height = imageRect.height - bp.TopBottom();
>+   else
>+      srcRect.height = (imageRect.height - bp.TopBottom()) * srcRect.height /
>+                       (imageSize.height - bp.TopBottom());
>     imageSize.height = imageRect.height;

I'm slightly confused about what happens here in the non-imageregion case.  Can you explain what this calculation does?
Attachment #218085 - Flags: superreview?(bryner) → superreview-
Comment on attachment 218085 [details] [diff] [review]
v1.5 patch - incorporates previous patch from 2005-11-30 03:07 PDT

Interesting... it looks like that the existing code never drew small bordered images correctly.
Attachment #218085 - Flags: review?(neil) → review-
Comment on attachment 218085 [details] [diff] [review]
v1.5 patch - incorporates previous patch from 2005-11-30 03:07 PDT

>+  PRBool useImageRegion = PR_TRUE;
>+  nsCOMPtr<imgIContainer> image; 
>+  GetImage(aRowIndex, aColumn, PR_FALSE, imageContext, useImageRegion, getter_AddRefs(image));
Unfortunately useImageRegion is badly named. If you look at GetImage you will see it actually means allowImageRegion. This means that your images will only stretch if they were set by a src attribute rather than by css. Was that your intent? It does not seem reasonable to me.
Oh, and with this patch I don't get any text painted in the trees I tested.
(In reply to comment #49)
> I have emailed Takeshi to ask him if I can assign this bug to myself.

Thanks for your offer! I have no time to make a patch. Feel free to do that!
Attachment #173946 - Attachment is obsolete: true
Attachment #192363 - Attachment is obsolete: true
Attachment #204526 - Attachment is obsolete: true
Neil and Bryner, thanks for the review comments on my last patch.
I'm adding a new patch now.
Instead of modifying GetImageSize, which is used for the twisty as well as the image, I've added two new methods: GetImageDestSize (which returns the destination size of the image, including borders and padding) and GetImageSourceRect (which returns the source rectangle of the image to be displayed).
I have modified PaintImage to use GetImageDestSize and GetImageSourceRect.
I put in a lot of comments in the methods that I added/modified.
I tested the images displayed in the Places trees as well as images that are specified by src and by css. I tested cases where the css specifies width and height, just width, just height, or neither, for the images.
Images are only scaled if the css specifies width and/or height. If the css does not specify width or height, then the image is drawn without scaling and is clipped to the cell bounds (if necessary).
Attachment #218085 - Attachment is obsolete: true
Attachment #219780 - Flags: superreview?
Attachment #219780 - Flags: review?
Attachment #219780 - Flags: superreview?(bryner)
Attachment #219780 - Flags: superreview?
Attachment #219780 - Flags: review?(neil)
Attachment #219780 - Flags: review?
I was about to report a new bug on Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20060321 Firefox/2.0a1 's Places functionality but it appears to be the same bug as the present one.

The related website is http://www.progressia.net/forum and I'm going to attach a screenshot righ now.
(In reply to comment #58)
> Created an attachment (id=219887) [edit]
> a new screenshot showing that this bug still appears on Bon Echo's Places
> functionality
> 

Yes, it is sort of 2 separate bugs.
One is that the places css does not specify the width/height for the images. Ben Goodger is aware of that and I think he'll be fixing the places css.

And then there is this bug, where the images are not scaled, but are only clipped. So, even after the places css is fixed by adding specifing a width of 16 pixels, the images will be clipped to the 16 pixels, instead of scaled to 16 pixels. My "v2 patch" attachment that I added yesterday (2006-04-25) fixes this bug by scaling the images to the specified width/height.

Comment on attachment 219780 [details] [diff] [review]
v2 patch - works for images specified by src and css, text is painted properly

This is looking convincing, but before I try it, would you mind explaining some of the code for me please?

>+// that the result can be easily inflated and deflated.
[Grammar nit: the result may be easily inflated and deflated]

>+  GetImage(aRowIndex, aCol, aUseContext, aStyleContext, useImageRegion, getter_AddRefs(image));
It seems inefficient to me that these helpers call GetImage, and PaintImage does too. GetImage calls mView->GetImageSrc which I believe isn't cheap.

>+        // If the CSS specified the destination height, but not the
>+        // destination width. We need to calculate the width so that we
>+        // maintain the image's aspect ratio.
[Grammar nits: one of either // The CSS specified... or , we need]

>+  r.width += size.width;
>+  r.height += size.height;
Might it be better to create the rect with the image dimensions and then inflate it?

>+  if (useImageRegion) {
>+    // CSS has specified an image region.
That's not quite true; an image region is always available, it just defaults to rect(0, 0, 0, 0) thus the width/height test in GetImageDestSize().

>+  // The imageDestSize width and height have not been adjusted to fit
>+  // within the row height or cell width. Let's do that now.
>+  // Before we adjust the imageDestSize, we save the original rect so we can
>+  // make corresponding adjustments later to the image source rectangle.
What is the advantage of that over simply using the original source?

>+  if (imageDestSize.height > imageRect.height) {
>+    // The imageDestSize is too tall to fit within the row height.
>+    // Adjust imageDestSize height to fit within the row height.
>+    imageDestSize.height = imageRect.height;
>+  }
Why don't you vertically align (also horizontally align cycler cells in the code above that I didn't quote) the image here?

>   // Subtract out the remaining width.
>   nsRect copyRect(imageRect);
>   copyRect.Inflate(imageMargin);
>   aRemainingWidth -= copyRect.width;
>   aCurrX += copyRect.width;
I wonder: if this was done last would that make it unnecessary to copy the rect?

>+    // Get the image destination rectangle - the rectangle
>+    // Use imageRect for the initial x/y.
>+    // Use imageDestSize for the width/height.
So we've only used imageDestSize for its width/height; could you rename it destRect and copy its x/y from imageRect instead?
Thanks for all the suggestions!

Here's patch3.

After implementing your suggestions:
  GetImageDestSize...
    has less local variables.
    no longer calls GetImage
    returns an nsSize that does not include borders and padding
  GetImageSourceRect...
    no longer calls GetImage
  PaintImage
    has less local variables
    the code for updating aRemainingWidth and aCurrX is at the end of the method

> >+  if (useImageRegion) {
> >+    // CSS has specified an image region.
> That's not quite true; an image region is always available, it just defaults to
> rect(0, 0, 0, 0) thus the width/height test in GetImageDestSize().

Ok. Thanks for catching that. I changed it to: 
  if (useImageRegion &&
      (myList->mImageRegion.width > 0 || myList->mImageRegion.height > 0)) {

Please let me know if you think it should be:
  if (useImageRegion &&
      (myList->mImageRegion.width > 0 && myList->mImageRegion.height > 0)) {

> >+  // The imageDestSize width and height have not been adjusted to fit
> >+  // within the row height or cell width. Let's do that now.
> >+  // Before we adjust the imageDestSize, we save the original rect so we can
> >+  // make corresponding adjustments later to the image source rectangle.
> What is the advantage of that over simply using the original source?

I'm not sure what you're asking. If you are asking "Why do need to make
corresponding adjustments later?", then I've added the following comments above
the code where the adjustments are actually made:
    // Here's an explanation. Let's say that the image is 100 pixels tall and
    // that the CSS has specified that the destination height should be 50
    // pixels tall. Let's say that the row height is only 20 pixels. So, in
    // those 20 visible pixels, we want to see the top 20/50ths of the image.
    // So, the sourceRect.height should be 100 * 20 / 50, which is 40 pixels.
    // Essentially, we are scaling the image as dictated by the CSS destination
    // height and width, and we are then clipping the scaled image by the row
    // height and cell width.
Also, I've rearranged this code so that saveImageDestSize is no longer needed.

> >+  if (imageDestSize.height > imageRect.height) {
> >+    // The imageDestSize is too tall to fit within the row height.
> >+    // Adjust imageDestSize height to fit within the row height.
> >+    imageDestSize.height = imageRect.height;
> >+  }
> Why don't you vertically align (also horizontally align cycler cells in the
> code above that I didn't quote) the image here?

The previous code did the alignment just before painting the image and I'm doing
the same. By putting it inside the "if (image)" block, then we don't waste any
time on it when image is null.
Attachment #219780 - Attachment is obsolete: true
Attachment #220446 - Flags: superreview?(bryner)
Attachment #220446 - Flags: review?(neil)
Attachment #219780 - Flags: superreview?(bryner)
Attachment #219780 - Flags: review?(neil)
Comment on attachment 220446 [details] [diff] [review]
v3 patch has fewer local variables, but still tastes great!

>+nsSize nsTreeBodyFrame::GetImageDestSize(nsStyleContext* aStyleContext,
>+   PRBool useImageRegion, imgIContainer* image)
[1] File style is this:
nsSize
nsTreeBodyFrame::GetImageDestSize(nsStyleContext* aStyleContext,
                                  PRBool useImageRegion,
                                  imgIContainer* image)
Same goes for GetImageSourceRect of course.

>+    // CSS has specified the destination width.
>+    PRInt32 val = myPosition->mWidth.GetCoordValue();
>+    size.width = val;
[2] I don't see why this is a temporary, and even so it's the wrong type, as both myPosition->mWidth.GetCordValue() and size.width are nscoord type. The same goes for the height, of course.

>+      // If this column is not a cycler, we do not want to center the image
>+      // horizontally.
>+      // We adjust the imageRect width so that the image is placed at the left
>+      // side of the cell. (Left in a left-to-right context.)
>+      imageRect.width = destRect.width;
OK, so I figured out why you need this here even when you don't have an image :-) [3] You could still merge the height adjustment and centering code though. [4] Also, I'd prefer if you described the image as being placed at the start of the cell.

>+    // For cyclers, we want to center the image horizontally in the cell.
>+    // Adjust the destRect x accordingly.
>+    if (aColumn->IsCycler() && destRect.width < imageRect.width) {
[5] The widths are always equal for non-cyclers so I don't see the point of checking for IsCycler; comparing the widths would seem to suffice.

>+    // Essentially, we are scaling the image as dictated by the CSS destination
>+    // height and width, and we are then clipping the scaled image by the row
>+    // height and cell width.
Personally I would be happy just to scale the image to the final size... mainly because there are two many rects for my tiny mind and this would avoid having to store the original dest size ;-)

[6] I think I found an optimization: Your code does this (pseudocode):
destRect.HCropOrSize(imageRect);
destRect.VCrop(imageRect);
if (image) {
  PaintBackgroundLayer;
  imageRect.Deflate(bp);
  destRect.Deflate(bp);
  destRect.position = imageRect.position;
  destRect.HCenter(imageRect);
  destRect.VCenter(imageRect);
  [source rect stuff]
  DrawImage;
  imageRect.Inflate(bp);
}
I think you could move the deflates to after the centering. This would then mean that you could eliminate the Deflate and Inflate of the imageRect:
destRect.HCropOrSize(imageRect);
if (image) {
  PaintBackgroundLayer;
  destRect.position = imageRect.position;
  destRect.HCenter(imageRect);
  destRect.VCropOrCenter(imageRect);
  destRect.Deflate(bp);
  [source rect stuff]
  DrawImage;
}

r=me with [1] [2] and [4] fixed, and [3] [5] and [6] would be nice too.
Attachment #220446 - Flags: review?(neil) → review+
Attachment #220446 - Attachment description: v3 patch has less local variables, but still tastes great! → v3 patch has fewer local variables, but still tastes great!
Comment on attachment 220446 [details] [diff] [review]
v3 patch has fewer local variables, but still tastes great!

>+nsRect nsTreeBodyFrame::GetImageSourceRect(nsStyleContext* aStyleContext,
>+   PRBool useImageRegion, imgIContainer* image)
>+{
>+  if (useImageRegion &&
>+      (myList->mImageRegion.width > 0 || myList->mImageRegion.height > 0)) {
>+    // CSS has specified an image region.
>+    r.x = myList->mImageRegion.x;
>+    r.y = myList->mImageRegion.y;
>+    r.width = myList->mImageRegion.width;
>+    r.height = myList->mImageRegion.height;

You can just do:

r = myList->mImageRegion;

since that's also a nsRect.


Looks good otherwise, including Neil's suggestions.
Attachment #220446 - Flags: superreview?(bryner) → superreview+
Attached patch v4 patch (obsolete) — — Splinter Review 
Here's patch #4.
It incorporates Neil's suggestions 1-6 and Brian's suggestion.
Also, in GetImageSourceRect, I was able to use just one nscoord (named coord) instead of two (named width and height) when getting the actual image size.
Attachment #220446 - Attachment is obsolete: true
Attachment #220555 - Flags: superreview?(bryner)
Attachment #220555 - Flags: review?(neil)
Attachment #220555 - Flags: superreview?(bryner) → superreview+
Comment on attachment 220555 [details] [diff] [review]
v4 patch

>+nsSize
>+nsTreeBodyFrame::GetImageDestSize(nsStyleContext* aStyleContext,
>+   PRBool useImageRegion, imgIContainer* image)
You didn't quite understand my first comment:
all the parameters should be aligned, rather than wrapped, i.e.
nsSize
nsTreeBodyFrame::GetImageDestSize(nsStyleContext* aStyleContext,
                                  PRBool useImageRegion,
                                  imgIContainer* image)
                           aligned^
Attachment #220555 - Flags: review?(neil) → review+
Attached patch v5 patch — — Splinter Review 
Oh, sorry about that. I noticed the return type on the preceding line, but I completely missed the argument alignment.
Patch #5 aligns the arguments.
Attachment #220555 - Attachment is obsolete: true
Attachment #220662 - Flags: superreview?(bryner)
Attachment #220662 - Flags: review?(neil)
Comment on attachment 220662 [details] [diff] [review]
v5 patch

Whitespace change only - only needs one rereview.
Attachment #220662 - Flags: superreview?(bryner)
Attachment #220662 - Flags: superreview+
Attachment #220662 - Flags: review?(neil)
Attachment #220662 - Flags: review+
fixed-on-trunk
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Have you tested the case where you're rescaling from a -moz-image-region on multiple platforms?  I'm not sure if all the rendering context implementations handle that, given at least some API use, especially pre-cairo...

Though it doesn't look like you're considering this for branches, but if you are, that could be an issue.
Liz, did you find or file the bug for the changes to places.css?
I found the bug for changing the places.css. It is bug 319481.
*** Bug 341600 has been marked as a duplicate of this bug. ***
Since this is blocking 1.8.1 (and thus probably 2.0): Did this land onto the relevant 1.8 branch?
(In reply to comment #73)
> Since this is blocking 1.8.1 (and thus probably 2.0): Did this land onto the
> relevant 1.8 branch?
> 

Also, it says "blocking 1.8.1 +", but it doesn't have "fixed 1.8.1" under key words  like other bugs do. Is it?
is this branch-safe?  If so, please nominate for approval ASAP
Since places isn't on in the 1.8.0 branch by default, we're not going to block on this, but we'll take a patch to clean up the platform. Please generate a branch patch and request approval1.8.1.
Flags: blocking1.8.1+ → blocking1.8.1-
Maybe I do not understand your comment. But the current patch doesn't affect places.css, just nsTreeBodyFrame.cpp and nsTreeBodyFrame.h and should be usable on the branch as is. What should a branch patch provide?
Until now this wasn't been a big deal since the bug wasn't exposed in any significant place. However, we just got a new case where it appears: the "all tabs" menu (see the attachment).

Considering that it badly affects a new feature I think we should push harder towards fixing this on branch.
As per comment 78, we might want to reconsider blocking on this since this bug will be exposed in highly-visible UI.
Flags: blocking1.8.1- → blocking1.8.1?
wait, I thought I fixed this by specifying at 16x16 width of these images!
adam / adam:  what platform are you seeing this bug?

For http://livescore.com/favicon.ico (which is 32x32) this is not a problem for me.

the reason (claim) it should not be happening is:

http://lxr.mozilla.org/mozilla1.8/source/toolkit/themes/pinstripe/global/browser.css#265
http://lxr.mozilla.org/mozilla1.8/source/toolkit/themes/winstripe/global/browser.css#158

but screen shots don't lie.

so, the screen shot of the "all tabs" menu may just be another bug (and not this one?)
I recommend this gets minused.  (I can't do it.)

asa (my clue vendor) pointed out that the screen shot appears to be of a custom them, and not pinstripe,winstripe, or gnomestripe.

if that is true, then the all tabs issue has nothing to do with this bug, and is really a theme issue:  the custom theme needs to do what we did in browser.css and set the width and height to 16 px.
(In reply to comment #83)
> if that is true, then the all tabs issue has nothing to do with this bug, and
> is really a theme issue:  the custom theme needs to do what we did in
> browser.css and set the width and height to 16 px.
> 
Yes that's true, the screenshot wasn't taken on the default theme. I don't see this on Winstripe.

Sorry for a false alarm, then.



> Yes that's true, the screenshot wasn't taken on the default theme. I don't see
> this on Winstripe.

thanks for confirming.

> Sorry for a false alarm, then.

no problem.
Not a blocker, given comment 84
Flags: blocking1.8.1?
When will the patch for this bug land on branch? It's a very old bug and still present on the branch.
Which branch are you referring to? The Firefox 1.5 (Gecko 1.8.0.x) branch?

No. Screenshot from Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1b2) Gecko/20060820 BonEcho/2.0b2 ID:2006082004.

Unfortunately I did a mistake exporting the screenshot. Here a corrected one.

Sorry so much for it...
No. Screenshot from Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1b2) Gecko/20060820 BonEcho/2.0b2 ID:2006082004.

Unfortunately I did a mistake exporting the screenshot. Here a corrected one.

Sorry so much for it...
Attachment #234884 - Attachment is obsolete: true
So with 1.8-branch builds (Firefox 2.0b2), this works in the bookmarks menu, bookmarks toolbar, tab bar, and all tabs menu. It's broken in the bookmarks sidebar and the bookmarks manager.

To fix this, we could either check in the patch from this bug, if that's acceptable on the branch, or work around by setting the width and height explicitly, like suggested in comment 82. Just search for .menu-iconic-icon and look for width and height: http://lxr.mozilla.org/mozilla1.8/search?string=.menu-iconic-icon
(In reply to comment #92)
>... or work around by setting the width and height
> explicitly, like suggested in comment 82. Just search for .menu-iconic-icon and
> look for width and height:
> http://lxr.mozilla.org/mozilla1.8/search?string=.menu-iconic-icon
> 

This bug has absolutely nothing to do with menu items. It's a treeview problem. Again, comment #78 refers to another bug (already fixed), but this one stills present.
This bug is marked fixed, which I assume means it's fixed on the trunk and blocking was minused which should mean it was something like too risky to take for branch.  Someone who actually *knows* please correct/confirm, but I assume that means it'll show up on branch *after* 2 is out.
Should not be too risky. I have been building amano builds for over 1 year with all different kinds of this patch and none regressed anything.
(In reply to comment #94)
> This bug is marked fixed, which I assume means it's fixed on the trunk and
> blocking was minused which should mean it was something like too risky to take
> for branch.  Someone who actually *knows* please correct/confirm, but I assume
> that means it'll show up on branch *after* 2 is out.
> 

(In reply to comment #95)
> Should not be too risky. I have been building amano builds for over 1 year with
> all different kinds of this patch and none regressed anything.

I am hoping that this means it will be reconsidered for 2.0 final since a patch is already baked and shown to be good. Minimal risk, max gain.

Can't hurt to ask for 1.8.1.1., since the fix is well baked.
Flags: blocking1.8.1.1?
Hi,

with the "Fit Bookmark Icon 0.6"-AddOn FF 1.5 was showing favicons very fine in sidebar. Works very good!
But FF 2.0 doesn't seem to work with this Add-on correctly, the cropping icons error is back again! 
Last week I have received the update "FBI 0.7", but the error is still working.

Please, have a look on this!

Regards Mario
(In reply to comment #98)

Hi Mario,
If this is something that worked in earlier versions of the add-on (even though the root problem wasn't fixed for Firefox) you should probably speak to the add-on owner, rather than file in Bugzilla.
(In reply to comment #99)

Hi Wayne,

ok, the Not-Working of the Add-On (it comes from this bug report side) is the one. 
But I can't understand that the bug is marked as FIXED while FireFox further dosn't work correctly. 

Somebody should be informed and have a look to this, the FF error and the AddOn, is FF isn't to get working fine. 
(In reply to comment #100)
> But I can't understand that the bug is marked as FIXED while FireFox further
> dosn't work correctly. 

This bug was not fixed in Firefox 2. It will be fixed in Firefox 3 as you could have learned by reading previous comments. Next time please read the whole bug and refrain from posting redundant comments.


(In reply to comment #100)
> But I can't understand that the bug is marked as FIXED while FireFox further
> dosn't work correctly. 

The Firefox trunk (which will lead to Firefox 3) was fixed. The bug was not fixed for Firefox 2, because it was considered too risky. Because it was decided to only fix the trunk, this bug was marked as FIXED. Maybe it'll also be fixed on the branch for Firefox 2.1, but maybe not.

> Somebody should be informed and have a look to this, the FF error and the
> AddOn, is FF isn't to get working fine.

You say that your add-on used to display icons correctly for Firefox 1.5. But this bug was never fixed for 1.5. If that's true, then it sounds like your add-on did something special to get around the bug, but it's now broken. You should contact the add-on owner about that.
Flags: blocking1.8.1.1? → blocking1.8.1.1-
Depends on: 365742
Target Milestone: --- → mozilla1.9alpha1

Can someone delete all the above spams?
Is it possible to lock this bug forever?
Thanks.

Locking this to editbugs-privileged users only.

Restrict Comments: true
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: