Closed Bug 237628 Opened 20 years ago Closed 20 years ago

No change of image size if changed in Format->Image Properties

Categories

(SeaMonkey :: Composer, defect, P1)

Product:
SeaMonkey
Component:
Composer
Platform:
x86
All
Type:
defect

Tracking

(Not tracked)

Status:
VERIFIED FIXED
Milestone:
mozilla1.8alpha3

People

(Reporter: winthir, Assigned: bzbarsky)

References

Details

Attachments

(3 files)

Testcase (HTML)
822 bytes, text/html
Details
Testcase (GIF)
824 bytes, image/gif
Details
Proposed patch
2.02 KB, patch
glazou
: review+
peterv
: superreview+
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.7b) Gecko/20040315
Build Identifier: Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.7b) Gecko/20040315

Image size cannot be changed in Format->Image Properties. Either Actual Size or
Custom Size don't work. However, images can be pulled larger and smaller by
pulling on the image.

Reproducible: Always
Steps to Reproduce:
1.
2.
3.
Confirming.
Doesn't act on / remember changes in that setting on Linux either - OS: All
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 98 → All
Summary: No change of image size if changed in Format->Image Properties → No change of image size if changed in Format->Image Properties
I can reproduce this bug.  I am running 1.7 (20040316) on the latest Panther. 
It never fails.  I have a feeling it is tied to another bug, for which I am
searching and I may report yet.  The other bug has to do with page and table
background colors failing to change when changes are made through the
"Format"-->"Page Colors and Background" or through "Table or cell properties"
from the context menu.
*** Bug 240347 has been marked as a duplicate of this bug. ***
*** Bug 240548 has been marked as a duplicate of this bug. ***
confirming, taking, raising radar.
Assignee: composer → daniel
Severity: normal → major
Priority: -- → P1
Target Milestone: --- → mozilla1.8alpha
Very strange... It seems this is broken because nsEditor::CloneAttributes does
not work properly and that one does not work properly because of
nsAttrAndChildArray::GetAttr. Still investigating.
Sicking: this seems to be a bad side effect of your checkin for bug 195350...
I am currently trying to confirm that; if that's the case, please consider all
possibilities for a fix, including a backout. Changing an image size is a highly
visible feature and we just can't let that as it is.
Daniel, which exact attributes are being gotten, from what object, and how?  At
the very least, where is the relevant code?  It's a lot easier to debug problems
if you don't have to redo all the work that's already been done.
I think the problem is in nsMappedAttributes but I am not certain for the moment.
I have narrowed the pb a bit: it appeared between 01-jan-2004 and 30-jan-2004.

Which call to GetAttr looks like it's failing? I debugged this and it looks like
it's getting the width and height right from the temporary <img> element. Though
it took me some head-scratching to see that it was working fine, the new
stringclasses are kind'a tricky in that they can contain an inline buffer
without using it, so sometimes it looks like the value is an empty string,
whereas it has a set value.
(In reply to comment #10)
> Which call to GetAttr looks like it's failing? I debugged this and it looks like
> it's getting the width and height right from the temporary <img> element. Though
> it took me some head-scratching to see that it was working fine, the new
> stringclasses are kind'a tricky in that they can contain an inline buffer
> without using it, so sometimes it looks like the value is an empty string,
> whereas it has a set value.

That's the problem: the return value - ie the value of the attribute - is the
empty string instead of the set value. You probably touched the problem.
Are you *sure* you are getting the empty string back? As I said the new
stringclasses can look empty when they really do contain a real value. You need
to check the |mData| member and nothing else. I fell into this trap at first too.
*** Bug 241284 has been marked as a duplicate of this bug. ***
Is bug 244834 related to this one?
(In reply to comment #14)
> Is bug 244834 related to this one?

Possibly. But, I think this bug (formatting PICTURE size) has been fixed. I
reported it a while back, and I don't recall encountering it in 1.7RC1 or RC2
Did you try to reproduce it? I can reproduce it with 1.7RC2 on Mac.
(In reply to comment #16)
> Did you try to reproduce it? I can reproduce it with 1.7RC2 on Mac.

I do beg your pardon. The bug IS still there. I recall testing a recent Mozilla
release from which the bug was absent. It was clearly not RC2, even though I
thought it was. 

I was unaware, however, of how nicely the resize feature works when one uses any
one of the image's handles. So, resizing is possible, just not through any of
the menus. 

Payam
I checked it yesterday and the problem is still there...
Mozilla 1.7RC2 on Win98SE...
Attached file Testcase (HTML) — 
Here is a testcase I came up with to illustrate the problem discussed in this
bug. To run the testcase, just try and resize every copy of the image in the
attached HTML page in the Mozilla editor to a larger size (200w x 200h). I will
attach the image to this bug.

Here are the results I found:
1 - Image resized properly, however subsequent attempts to resize the image
failed.
2 - Only the image height was resized - the existing CSS width attribute was
not modified.
3 - Image resized properly, however subsequent attempts to resize the image
failed.
4 - Neither width or height were resized properly.
5 - Image resized properly, however subsequent attempts to resize the image
failed.
6 - Neither width or height were resized properly.
7 - Neither width or height were resized properly.
8 - Neither width or height were resized properly.
9 - Neither width or height were resized properly.
10 - Neither width or height were resized properly.
11 - Neither width or height were resized properly.
Attached image Testcase (GIF) — 
This is the image which accompanies the HTML testcase.
*** Bug 249556 has been marked as a duplicate of this bug. ***
*** Bug 249799 has been marked as a duplicate of this bug. ***
I have the same problem with adapting the styles of a  table cell. It may be
related to bug 170436, but I seem to be getting it with a single cell as well.
This bug is still here in 1.7 final.

Proposing keywords: Regression, dataloss, useless-UI (even though the latter
seems to be intended for features that hadn't been implemented yet).

On a side note: is there a timeframe for merging the NVU expansion back into
Mozilla? I am asking because last time I checked, there was no NVU version for
Mac OSX.
This bug is only present, if "Use CSS styles instead of HTML elements and
attributes" is enabled in Composer prefernces.
OK, so here's what's going on.  The way the dialog is implemented, it creates a
clone of the image node it's working on, modifies the clone, then copies over
the attributes of the clone to the original node.  This last part is what's
breaking, as comment 6 says.

Say we start with <img src="foo" style="width: 10px; height: 10px">.  This is
the dest node.

After cloning we have <img src="foo" style="width: 10px; height: 10px">.  This
is the source node.

Now the dialog sets the width and height to 20px.  It sets the _attributes_.  So
now the source node is:

  <img src="foo" style="width: 10px; height: 10px" width="20px" height="20px">

Now we copy over the attributes by walking the attr node map and copying over
one at a time (after clearing the attrs on the dest node).  This is the place
where the width and height attributes are converted to CSS.  Now with sicking's
changes, the mapped attrs all come at the beginning of the node map, so the
process looks like this (listing what the dest node looks like after each step:

<img>
<img style="width: 20px">
<img style="width: 20px;  height: 20px">
<img style="width: 20px;  height: 20px" src="foo">
<img style="width: 10px;  height: 10px" src="foo">

where in the last step we have copied over the style attribute and hence
clobbered the width and height that came from HTML attributes.

The real problem here is that the code assumes that attributes in the node map
will be in the order they were set in.  That was never true; before sicking's
landing, the following image would not have been resizable with this dialog either:

  <img src="foo" width="40" height="40" style="width: 40px; height: 40px">

(since setting the width and height attrs would have just modified them in
place, then when copying over the attrs we would do them "in order", hence copy
over width, height, then style).

I'm really not sure what the right solution is here, partly because I do not
quite understand how this UI works.  Is it safe to assume that all the relevant
attributes will be set every time the dialog is closed?  That is, if I have:

  <img src="foo" width="40" height="40" style="width: 20px; height: 20px">

and I edit its properties, will the clone end up as:

  <img src="foo" width="20" height="20" style="width: 20px; height: 20px">

?  Or what?  If so, we could copy over the style attr first and then the other
ones, so they will override... but what about:

  <img src="foo" width="40" height="40" style="width: 20em; height: 20em">

?

The real "right" solution, may be, is that instead of simply setting the
attributes on the clone the dialog should be calling setAttributeOrEquivalent.
Attached patch Proposed patch — — Splinter Review 
This fixes the image properties dialog for me on the attached testcase.
You rock bz! Thanks for hunting this down!

I can see two solutions to this:

1. Do what bz says and set attributes on the clone through
   setAttributeOrEquivalent. Note that we'd also need to also unset any
   html-attributes at the same time. I.e. when setting .style.height = "10px" we
   should also unset the height attribute so that it doesn't bite us when the
   attributes are copied from the clone.

2. Always make sure to remove any properties from the style attribute when
   setting the html-attribute on the clone. I.e. when we set the
   height-attribute on the clone we also unset .style.height.

Of course, the editor people should decide what to do.
Comment on attachment 153065 [details] [diff] [review]
Proposed patch

Daniel, Peter, could you review?
Attachment #153065 - Flags: superreview?(peterv)
Attachment #153065 - Flags: review?(daniel)
> Note that we'd also need to also unset any html-attributes at the same time.

setAttributeOrEquivalent already does this (if it finds a CSS equivalent, it
unsets the HTML attr in addition to setting the CSS).
Attachment #153065 - Flags: superreview?(peterv) → superreview+
Comment on attachment 153065 [details] [diff] [review]
Proposed patch

r=daniel@glazman.org
Well done Boris. At the time I wrote that code, what you just did was not
possible because of inline styles limitations when the element was not
physically in the document instance (that is the case for globalElement).
Attachment #153065 - Flags: review?(daniel) → review+
Assignee: daniel → bzbarsky
Target Milestone: mozilla1.8alpha1 → mozilla1.8alpha3
Fixed on trunk.
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Isn't there a lot of other dialogs that are broken too?
There are other dialogs that suffered from the same problem. But if I read the
discussion correctly these may also have been fixed. I'll see when the next
1.8a3 is released :). If not, should this bug be reopened or a new one filed?
One bug per dialog, with the steps to reproduce relevant to those dialogs.  I
fixed the one dialog that had steps to reproduce the problem in this bug.
As far as I recall, bug 170436 deals with similar issues on another dialog.
Thanks for the good work on this one.
*** Bug 271285 has been marked as a duplicate of this bug. ***
Product: Browser → Seamonkey
Blocks: 243890
As first time user this bug popped up immediately in the stable release 1.7.13
Changes in the source work fine though. May be the meaning of stable is that bugs will reproduce faithfully even after several years.
Right.  As you might have noticed, the target milestone for this bug is 1.8 alpha 3.  That's when it was fixed.  So in a 1.7 build you'll see the bug.

I strongly urge you to upgrade, since security fixes are no longer being made for the 1.7 branch.
Thank you, Boris,

Still I'm confused: the 1.7 release is advertised on the Mozilla site as THE stable release, and for the dare-devils there is 1.8, correct?
Being a ranking amateur, would you advise me to go the 1.8 route and hope to survive?


Um...  That _was_ the state of things before August of 2005.  ;)

The 1.7 release of the Mozilla Application Suite is the last release supported by the Mozilla Corporation.  Support for it ended sometime in fall of 2006.  I don't see any links to it from http://www.mozilla.org/, which is good!

There is an application suite based on the Gecko 1.8+ code that's called Seamonkey.  It's linked off of the Mozilla site.  Seamonkey 1.1 (which is what's linked) is the equivalent of Firefox 2 in terms of stability and is based on Gecko 1.8.1.

So I advise you to download Seamonkey 1.1.  If you're really paranoid, you could do Seamonkey 1.0.7 (based on Gecko 1.8.0), linked from http://www.mozilla.org/projects/seamonkey/releases/.  But I really do think Seamonkey 1.1 is a better idea.

At this point the dare-devils thing would be builds based on Gecko 1.9.  :)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: