Closed Bug 252054 Opened 20 years ago Closed 18 years ago

emulate Opera's image centering

Categories

(Core :: Layout, enhancement)

Product:
Core
Component:
Layout
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED WONTFIX

People

(Reporter: asa, Assigned: pythonesque+bugzilla)

References

Details

Attachments

(1 file, 8 obsolete files)

More Bugfixes
12.07 KB, patch
bzbarsky
: review+
mconnor
: review-
Details | Diff | Splinter Review 
When Opera displays just an image (not an HTML page) it centers the image in the
viewport. This is a more pleasant way to view images. We should emulate this.
The HTML document is generated in nsImageDocument::CreateSyntheticDocument
(http://lxr.mozilla.org/seamonkey/source/content/html/document/src/nsImageDocument.cpp#500).
 Inline style could be tossed on nodes as needed to do this.
Whiteboard: [good first bug]
In fact, I bet just setting style="text-align: center" on the <body> will work.
does opera just center horizontally, or also vertically?
It's both horizontal and vertical. Sorry for leaving that point off. (probably
unrelated but the vertical centering seems a bit "slower". it sometimes doesn't
recenter when enlarging the window vertically and if I resize (up and down)
vertically very quickly, it flickers a vertical scrollbar.)
(Also probably not of interest to anyone (though it might help explain how they
accomplish the centering), on further investigation of the Opera feature, it
appears that Opera's vertical centering is pretty broken. If you're careful not
to do any horizontal resizing of the window, and just do vertical shrinking, the
image isn't recentered and you get a vertical scrollbar. Any purely vertical
resizing doesn't adjust the image placement. You have to move some in the
horizontal to get the realignment of the image. Most users probably don't notice
this, though, since they're probably resizing in both directions.)
If you're going to do this, please provide a pref to turn it off. I don't
generally use a browser just to view images, except when investigating
characteristics of a web page image (view -> image). For this case, I want the
image in the normal top-to-bottom, right-to-left position, especially when the
image background is the same color as the viewport background. Simply having the
image dimensions in the titlebar isn't good enough for me to gain a sense of the
image's real size, which is helped by apparent padding, or lack thereof, when
tucked into the upper left.
Severity: normal → enhancement
OS: Windows XP → All
Hardware: PC → All
*** Bug 290109 has been marked as a duplicate of this bug. ***
What do you expect the pref to look like? Should it just be "center images when
viewed alone" or should there be, for example, a difference when generated using
"view image" as you supposed?
Attached patch Center Images v1.0 (obsolete) — — Splinter Review 
Tested this myself, and it appears to work, both with and without the
preference set.  The hidden preference in question is
browser.enable_automatic_image_centering, incidentally.  Requesting review.
Attachment #185446 - Flags: review?(peterv)
Attachment #185446 - Flags: review?(peterv)
Attached patch Center Images v1.1 (obsolete) — — Splinter Review 

        Attachment #185446 -
        Attachment is obsolete: true

        Attachment #185739 -
        Flags: review?

    
    

    
  
Comment on attachment 185739 [details] [diff] [review]
Center Images v1.1

The previous patch had a number of issues.  I've tested this patch fairly
thoroughly, so I think this one's good, but I'd welcome anyone who wants to
test this patch's doing so.

        Attachment #185739 -
        Flags: review? → review?(peterv)

    
  

        Attachment #185739 -
        Flags: review?(peterv)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Center Images v1.2 (obsolete)
        — 
          — Splinter Review
      

    


  
Added comments and changed some confusing code on the advice of bz.

        Attachment #185741 -
        Flags: review?(peterv)

    
  

        Attachment #185739 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 185741 [details] [diff] [review]
Center Images v1.2

the IDL needs a new IID.

    
    

    
  

        Attachment #185741 -
        Attachment is obsolete: true

        Attachment #185741 -
        Flags: review?(peterv)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Center Images v1.3 (obsolete)
        — 
          — Splinter Review
      

    


  
Sorry about the empty comment.	I accidentally inserted whitespace.  Anyway,
this should be the last patch, unless there's something else I'm missing.

        Attachment #185806 -
        Flags: review?(peterv)

    
    

    
  
Has anyone tested this patch with image zooming?
[i.e. click on a pixel in the image and the image zooms to that pixel]

    
  

        Attachment #185806 -
        Attachment is obsolete: true

        Attachment #185806 -
        Flags: review?(peterv)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Center Images v1.4 (obsolete)
        — 
          — Splinter Review
      

    


  
Argh.  No, it didn't do that properly, or at least it didn't center properly. 
This patch should fix that.  I'm fairly certain it works properly.  Test it if
you like; that would be cool.

    
  

        Attachment #185832 -
        Flags: review?(peterv)

    
  

        Attachment #185832 -
        Flags: review?(peterv) → review?(jst)

    
  

        Attachment #185832 -
        Attachment is obsolete: true

        Attachment #185832 -
        Flags: review?(jst)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Center Images v1.5 (obsolete)
        — 
          — Splinter Review
      

    


  
Yeah.  This bugfix is actually more of a feature thing, but I figured that if
you zoom in on an image using the standard RestoreImage() (like the keyboard,
whatever) it should center automatically, even if the image is considerably
larger than the screen.  This doesn't affect the loading in a non-resizing
situation, because A) Opera doesn't do that and B) it doesn't feel natural to
me.  Anyone who feels that this shouldn't be included (or should be included in
both situations), please say so.

        Attachment #185994 -
        Flags: review?(bzbarsky)

    
    

    
  
I won't be able to get to this review until I get back to town (so in July).

    
    

    
  
Comment on attachment 185994 [details] [diff] [review]
Center Images v1.5

Hm.  I'm not going to be around at the time, so I'll ask jst.  Thanks anyway.

        Attachment #185994 -
        Flags: review?(bzbarsky) → review?(jst)

    
  

        Attachment #185994 -
        Flags: review?(jst) → review?(bzbarsky)

    
    

    
  
Never mind... bz, could you please review this patch?  'Twould be very helpful,
and I know at least a few people who want this in (provided it's not too high-risk).

    
    

    
  
I won't be able to review this for at least a week, possibly longer.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Center Images v1.6 (obsolete)
        — 
          — Splinter Review
      

    


  
Fixing bitrot.

        Attachment #185994 -
        Attachment is obsolete: true

        Attachment #194059 -
        Flags: review?(bzbarsky)

    
  

        Attachment #185994 -
        Flags: review?(bzbarsky)

    
    

    
  
Just out of curiosity, is this bug likely to receive any love until after 1.5 is
released?
Status: NEW → ASSIGNED

    
  
Assignee: nobody → pythonesque+bugzilla
Status: ASSIGNED → NEW

    
  
Status: NEW → ASSIGNED

    
    

    
  
Probably, but it's not likely to make 1.5.... :(

    
    

    
  
Comment on attachment 194059 [details] [diff] [review]
Center Images v1.6

>Index: public/nsIImageDocument.idl
>+  readonly attribute boolean imageCenteringEnabled;
>+  readonly attribute boolean imageVerticallyCentered;

This patch doesn't seem to use these.  Is the idea to expose them to extensions
or future UI (eg so that the context menu can be used to toggle between enabled
and disabled or something like it can toggle between shrink-to-fit and not in
Seamonkey and Thunderbird)?

>Index: src/nsImageDocument.cpp
>-#define AUTOMATIC_IMAGE_RESIZING_PREF "browser.enable_automatic_image_resizing"
>+#define AUTOMATIC_IMAGE_RESIZING_PREF  "browser.enable_automatic_image_resizing"

Why that change?

>@@ -441,45 +467,71 @@ nsImageDocument::RestoreImageTo(PRInt32 
>+  if (aX < 0 || aY < 0) { // Resize to center
>+    aX = NSToCoordFloor(ratio * mImageWidth / 2);
>+    aY = NSToCoordFloor(ratio * mImageHeight / 2);
>+    
>+  }

There seems to be an extra blank like there.

I assume the idea with this change is that if the user clicked outside the
image when unzooming it then we should center the image?  Do we really want to
be centering images that are bigger than the viewport, though?	That seems kind
of odd to me...

>@@ -616,16 +668,21 @@ nsImageDocument::CreateSyntheticDocument

>+  if (mImageCenteringEnabled) { // Horizontal resize

"resize"?

>+    body->SetAttr(kNameSpaceID_None, nsHTMLAtoms::style,
>+                  NS_LITERAL_STRING("text-align: center"), PR_FALSE);

How does this behave when the image is wider than the viewport?

>@@ -656,46 +713,71 @@ nsImageDocument::CheckOverflowing(PRBool
>   nsRect visibleArea = context->GetVisibleArea();
...
>+  if(mImageVerticallyCentered) { // Get rid of styles so visibleArea works properly

But you've already called GetVisibleArea.  I'm not sure I follow...

>+  PRInt32 clientHeight = NSTwipsToIntPixels(visibleArea.height, t2p);

Why do you need to compute it all the way up here, if you won't use it for a
while?

>+  if (mImageCenteringEnabled && mImageHeight < clientHeight) { // Vertical resize

"Resize"?

So we're only doing this for the case when the intrinsic size of the image is
shorter than the viewport height?  That might be worth documenting somewhere
(eg near the decls of the members that control this behavior).

>+    PRInt32 apparentHeight = PRInt32(mImageHeight);
>+    if (mImageIsResized) {
>+      apparentHeight = NSToCoordFloor(GetRatio() * mImageHeight);
>+    }

Why set apparentHeight twice when mImageIsResized?

>+    PRInt32 value = PRInt32((clientHeight - apparentHeight) / 2);

"value"?  Is there no better name for this?

>+    nsAutoString valueString;
>+    valueString.AppendLiteral("padding-top: ");
>+    valueString.AppendInt(value);
>+    valueString.AppendLiteral("px; margin-top:0; text-align:center");

Why text-align:center?

Do you need to worry about the body margins here?

>+    content->SetAttr(kNameSpaceID_None, nsHTMLAtoms::style, valueString, PR_TRUE);

Doesn't this clobber the cursor styles image resizing sets?

>+  else if (mImageVerticallyCentered) { // Remove vertical-centering styles
>+    content->SetAttr(kNameSpaceID_None, nsHTMLAtoms::style,
>+                     NS_LITERAL_STRING("text-align:center"), PR_TRUE);

Again, whence text-align, and it seems that we're racing against resizing to
see who will SetAttribute() later...  Any reason not to use GetStyle() and set
individual properties?

There seems to be a fundamental asymmetry between the handling horizontal and
vertical centering in this patch.  (eg horizontal centering is "always
enabled", and there is no way to detect when an image is actually horizontally
centered).  Is there a good reason for this?

        Attachment #194059 -
        Flags: review?(bzbarsky) → review-

    
    

    
  
(In reply to comment #26)
> (From update of attachment 194059 [details] [diff] [review] [edit])
> >Index: public/nsIImageDocument.idl
> >+  readonly attribute boolean imageCenteringEnabled;
> >+  readonly attribute boolean imageVerticallyCentered;
> 
> This patch doesn't seem to use these.  Is the idea to expose them to extensions
> or future UI (eg so that the context menu can be used to toggle between enabled
> and disabled or something like it can toggle between shrink-to-fit and not in
> Seamonkey and Thunderbird)?
The patch does use them; imageVerticallyCentered probably shouldn't be exposed
to the user, though.  imageCenteringEnabled is useful both for debugging (on my
part) and in the likely case that some users won't want to have image centering
enabled.
> 
> >Index: src/nsImageDocument.cpp
> >-#define AUTOMATIC_IMAGE_RESIZING_PREF "browser.enable_automatic_image_resizing"
> >+#define AUTOMATIC_IMAGE_RESIZING_PREF  "browser.enable_automatic_image_resizing"
> 
> Why that change?
> 
> >@@ -441,45 +467,71 @@ nsImageDocument::RestoreImageTo(PRInt32 
> >+  if (aX < 0 || aY < 0) { // Resize to center
> >+    aX = NSToCoordFloor(ratio * mImageWidth / 2);
> >+    aY = NSToCoordFloor(ratio * mImageHeight / 2);
> >+    
> >+  }
> 
> There seems to be an extra blank like there.
> 
I'll fix these.
> I assume the idea with this change is that if the user clicked outside the
> image when unzooming it then we should center the image?  Do we really want to
> be centering images that are bigger than the viewport, though?	That seems kind
> of odd to me...
> 
When the image is resized to fit the viewport, it should be centered.  I did the
patch a long time ago, so I don't remember exactly, but if an image is supposed
to be centered it should be able to hold any values that RestoreImageTo throws
at it.
> 
> >+  if (mImageCenteringEnabled) { // Horizontal resize
> 
The image is being resized to fit the window.  I'll try to clarify further with
these comments.
> "resize"?
> 
> >+    body->SetAttr(kNameSpaceID_None, nsHTMLAtoms::style,
> >+                  NS_LITERAL_STRING("text-align: center"), PR_FALSE);
> 
> How does this behave when the image is wider than the viewport?
If automatic image resizing is enabled, the image is never wider than the
viewport.  If not, this change doesn't really impact anything - it will just
stretch the viewport width, like it should.
> 
> >@@ -656,46 +713,71 @@ nsImageDocument::CheckOverflowing(PRBool
> >   nsRect visibleArea = context->GetVisibleArea();
> ...
> >+  if(mImageVerticallyCentered) { // Get rid of styles so visibleArea works
properly
> 
> But you've already called GetVisibleArea.  I'm not sure I follow...
Again, I did this patch awhile ago, but there were issues with the visible area
deflating too much when the image was vertically centered, because it sets
padding on the top and bottom (IIRC).  So it would end up centering the image
within a really narrow space, which wasn't the intended behavior.
> 
> >+  PRInt32 clientHeight = NSTwipsToIntPixels(visibleArea.height, t2p);
> 
> Why do you need to compute it all the way up here, if you won't use it for a
> while?
> 
Because I need the actual visible height, not the visible height minus the
margin and padding.
> >+  if (mImageCenteringEnabled && mImageHeight < clientHeight) { // Vertical
resize
> 
> "Resize"?
Yes, yes.  I'll correct the ambiguities.
> 
> So we're only doing this for the case when the intrinsic size of the image is
> shorter than the viewport height?  That might be worth documenting somewhere
> (eg near the decls of the members that control this behavior).
> 
If the image is greater than the viewport height, the way automatic image
resizing currently works, the image will be scaled until it is exactly as tall
as the available viewport height.  And if automatic image resizing is disabled,
or you're zoomed in, on such an image, then it's irrelevant whether automatic
image centering is enabled or not.  So I think it sort of follows.  If you think
I should, though, I'll add comments.
> >+    PRInt32 apparentHeight = PRInt32(mImageHeight);
> >+    if (mImageIsResized) {
> >+      apparentHeight = NSToCoordFloor(GetRatio() * mImageHeight);
> >+    }
> 
> Why set apparentHeight twice when mImageIsResized?
I could throw that into an else, if you want, but I don't think there are any
perf wins, unless I misunderstanding PRInt32.
> 
> >+    PRInt32 value = PRInt32((clientHeight - apparentHeight) / 2);
> 
> "value"?  Is there no better name for this?
"Offset" I suppose.
> 
> >+    nsAutoString valueString;
> >+    valueString.AppendLiteral("padding-top: ");
> >+    valueString.AppendInt(value);
> >+    valueString.AppendLiteral("px; margin-top:0; text-align:center");
> 
> Why text-align:center?
It's the simplest (and fastest) way to get horizontal centering working. 
Vertical centering is slow because it relies on the resize method and has to set
margins.  If there were a way to automatically vertically center with CSS, I
would use that.
> 
> Do you need to worry about the body margins here?
> 
No, because I already included it when I set clientHeight before I deflated
margin and padding.
> >+    content->SetAttr(kNameSpaceID_None, nsHTMLAtoms::style, valueString,
PR_TRUE);
> 
> Doesn't this clobber the cursor styles image resizing sets?
No, those are set on the image, not the body.
> 
> >+  else if (mImageVerticallyCentered) { // Remove vertical-centering styles
> >+    content->SetAttr(kNameSpaceID_None, nsHTMLAtoms::style,
> >+                     NS_LITERAL_STRING("text-align:center"), PR_TRUE);
> 
> Again, whence text-align, and it seems that we're racing against resizing to
> see who will SetAttribute() later...  Any reason not to use GetStyle() and set
> individual properties?
Probably not, unless my memory fails me once again.  When I wrote the patch, I
didn't know how to use that, though.  I'm still not sure I do, but I could
probably reason it out.
> 
> There seems to be a fundamental asymmetry between the handling horizontal and
> vertical centering in this patch.  (eg horizontal centering is "always
> enabled", and there is no way to detect when an image is actually horizontally
> centered).  Is there a good reason for this?
> 
Yes.  Horizontal centering is MUCH easier, because of the fact that there is
actually an easy way to do it with pure CSS.  Vertical centering, on the other
hand, is something that it's impossible to do without some sort of scripting
trickery.  To be honest, as I said earlier, the "vertically centered" attribute
doesn't need to be exposed for the user; I can change that in the next version
of this patch.

If you have any issues with my responses or any lingering concerns with the bits
of the patch I haven't addressed here, please tell them to me; otherwise, I'll
begin working on an updated version of the patch.

    
    

    
  
Oh, wait.  I remember now.  If you give it values less than 0, RestoreImageTo
will resize to center, which was a workaround method of allowing the 'zoom in'
functionality to automatically zoom to center.  I'll write a comment.  Sorry for
the confusion about that.

    
    

    
  
Personally I prefer images at the top left... Are we sure we want this?

    
    

    
  
Ian: Hence the preference.  Most people I've discussed this with, however (and
I'm actually thinking more nontechnical users than not) have liked the idea of
viewing images centered, though.  Is there an especially compelling reason that
this should be off by default?

    
    

    
  
This is not the kind of thing where it makes sense to have a UI-visible
preference. Maybe it could be the kind of thing it would make sense to have
under the control of a skin.

I'm also skeptical of adding code to do this, especially if one of the code
branches will be used by just a small fraction of the userbase.

    
    

    
  
It's not UI-visible at the moment, nor is it intended to be.

I'm not sure how a theme would have control over this; are themes supposed to be
messing with stuff like nsImageDocument.cpp?  Adding the (hidden, both from the
UI and from the default list) preference took just a few lines, and was useful
for debugging and stuff.  I'm not sure what you meant by your last line, either;
the "code branch" that is "not centering images" just ignores some of the stuff
I included.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Fixes from comment 26 (obsolete)
        — 
          — Splinter Review
      

    


  
This should fix bz's complaints, anyway.  Hixie, if you really think there
shouldn't be a preference, feel free to talk me out of it, but I think this way
is fine (the codepath introduced does nothing much other than set different
padding and text-align properties).

    
  

        Attachment #194059 -
        Attachment is obsolete: true

        Attachment #199246 -
        Flags: superreview?(jst)

        Attachment #199246 -
        Flags: review?(bzbarsky)

    
  

        Attachment #199246 -
        Flags: superreview?(jst)

    
    

    
  
> The patch does use them

I don't see where.  What am I missing?  Note that I'm talking about the IDL
properties, not the nsImageDocument members or the preferences.

> It should be able to hold any values that RestoreImageTo throws at it.

Agreed, but with your change it seems like clicking outside the image will zoom
in on the center of the image.  That seems odd to me.  Why would someone want this?

> It will just stretch the viewport width

Er... No.  The viewport width is imposed from outside the document, no?  I guess
this is probably ok because of how text-align behaves, but make sure to test!

> but there were issues with the visible area deflating too much when the image
> was vertically centered

Please document that clearly.

> Because I need the actual visible height, not the visible height minus the
> margin and padding.

Comment that.

> If the image is greater than the viewport height, the way automatic image
> resizing currently works, the image will be scaled until it is exactly as tall
> as the available viewport height.

That's not true.  For example, an image that's 1.5 times the viewport height and
2 times the viewport width, will be scaled down by a factor of 2, so that it's
the viewport width and 0.75 times the viewport height.

> I could throw that into an else, if you want

Please do.

> It's the simplest (and fastest) way to get horizontal centering working. 

Ah, I see.  I thought you were messing with the padding on the image.


    
    

    
  
Comment on attachment 199246 [details] [diff] [review]
Fixes from comment 26

>Index: public/nsIImageDocument.idl
>+  /* Whether the pref for image centering has been set. */
>+  readonly attribute boolean imageCenteringEnabled;
>+

Again, do you expect someone to use this?  If so, whom?

>Index: src/nsImageDocument.cpp
>+  mImageCenteringEnabled = 
>+    nsContentUtils::GetBoolPref(AUTOMATIC_IMAGE_CENTERING_PREF, PR_TRUE);

I've thought about this, and the gecko default should be compatible behavior
(that is, don't pass PR_TRUE here).  If some app wants to enable this, it
should set the pref.

>+  mImageVerticallyCentered = PR_FALSE;

This can happen in the constructor, using an initializer, right?

> nsImageDocument::RestoreImageTo(PRInt32 aX, PRInt32 aY)

>+  if (aX < 0 || aY < 0) { // For negative values, this centers the viewport
>+    aX = NSToCoordFloor(ratio * mImageWidth / 2);
>+    aY = NSToCoordFloor(ratio * mImageHeight / 2);    

Sorta centers.	It's off by the body margins, I believe...

In any case, as I said, I think this code is very odd.	In some cases it leads
to very unintuitive results.  So I really don't like it.

>+nsImageDocument::RestoreImageSize()

>+    if (mImageCenteringEnabled) {
>+      CheckOverflowing(PR_FALSE);

Please document why this is needed.

>@@ -657,46 +709,86 @@ nsImageDocument::CheckOverflowing(PRBool
>-  mImageIsOverflowing =
>+  mImageIsOverflowing = 

Please don't add that space.

>+  // We center vertically if the image is shorter than the viewport
>+  if (mImageCenteringEnabled && mImageHeight < clientHeight) {

As I said, this should be using apparentHeight for the comparison, not
mImageHeight.

        Attachment #199246 -
        Flags: review?(bzbarsky) → review-

    
    

    
  
(In reply to comment #35)
> Again, do you expect someone to use this?  If so, whom?
Sorry if I was unclear.  The patch itself does not ever set this.  This is
simply there so that a preference exists, as a few people (mostly developers)
have expressed consternation at having their images centered by default.  It was
also a useful tool for debugging, but that isn't why it was originally put into
the patch.
> 
> >+  mImageVerticallyCentered = PR_FALSE;
> 
> This can happen in the constructor, using an initializer, right?
It's really an internal variable, and is set appropriately as needed, so there's
no need to pass anything to it.  If you're referring to the actual
nsImageDocument constructor, that's empty, with a comment to the effect that it
should remain empty.

> Sorta centers.	It's off by the body margins, I believe...
It's possible, but as far as I can tell it's not off by more than everything
else is off, because all other calls to RestoreImageTo subtract the body margins
first.  I fixed up that part to be less unintuitive, but it should still be
centering to the same place.  If this is something that you think should
seriously be fixed, I could probably make it figure out the exact amount the
body margins were contributing, but by the time the image height is greater than
the viewport's (which is the only time this is relevant) the margins are fairly
unimportant anyway.

I have a working patch with fixes for the other changes, but I'd like to know
your position on these points before I submit it.

    
    

    
  
> This is simply there so that a preference exists,

The preference exists independently of the changes to the IDL that you made.

> If you're referring to the actual nsImageDocument constructor,

Yes.

> with a comment to the effect that it should remain empty.

No, it's a comment that says that the memory is zeroed out.  Which raises the
question of why we're setting anything to PR_FALSE in Init() -- it's already set
that way.

On the centering when zooming in thing, I still don't really see why that makes
sense.  That is, why does it make sense to center the zoomed-in image in that case?

    
    

    
  
Given that we're really riding the edge of the 4.9MB hardlimit on download size,
I think we should not be accepting such new code. This is going to be a rarely
used code path, which means rarely tested; it'll add to our codesize with little
benefit... I would really strongly recommend just fixing this in your
userContent stylesheet and not having this checked in.

    
    

    
  
Hixie, this isn't just a userContent.css change (unfortunately, there's no
pure-CSS way for this to work), and I'm doing it more because there was a bug to
get images centered than because I particularly regretted Firefox's not having
the feature.  I was also under the impression that, at least under Firefox, it
would default to on, so it would receive plenty of testing.  The actual patch
size is fairly small, and I doubt it increases anything significant.

That said, if the drivers feel that it shouldn't get in at all, I'm fine with it.

    
    

    
  
(In reply to comment #37)
> > This is simply there so that a preference exists,
> 
> The preference exists independently of the changes to the IDL that you made.
> 
Good point.  There's no real reason why JS needs to know about this, so I'll
remove it.
> > If you're referring to the actual nsImageDocument constructor,
> 
> Yes.
> 
> > with a comment to the effect that it should remain empty.
> 
> No, it's a comment that says that the memory is zeroed out.  Which raises the
> question of why we're setting anything to PR_FALSE in Init() -- it's already set
> that way.
Okay, I'll fix that.
> 
> On the centering when zooming in thing, I still don't really see why that makes
> sense.  That is, why does it make sense to center the zoomed-in image in that
case?
As I said, I fixed it somewhat.  It currently centers the image before calling
the RestoreImageTo function - the function itself doesn't care if it's fed
negative numbers or not.  The only time that the centering is toggled is when
the keyboard is used to zoom in.  If you don't feel this is good behavior, it
can be (trivially) removed.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        More Bugfixes
        — 
          — Splinter Review
      

    


  
Okay, updated patch.  There may be remaining quibbles, but if so I either
haven't heard about them or they're that it shouldn't be included anyway and/or
the image shouldn't center upon zoom.

        Attachment #199246 -
        Attachment is obsolete: true

        Attachment #200021 -
        Flags: review?(bzbarsky)

    
    

    
  
Default to on? I really don't think this should go in without demonstrating that
there is a definite gain here. I've used both Opera's and Firefox's renderings
for images and frankly Firefox's is easier to understand (especially for small
images). I spoke with Ben last week when activity here started and he indicated
a preference to not changing Firefox's behaviour (and he's the UI module owner,
so it would seem to be his call).

    
    

    
  
OK, so from a technical point of view I'm ok with the patch if it has consumers.
 If no one's planning to use this, I'd rather not add unused code, of course. 
I'm still waiting on one of the UI folks cced on this bug to comment on that part.

        Attachment #200021 -
        Flags: review?(bzbarsky) → review+

    
  

        Attachment #200021 -
        Flags: superreview?(dbaron)

    
    

    
  
I would have requested SR from Ben, but it says he's not doing it at the moment.

    
  

        Attachment #200021 -
        Flags: superreview?(dbaron) → superreview?(mconnor)

    
    

    
  
Comment on attachment 200021 [details] [diff] [review]
More Bugfixes

Canceling any spurious requests for SR until I get some sort of verbal (dis)approval.

        Attachment #200021 -
        Flags: superreview?(mconnor)

    
  

        Attachment #200021 -
        Flags: review?(mconnor)

    
    

    
  
Comment on attachment 200021 [details] [diff] [review]
More Bugfixes

I don't think this is better, and in fact I'm almost certain it's less useful in the browser context.

        Attachment #200021 -
        Flags: review?(mconnor) → review-

    
    

    
  
'Kay, wontfixing.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → WONTFIX

    
    

    
  
And in these times of browser wars when every detail matters, this isn't fixed why exactly?

Especially since it's friggin' 5 years old!!
Whiteboard: [good first bug]

    
    

    
  
1. We don't want the options to get too complex for the users.
2. Many people prefer the current behavior.
3. Firefox is already a large download, and this and others like it would increase the size further.  Initially we touted our small download size as a benefit.

    
    

    
  
Increase the size?

You know that to all that is needed for this is to add

html > body > img:only-child {
 position: absolute;
 top: 0;
 left: 0;
 right: 0;
 bottom: 0;
 margin: auto;
 padding: 3px;
}

to usercontent.css? :)

    
    

    
  
I'd like to add that this always looks nicer in Opera, and seems such a small but attractive change (ie. a vote for not preferring the current behaviour)

    
    

    
  
BTW, this can be done by adding these lines in your profile/chrome/usercontent.css :

/* Render images-only in the middle */
html > body > img:only-child {
  position: absolute;
  top: 0;
  left: 0;
  right: 0;
  bottom: 0;
  margin: auto;
  padding: 3px;
}

    
    

    
  
Despite "wontfix" it seems to be fixed in todays nightly, supposedly due Bug 472942. (Almost exactly the way proposed above :])

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: