190545 - Meta Improvements and bug-fixes to auto image resizing

Status: RESOLVED EXPIRED

(SeaMonkey :: UI Design, enhancement, P1)

Description

[Brian 'netdragon' Bober]

There are a few issues with auto image resizing (bug 73322). They will be 
carried into here so we can mark the other bug FIXED.

Comment 1

[Brian 'netdragon' Bober]

Reassigning to myself. This is aimed for 1.4a and has been pushed to the front 
of my development pipeline ;-)

Comment 2

[Brian 'netdragon' Bober]

The bugs this depends on will depend on the patch that is in this bug. I know 
that's a bit circular, but I don't want to file yet another bug and mark the 
bugs that blocks this as blocking that one and that one blocking this because 
it will create unecessary bugmail. If you don't agree, and don't mind the 
mail, let me know and I will be happy to ablidge (sp?). ;-)

Comment 3

[Brian 'netdragon' Bober]

Attachment: Prelimary patch from bug 73322, needs work.

Preliminary patch from bug 73322. I will include glazou's comments about the
patch after this paragraph. The final patch will include the fixes to bug
189982, and bug 190551. They are so closely related that there is no point in
fixing them separately.

glazou (Daniel Glazman) said in comment 171 of bug 73322:
>+<groupbox id="imageResizeOptions">
>+    <caption label="&automaticImageResizeOptions.label;"/>
>+    <radiogroup id="automaticImageResizeMethod" align="start"
>+		  prefstring="browser.automatic_image_resizing">
>+	<radio group="automaticImageResizeMethod" value="1"
label="&autoResizeRadio.label;" accesskey="&autoResizeRadio.accesskey;"/>
>+	<radio group="automaticImageResizeMethod" value="2"
label="&lastChoiceRadio.label;" accesskey="&lastChoiceRadio.accesskey;"/>
>+	<radio group="automaticImageResizeMethod" value="0"
label="&noResizeRadio.label;" accesskey="&noResizeRadio.accesskey;"/>

max 80 chars per line please.

>+<!ENTITY automaticImageResizeOptions.label "Resize larger images than the
browser window">

Same thing here. Please check all your code additions, you often go
far beyond that 80 chars limit and it makes your source harder to read
and manipulate.


>-  PRPackedBool		  mImageResizingEnabled;
>-  PRPackedBool		  mImageIsOverflowing;
>-  PRPackedBool		  mImageIsResized;
>+  PRInt32			  mImageResizingMethod; // How to perform
auto-zoom 0=none 1=auto 2=based on last images 
>+  PRPackedBool		  mImageResizingLast;	// Was the image
successfully shrunk before?
>+  PRPackedBool		  mImageShouldResize;	// A combination of
mImageResizingLast and mImageResizingMethod
>+  PRPackedBool		  mImageIsOverflowing;	// Does the image
overflow the window?
>+  PRPackedBool		  mImageIsResized;	// Whether or not image
is shrunken

Just move the PRInt32 before or after the all the PRPackedBool ?

>+  mImageShouldResize = PR_FALSE;
>+  if (mImageResizingMethod == 1) mImageShouldResize = PR_TRUE;

This last test should stand on 2 lines, not one.

>-    if (mImageResizingEnabled) {
>-	nsCOMPtr<nsIDOMEventTarget> target = do_QueryInterface(mImageElement);
>-	target->RemoveEventListener(NS_LITERAL_STRING("click"), this,
PR_FALSE);
>-
>-	target = do_QueryInterface(mScriptGlobalObject);
>-	target->RemoveEventListener(NS_LITERAL_STRING("resize"), this,
PR_FALSE);
>-	target->RemoveEventListener(NS_LITERAL_STRING("keypress"), this,
PR_FALSE);
>-    }
>+    target->RemoveEventListener(NS_LITERAL_STRING("click"), this, PR_FALSE);
>+    target = do_QueryInterface(mScriptGlobalObject);
>+    target->RemoveEventListener(NS_LITERAL_STRING("resize"), this, PR_FALSE);

>+    target->RemoveEventListener(NS_LITERAL_STRING("keypress"), this,
PR_FALSE);
>   }
>   else {
>-    if (mImageResizingEnabled) {
>-	nsCOMPtr<nsIDOMEventTarget> target = do_QueryInterface(mImageElement);
>-	target->AddEventListener(NS_LITERAL_STRING("click"), this, PR_FALSE);
>-
>-	target = do_QueryInterface(aScriptGlobalObject);
>-	target->AddEventListener(NS_LITERAL_STRING("resize"), this, PR_FALSE);
>-	target->AddEventListener(NS_LITERAL_STRING("keypress"), this,
PR_FALSE);
>-    }
>+    target->AddEventListener(NS_LITERAL_STRING("click"), this, PR_FALSE);
>+    target = do_QueryInterface(aScriptGlobalObject);
>+    target->AddEventListener(NS_LITERAL_STRING("resize"), this, PR_FALSE);
>+    target->AddEventListener(NS_LITERAL_STRING("keypress"), this, PR_FALSE);
>   }

You could use a few NS_NAMED_LITERAL_STRING here.

>-    mImageElement->SetAttribute(NS_LITERAL_STRING("style"),
>-				  NS_LITERAL_STRING("cursor: move"));
>+  mImageElement->SetAttribute(NS_LITERAL_STRING("style"),
>+				NS_LITERAL_STRING("cursor: move"));

Hmmm. Call me a maniac but I'd like to see another approach: you put
a new style rule for a given class in the stylesheet applied to the
image document and you set/reset the class on mImageElement instead
of setting/removing the 'cursor' property (btw, you do that now through
'style' attribute and that is more expensive than a CSS OM call for the
single 'cursor' property).
That way, you can easily extend the styles applied to a resized image
if you need it, or if an emebeddor needs it. You can even attach an XBL
behavior that way and that's wort making the change, imho. In particular
if we ever want to extend the image view and put a special menu or whatever.

>+  mImageElement->RemoveAttribute(NS_LITERAL_STRING("width"));

Just wondering why you apply/remove a WIDTH attribute instead of the
CSS 'width' property.

>+  if (!mImageIsOverflowing) {
>+    mImageElement->RemoveAttribute(NS_LITERAL_STRING("style"));

See above.

>+    if (mImageIsOverflowing)
>+	  mImageElement->SetAttribute(NS_LITERAL_STRING("style"),
>+				      NS_LITERAL_STRING("cursor: move"));

See above.

>   nsAutoString eventType;
>   aEvent->GetType(eventType);
>-  if (eventType.Equals(NS_LITERAL_STRING("resize"))) {
>-    CheckOverflowing();
>-  }
>-  else if (eventType.Equals(NS_LITERAL_STRING("click"))) {
>-    ToggleImageSize();
>-  }
>+  if (eventType.Equals(NS_LITERAL_STRING("resize")) || 
>+	eventType.Equals(NS_LITERAL_STRING("click"))) 
>+	  ToggleImageSize();
>   else if (eventType.Equals(NS_LITERAL_STRING("keypress"))) {

Named literal strings again ?







Then he said in comment 172 of bug 73322:

> Hmmm. Call me a maniac but I'd like to see another approach: you put
> a new style rule for a given class in the stylesheet applied to the
> image document and you set/reset the class on mImageElement instead
> of setting/removing the 'cursor' property

I just realize that we don't apply a special stylesheet when only an image
is viewed in the browser....
If you don't want to dive into that, fix the minor things I commented and you
have my r=.

Comment 4

[Brian 'netdragon' Bober]

Marking this bug as blocker of the other two, which is more accurate. Adding 
Daniel Glazman since he offered an r= if his issues were fixed. But before 
that, bug 189982 and bug 190551 will have to be fixed, also.

First I will fix the issues he mentioned, attach the patch, then I will add 
the fixes for bug 189982 and bug 190551 and then attach the final patch ready 
for initial review.

The initial patch fixes the following issues of bug 73322:
* It previously didn't allow manual resizing when you didn't have auto-resize  
selected as a pref.
* If you are looking at a group of images, and normally have auto-resize off, 
you have to go into prefs to turn it on, then turn it off when you are done 
with the group. I added a preference that has Mozilla remember what you did on 
the previous image and do that on the next one.
* The pref Window now has 3 radio boxes for this feature as opposed to one 
checkbox since there are 3 possibilities.

BTW: I have been alerted that Jan is in fact a he. Sorry for accidentally 
misleading anyone in bug 73322 because of an incorrect assumption based on his 
name.

Comment 5

[Brian 'netdragon' Bober]

Attachment: Test image 1472 x 1104

Comment 6

[Brian 'netdragon' Bober]

Attachment: Test image 736 x 552

Comment 7

[Brian 'netdragon' Bober]

Attachment: Screen Shot of Preferences > Appearance pane

<offtopic> I took this using MWSnap 3 http://www.mirekw.com/
It is the best working screenshot program I have ever used with many features.
Its also freeware. </offtopic>

Comment 8

[Brian 'netdragon' Bober]

Attachment: New patch

This patch also has the bugfix for bug 189982.

Comment 9

[Brian 'netdragon' Bober]

Comment on attachment 113179 [details] [diff] [review]
New patch

glazou: Thanks for the previous review. I fixed everything you mentioned, plus
fixed some older code that had more than 80 chars per line. I added enums. I
also fixed bug 189982 in this patch.

>>   nsAutoString eventType;
>>   aEvent->GetType(eventType);
>>-  if (eventType.Equals(NS_LITERAL_STRING("resize"))) {
>>-    CheckOverflowing();
>>-  }
>>-  else if (eventType.Equals(NS_LITERAL_STRING("click"))) {
>>-    ToggleImageSize();
>>-  }
>>+  if (eventType.Equals(NS_LITERAL_STRING("resize")) || 
>>+	eventType.Equals(NS_LITERAL_STRING("click"))) 
>>+	  ToggleImageSize();
>>   else if (eventType.Equals(NS_LITERAL_STRING("keypress"))) {
>
>Named literal strings again ?

Yes, because there is only one of each used in this function.

>Hmmm. Call me a maniac but I'd like to see another approach: you put
>a new style rule for a given class in the stylesheet applied to the
>image document and you set/reset the class on mImageElement instead
>of setting/removing the 'cursor' property (btw, you do that now through
>'style' attribute and that is more expensive than a CSS OM call for the
>single 'cursor' property).
>That way, you can easily extend the styles applied to a resized image
>if you need it, or if an emebeddor needs it. You can even attach an XBL
>behavior that way and that's wort making the change, imho. In particular
>if we ever want to extend the image view and put a special menu or whatever.
>+  mImageElement->RemoveAttribute(NS_LITERAL_STRING("width"));
>
>Just wondering why you apply/remove a WIDTH attribute instead of the
>CSS 'width' property.
> ... etc

I know you said I don't have to fix this. I'm still curious of exactly
what you mean. Are you saying that this should be done through the stylesheet
instead of inline style?
Is it something to keep in mind to fix in the future? i.e. new bug or
something.

>>-  PRPackedBool		  mImageResizingEnabled;
>>-  PRPackedBool		  mImageIsOverflowing;
>>-  PRPackedBool		  mImageIsResized;
>>+  PRInt32			  mImageResizingMethod; // How to perform
>auto-zoom 0=none 1=auto 2=based on last images 
>>+  PRPackedBool		  mImageResizingLast;	// Was the image
>successfully shrunk before?
>>+  PRPackedBool		  mImageShouldResize;	// A combination of
>mImageResizingLast and mImageResizingMethod
>>+  PRPackedBool		  mImageIsOverflowing;	// Does the image
>overflow the window?
>>+  PRPackedBool		  mImageIsResized;	// Whether or not image
>is shrunken
>
>Just move the PRInt32 before or after the all the PRPackedBool ?

It used to be before all the PRPackedBool, and now is after.

Comment 10

[Hixie (not reading bugmail)]

Comment on attachment 113179 [details] [diff] [review]
New patch

We need FEWER prefs not MORE prefs! The existing auto resizing pref should be
removed completely. This is not a feature that should have any options. Mozilla
is already overflowing with user configurable settings, to the point where it
is one of the worst aspects of our UI.

Comment 11

[Prognathous]

I agree that there is no need for any special prefs. Users should be able to 
simply double-click an image to toggle between "Don't resize images" 
and "Always resize images".

Prog.

Comment 12

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

ok but this feature must be switchable on/off for embeddors.

Comment 13

[Hixie (not reading bugmail)]

As per the discussion on IRC, this should be changed as follows:

   1. Remove any UI in the prefs dialog for this.
   2. Make it so we remember what the last state was (auto-sized or not).
   3. That's it.

So then a user just has to click on the image (which should have a better
cursor, as mentioned in bug 189719) to enable this feature, and click it again
to disable it. The user is rarely, if ever, annoyed, since we just keep doing
what he wants.

If embedders really want to be able to disable this completely, then we can keep
the current pref internally, but it should not be exposed in the UI.

Comment 14

[Brian 'netdragon' Bober]

Bug 193039 seems to be a request about image resizing, but I can't understand
it. It appears to be written by someone German.

Comment 15

[Vincent Lefevre]

Here are two examples where auto image resizing doesn't work:
http://www.renater.fr/supervision/map-Renater3/level0/reseau-map-Renater3.html
http://www.lumiere.org/salles/contenu.html
I don't know what is the reason. Perhaps the map?

Comment 16

[Jan Varga [:janv]]

Those 2 examples are actually HTML pages, not standalone image pages.
Try to right click on image and then "View image"

Comment 17

[Vincent Lefevre]

OK, so, image resizing should work in HTML pages too. In addition to completely
see the image, this would be useful to avoid horizontal scrolling, like on
<http://www.grisbi.org/grisbi-manuel008.php>.

Comment 18

[Brian 'netdragon' Bober]

Vincent: First people will want that, then people will want everything in a page
to resize, which is a separate bug. Being able to resize images on a web page is
a separate bug. The logic that goes into detecting some page is just one image
is added overhead we don't need. Any scripts, etc on the page could change it to
be more than one image. Therefore, allowing you to individually resize an image
through the context menu would be the best choice. That has layout
issues/pitfalls that need to be addressed, and therefore is not appropriate for
this bug.

Please see bug 4821 and the other bugs mentioned within the comments for a more
appropriate place to bring such issues up.

Comment 19

[Brian 'netdragon' Bober]

New patch on its way.... I'm just cleaning it up, and testing applying it to the
latest trunk.

Comment 20

[Phillip Stewart :P]

Is there a bug for this situation:
When a large image is shrunk to fit the browser window, and you click on it to
return it to its actual size, the viewport should center on the point you
clicked instead of always showing the top left corner.

Comment 21

[Phillip Stewart :P]

****, I found it 10 seconds after adding that comment: bug 207219
This bug should be set as blocking it.

Comment 22

[Brian 'netdragon' Bober]

I guess I should revive this patch. There are now quite a few bugs that are
blocked by this bug not being fixed.

Comment 23

[miahz]

revived yet?  i don't necessarily want more bugs to be blocked by this one,
especially if it's gonna keep going stale, but there are some more bugs that
might be related to this:
bug 199395
bug 206589
bug 224293

not sure if the rotting patch covers these or not.  and is this seamonkey
specific? or does it filter down to firebird?

Comment 24

[Brian 'netdragon' Bober]

The patch is rotting for the time being, but it will be undusted. I assure you.
I first have to get msvc++ working in Wine with MSVC SP5.

Comment 25

[Kalin Kozhuharov]

What about bug 84123 ?

I think it is related and may be considered in the coures of fixing.

Comment 26

[Brian 'netdragon' Bober]

Kalin: Related but not a dependency.

Comment 27

[Prognathous]

(In reply to Brian 'netdragon' Bober, 2004-01-31, comment #24)
> The patch is rotting for the time being, but it will be undusted. I assure you.

Is there any progress on this front?

Thanks,

Prog.

Comment 28

[Brian 'netdragon' Bober]

Feel free to undust it if you'd like. I haven't had a chance yet. I'll answer
any questions you have.

Comment 29

[Robert Kaiser]

MASS-CHANGE:
This bug report is registered in the SeaMonkey product, but has been without a comment since the inception of the SeaMonkey project. This means that it was logged against the old Mozilla suite and we cannot determine that it's still valid for the current SeaMonkey suite. Because of this, we are setting it to an UNCONFIRMED state.

If you can confirm that this report still applies to current SeaMonkey 2.x nightly builds, please set it back to the NEW state along with a comment on how you reproduced it on what Build ID, or if it's an enhancement request, why it's still worth implementing and in what way.
If you can confirm that the report doesn't apply to current SeaMonkey 2.x nightly builds, please set it to the appropriate RESOLVED state (WORKSFORME, INVALID, WONTFIX, or similar).
If no action happens within the next few months, we move this bug report to an EXPIRED state.

Query tag for this change: mass-UNCONFIRM-20090614

Comment 30

[Robert Kaiser]

MASS-CHANGE:
This bug report is registered in the SeaMonkey product, but still has no comment since the inception of the SeaMonkey project 5 years ago.

Because of this, we're resolving the bug as EXPIRED.

If you still can reproduce the bug on SeaMonkey 2 or otherwise think it's still valid, please REOPEN it and if it is a platform or toolkit issue, move it to the according component.

Query tag for this change: EXPIRED-20100420