Closed Bug 47066 Opened 24 years ago Closed 21 years ago

Resize of elements with mouse

Categories

(Core :: DOM: Editor, defect, P3)

Product:
Core
Component:
DOM: Editor
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla1.4alpha

People

(Reporter: Brade, Assigned: glazou)

References

Details

(Keywords: helpwanted, polish, topembed+, Whiteboard: [adt2] EDITORBASE+ edt_x3)

Attachments

(2 files, 10 obsolete files)

screenshot : 8 resize handles, half opaque real-time resizing
66.62 KB, image/gif
Details
final final patch, in answer to all last comments
78.21 KB, patch
asa
: approval1.3-
Details | Diff | Splinter Review 
I want to be able to resize an image, hrule, table (columns, rows) by grabbing 
some "handles" and stretching/shrinking it.
moving to future
Status: NEW → ASSIGNED
Target Milestone: --- → Future
*** Bug 67266 has been marked as a duplicate of this bug. ***
-->brade
Assignee: beppe → brade
Status: ASSIGNED → NEW
Keywords: 4xp, helpwanted
spam composer change
Component: Editor: Core → Editor: Composer
nominate this bug since this is 4.x parity
Keywords: nsbeta1
Whiteboard: EDITORBASE
nsbeta1- per buffy triage
EDITORBASE- per editorbase triage
Whiteboard: EDITORBASE → EDITORBASE-
Blocks: 172236
*** Bug 172236 has been marked as a duplicate of this bug. ***
Getting nominations after duping of 172236.
Taking ownership.
Assignee: brade → glazman
No longer blocks: 172236
Keywords: polish, topembed+
Whiteboard: EDITORBASE- → EDITORBASE
Attached patch work in progress #1 (obsolete) — — Splinter Review 
shows the resize handlers but they're not active yet
Attached patch work in progress #2 (obsolete) — — Splinter Review 
This is almost a final version...

What it does :

  - display 8 resize handles when an image or a table is selected
  - allows to resize these two types of element using the mouse
  - if an image is resized, an half-opaque version of the resized image is
    shown real-time during mouse move
  - the handles are anonymous content, ie they are never saved even if they
    are visible when the user saves the document
  - This is done 99% through XBL. There is not a single line of c++ here.
    And that's damn fast.

What it doesn't **for the moment**:

  1 preserve a correct position of the handles when the width of the viewport
    is modified and the resized image's or table's position changes
  2 allow to resize table cells, rows, columns, and horizontal rules.
  3 preserve the ration when a corner handle is used
  4 allow to undo the resizing

1 3 and 4 are mandatory; 2 is optional and is future work.

Notes :

  1 Dreamweaver MX only offers THREE resize handles! YAY.
  2 Outlook does not allow to see the resized image real-time! YAY AGAIN.
Attachment #109974 - Attachment is obsolete: true
Attached patch work in progress #3 (obsolete) — — Splinter Review 
- now handles resizing of the viewport/window
- resizing is now undoable/redoable
- comments added to the code + cleanup

pending : preserve ratio on corner handles
Attachment #110006 - Attachment is obsolete: true
Attached patch work in progress #4 (obsolete) — — Splinter Review 
this is mostly done; expect a final patch for reviews tomorrow
Attachment #110189 - Attachment is obsolete: true
Comment on attachment 110778 [details] [diff] [review]
work in progress #4

>Index: xpfe/global/resources/content/bindings/editor.xml
>===================================================================
>@@ -45,6 +45,34 @@
>         ]]>
>         </body>
>       </method>
>+      <method name="setHTMLElementSize">

Do we need a try/catch around this to ensure we don't throw on an error between
beginTransaction and endTransaction calls?


...
>+            // we need to explicitely set the style attribute because

--> explicitly


>Index: mailnews/compose/resources/content/messengercompose.xul
>===================================================================
>@@ -487,9 +487,9 @@
>             onkeypress="editorKeyPress(event);" context="msgComposeContext"/>
>   </vbox>
> 
>-  <statusbar id="status-bar" class="chromeclass-status">
>+  <statusbar id="status-bar" class="chromeclass-status" >

The above change seems unnecessary



>Index: editor/ui/composer/content/editor.js
>===================================================================
>@@ -477,8 +478,14 @@
>   gContentWindow = window.content;
> 
>   // Set up the mime type and register the commands.
>-  if (IsHTMLEditor)
>+  if (IsHTMLEditor) {

I prefer putting '{' on a new line (which I believe is more consistent in this
file)


>+    // HACK...force our XBL bindings file to be load before we try to create our first xbl widget....

--> loaded
make sure it stays < 80 characters per line


>@@ -3036,6 +3043,29 @@
> 
>   if (!element) return;
> 
>+  var tag = element.nodeName.toLowerCase();

move this block down after we know we have a bodyelement


>Index: editor/ui/composer/content/resizableImage.xml
>===================================================================
Should this file be renamed?  Isn't the goal for it to handle resizing of more
html elements than images (tables and hrules)?


>+      <handler event="mouseup">
...
>+      <handler event="mousemove" phase="capturing">
...

Tiny nit: in a previous file, didn't you add these handlers in a different
order?


>+      <method name="HideImageResizers">
>+        <body>
>+        <![CDATA[
>+          // hide the height handles
height?  Do you mean width and height?
Attached patch fix #1 (obsolete) — — Splinter Review 
Final patch, ***ready for reviews***

A few hints :

1) Neil and I have detected that the XBL constructor is not always called when
   a mailcompose window is opened; this explains the explicit 
   bodyelement.Initialize() in editor.js
2) the fix does NOT work in mailcompose if JS is disabled in mail/news and
there
   is absolutely nothing I can do against that. We have somewhere in bugzilla
   a bug about allowing JS if it comes from chrome JS but it's not done yet.
3) I get layout assertions when I select an image. I don't understand these
   assertions and I think they are more relevant to bugs in
nsCSSFrameConstructor
   than bugs in my code
Attachment #110778 - Attachment is obsolete: true
Comment on attachment 110871 [details] [diff] [review]
fix #1

Behavior comments:
1. I get a large number of JS warnings:
"line 6: reference to undefined property bodyelement.resizing"
just moving the mouse around a blank page. I tried changing all
instances of "bodyelement.resizing" to something like:
   if ("resizing" in bodyelement && !bodyelement.resizing)
but that didn't seem to help.

2. Is the reflow supposed to be realtime? It isn't in my testing.
(Actually, I'm not so sure live reflow is a good idea!)

3. There's a problem if you drag a handle out of the editing window.
When you return, the mouse is locked in "drag mode" even if you release
the mouse button.

Code comments:
1. Use the same capitalization style for all methods in the XBL:
First letter should be lowercase (yes, I know I've broken that
rule in Composer!) Examples where you used upper case:
    Initialize, PrepareImageResize, HideImageResizers, ResizeShadow...

2. I don't think new blank lines are needed in MsgComposeCommands.js:
+
+      case "cmd_updateStructToolbar":

3. Thanks for the comments in XBL code! Not many contributors do that.

Otherwise, the code looks ok.
Very cool feature!
Attached patch fix #2, needing reviews (obsolete) — — Splinter Review 
> 1. I get a large number of JS warnings:
> "line 6: reference to undefined property bodyelement.resizing"
> just moving the mouse around a blank page.

I don't see those myself (strange isn't it?) but I understand
where they can come from.
Fixed.

> 2. Is the reflow supposed to be realtime? It isn't in my testing.
> (Actually, I'm not so sure live reflow is a good idea!)

When an image is resized, there is a semi-opaque shadow of
it which is resized real-time.

> 3. There's a problem if you drag a handle out of the editing window.
> When you return, the mouse is locked in "drag mode" even if you release
> the mouse button.

I'm afraid there is nothing I can do against that. Make the
mouse pointer enter again the editor zone and click on the button.

> 1. Use the same capitalization style for all methods in the XBL

Fixed.

> 2. I don't think new blank lines are needed in MsgComposeCommands.js

Fixed.

> 3. Thanks for the comments in XBL code! Not many contributors do that
> Otherwise, the code looks ok. Very cool feature!

:-) my pleasure...
Attachment #110943 - Flags: superreview?(kin)
Attachment #110943 - Flags: review?(cmanske)
> > 3. There's a problem if you drag a handle out of the editing window.
> > When you return, the mouse is locked in "drag mode" even if you release
> > the mouse button.
> 
> I'm afraid there is nothing I can do against that. Make the
> mouse pointer enter again the editor zone and click on the button.

This is a long-standing flaw in our event handling. We _have_ to fix this issue.
Simon: from my perspective, a good way of solving it is adding the button
information to the event in case of a "mousemove", "mouseover" and "mouseout"
event. It is not filled for the time being.
Status: NEW → ASSIGNED
Comment on attachment 110943 [details] [diff] [review]
fix #2, needing reviews

That assert in nsCSSFrameConstructor::AttributeChanged sure is annoying! Could
you please look into it or file a bug on the issue?
Attachment #110943 - Flags: review?(cmanske) → review+
FYI: According to glazman on IRC, the layout assertion being triggered is:

###!!! ASSERTION: parent frame but no child frame or undisplayed entry:
'!parentFrame', file c:/mozilla/layout/html/style/src/nsCSSFrameConstructor.cpp,
line 10664
Sorry, but we should not land the current patch until it also satisfies
embeddors requirements. We will need a solution that works both for embeddors
and Mozilla.
Attachment #110943 - Flags: superreview?(kin) → superreview-
Believe me, that emoticon is not here for nothing :-(

It _could_ be helpful if next time the embeddors give their requirements
**BEFORE** the implementation. That is a *very* bad surprise.
EB+ per yesterday's meeting; this is expected behavior of an editor.
Reset milestone to 1.3beta since Daniel is actively working on a fix for this.
Whiteboard: EDITORBASE → EDITORBASE+
Target Milestone: Future → mozilla1.3beta
Attachment #110871 - Attachment is obsolete: true
Attached patch c++ work in progress #1 (obsolete) — — Splinter Review 
Shows the resizing handles when the selection consists of a single image
element
and hides them when needed.
Attachment #110943 - Attachment is obsolete: true
Attached patch c++ work in progress #2 (obsolete) — — Splinter Review 
This is almost it. What remains on my todo list :

1. a small issue with html mail compose
2. add a EnableImageResizing() method so embeddors can decide to
   turn it on/off
3. code cleanup and optimization, there's a lot to do but that's fast
4. add comments to the code

The whole thing is a question of hours.
Attachment #111395 - Attachment is obsolete: true
Attached patch FINAL PATCH #1 (obsolete) — — Splinter Review 
Your dreams made real. Lovely in composer and mail composition.
Honeymoon with Undo/Redo. In love with window resizing. Tolerant with
any attribute change you make.
Shows semi-opaque shadow of the resized image. Will think of 3d version
for next embedded version if I am still alive.

Save it. Apply it. Use it. Love it. And let me sleep a bit while you review
the patch. Thanks :-)
Attachment #112053 - Attachment is obsolete: true
I just applied this patch on linux and it works great.
Good work!

Comment on attachment 112288 [details] [diff] [review]
FINAL PATCH #1

r=brade
Please rename files/methods to not use "Image" since we want to resize hrules
and potentially other objects.	Fix copyright year to 2002 or 2003 (depending
when you worked on it?).  Add some error checking we discussed on irc.	Fix the
copy/paste error for width/height in SetFinalSize and use named literals here
too.  In nsHTMLEditor.h, use PRPackedBool.
Attachment #112288 - Flags: superreview?(kin)
Comment on attachment 112288 [details] [diff] [review]
FINAL PATCH #1

r=brade
Please rename files/methods to not use "Image" since we want to resize hrules
and potentially other objects.	Fix copyright year to 2002 or 2003 (depending
when you worked on it?).  Add some error checking we discussed on irc.	Fix the
copy/paste error for width/height in SetFinalSize and use named literals here
too.  In nsHTMLEditor.h, use PRPackedBool.
Attachment #112288 - Flags: review+
Comment on attachment 112288 [details] [diff] [review]
FINAL PATCH #1

WOW! Just some comments/questions other than the copy/paste problem brade found
above:

==== I don't like the fact that we're adding |CheckImageResizing()| code to the
generic CSS inline txn. Isn't there a way to call this by plugging into the
TransactionListener interface? Or by calling this stuff at the point that
creates and triggers these transactions?


==== Not a big deal, but just wondering why not use "tl", "t", "tr", "l", "r",
"bl", "b", "br" to be consistent with enums/defines used everywhere? I know the
values you currently use map directly to the "cursor" property used in
EditorOverride.css so like I said it's not a big deal.


+#define kTopLeft	NS_LITERAL_STRING("nw")
+#define kTop		NS_LITERAL_STRING("n")
+#define kTopRight	NS_LITERAL_STRING("ne")
+#define kLeft		NS_LITERAL_STRING("w")
+#define kRight 	NS_LITERAL_STRING("e")
+#define kBottomLeft	NS_LITERAL_STRING("sw")
+#define kBottom	NS_LITERAL_STRING("s")
+#define kBottomRight	NS_LITERAL_STRING("se")


==== Why define these QI impls manually rather then use the XPCOM macros?


+ResizeEventListener::QueryInterface(REFNSIID aIID, void** aInstancePtr)

+ResizerSelectionListener::QueryInterface(REFNSIID aIID, void** aInstancePtr)

+ResizerMutationListener::QueryInterface(REFNSIID aIID, void** aInstancePtr)

+ResizerMouseMotionListener::QueryInterface(REFNSIID aIID, void** aInstancePtr)


==== In |nsHTMLEditor::CreateResizer()| there is no need to check |res| again
after the QI because it was already checked above the QI, unless you meant to
call |do_QueryInterface(newContent, &res)|?


+  nsCOMPtr<nsIContent> newContent;
+  nsresult res = CreateHTMLContent(NS_LITERAL_STRING("span"),
getter_AddRefs(newContent));
+  if (NS_FAILED(res)) return res;
+
+  nsCOMPtr<nsIDOMElement> newElement = do_QueryInterface(newContent);
+  if (NS_FAILED(res) || !newElement)
+    return NS_ERROR_FAILURE;


==== Same thing in |nsHTMLEditor::CreateShadow()|.


==== In |nsHTMLEditor::HideResizers()| do we really want to bail early if one
of the listener removals fail? Shouldn't we just assert or print a
warning/error and continue and try to remove as many as we can?



==== In |nsHTMLEditor::SetShadowPosition()| did you forget to get the |res=|
for this? or was this on purpose?


+  mResizingShadow->SetAttribute(NS_LITERAL_STRING("anonclass"),
+				 NS_LITERAL_STRING("mozResizingShadow"));
+  return res;


==== You can use |NS_IF_ADDREF(*aResizedObject)| here:


+  if (mResizedObject)
+    NS_ADDREF(*aResizedObject);


==== |mSelectionListenerP| is a comptr so no need to explicitly initialize it
right?


 , mIgnoreSpuriousDragEvent(PR_FALSE)
 , mTypeInState(nsnull)
+, mSelectionListenerP(nsnull)
 , mSelectedCellIndex(0)


==== The error returned here should be NS_ERROR_OUT_OF_MEMORY, but I understand
that everything else in this method that does a |new| returns
NS_ERROR_NULL_POINTER.


+    // init the selection listener for image resizing
+    mSelectionListenerP = new ResizerSelectionListener(this);
+    if (!mSelectionListenerP) {return NS_ERROR_NULL_POINTER;}
+
> WOW!

:-)

> ==== I don't like the fact that we're adding |CheckImageResizing()| code
> to the generic CSS inline txn. Isn't there a way to call this by plugging
> into the TransactionListener interface? Or by calling this stuff at the
> point that creates and triggers these transactions?

Ok, I understand. Fixing.

> ==== Not a big deal, but just wondering why not use "tl", "t", "tr", "l",
> "r", "bl", "b", "br" to be consistent with enums/defines used everywhere?
> I know the values you currently use map directly to the "cursor" property
> used in EditorOverride.css so like I said it's not a big deal.

Will make all based on n s e and w.

Other comments: ok to all.
Comment on attachment 112288 [details] [diff] [review]
FINAL PATCH #1

FYI, I'm giving a conditional sr=kin on this patch as long as the issues
brought up are addressed.

glazman, jfrancis and I discussed the transaction problem mentioned above and
suggested trying the route of enabling/disabling the image handles in
Begin/EndUpdateViewBatch() ... which enables/disables selection, reflow and
painting before edit operations. If for some reason that doesn't pan out,
glazman can check in the txn changes with the promise that in the future, when
style attribute change events/listeners are enabled, that he remove this hack.
Attachment #112288 - Flags: superreview?(kin) → superreview+
Kin: your suggested proposal is not easy to do. We always call Do() through
nsEditor and I don't want to add knowledge of an HTMLEditor-only thing to
the base class. I tried two another solutions but none of them was solving all
the problems.
This patch answers to the comments made by Kathy and Kin.
Attachment #112288 - Attachment is obsolete: true
Comment on attachment 112402 [details] [diff] [review]
FINAL PATCH #2, carrying forward r=brade and se=kin

Trunk approval request for bug 47066

The proposed patch adds resizing handles to selected images in HTML editors
(Composer and mail composition). It is then possible to use the mouse to resize
an image with realtime semi-opaque shadow of the target resized image. A
"tooltip" shows the target size and relative changes in pixels. The feature can
be disabled if an embedding client does not wish to allow the feature.

It must be noted that OE does not offer such a level of feature, having only 3
handles with no shadow.

The bug is an urgent topembed+, editorbase+ and should be nsbeta1+'ed in the
coming days. It has r=brade and sr=kin. The bug has been extensively tested by
myself on Windows and Linux, and tested by Jan Varga on Linux. It is
Undo/Redo-safe, window-resize-safe, css-mode-safe.

From my perspective, the patch is harmless.

A screenshot of Composer doing such an image resize can be found at
http://daniel.glazman.free.fr/tmp/demo2.jpg
Attachment #112402 - Flags: approval1.3b?
Comment on attachment 112402 [details] [diff] [review]
FINAL PATCH #2, carrying forward r=brade and se=kin

We're frozen for beta. This is a large new feature and without Alpha milestone
testing and cleanup and bugfixing in Beta it shouldn't be in 1.3final.
Attachment #112402 - Flags: approval1.3b? → approval1.3b-
just watching from the sidelines.

here's some supplimental review comments:

1)

+nsHTMLEditorMouseListener::MouseUp(nsIDOMEvent* aMouseEvent)
+{
+  NS_ENSURE_TRUE(aMouseEvent, NS_ERROR_NULL_POINTER);

consider:

NS_ENSURE_ARG_POINTER(aMouseEvent);

2)

+  *aResizedObject = mResizedObject;
+  NS_IF_ADDREF(*aResizedObject);

consider:

+  NS_IF_ADDREF(*aResizedObject = mResizedObject);

3)

+  NS_ENSURE_TRUE(aSelection, NS_ERROR_NULL_POINTER);

consider:

NS_ENSURE_ARG_POINTER(aSelection);

4)

+  w.AppendInt(width, 10);
+  h.AppendInt(height, 10);

and

+    x.AppendInt(GetNewResizingX(clientX, clientY), 10);
+    y.AppendInt(GetNewResizingY(clientX, clientY), 10);
+    w.AppendInt(newWidth, 10);
+    h.AppendInt(newHeight, 10);

10 is the default radix, so you can just do:

+  w.AppendInt(width);
+  h.AppendInt(height);

5)

+  return (resized > max) ? max : resized;
+  return (resized > 0) ? resized : 0;

isn't that the same as:

PR_MIN(max,resize);
PR_MAX(resized, 0);

6)

is it ever possible for mResizedObjectHeight or mResizedObjectWidth to be zero?

I'm worried about this code crashing:

+  float objectSizeRatio = 
+              ((float)mResizedObjectWidth) / ((float)mResizedObjectHeight);


+        result /=  objectSizeRatio;

a couple observations after playing with this patch:
  * all platforms: pressing delete with the image selected leaves handles until
you click in Composer window
  * Windows: I could resize to width=0px; perhaps we should limit it to minimum
of 1?  (Note: I did not crash and I did not test macho)
  * Macho: when resizing to be smaller, the image hilite color (blue in my case)
leaves turds on the screen (I don't see this on Windows); the turds do not clean
up when forcing a redraw in obvious ways.
Composer triage team: -> Editor: Core nsbeta1+/adt2
Component: Editor: Composer → Editor: Core
Keywords: nsbeta1 → nsbeta1+
Whiteboard: EDITORBASE+ → [adt2] EDITORBASE+
Deletion issue fixed in my tree. Working on the two others.
Table resizing and table celle resizing added to my tree.
Layout issues with HR resizing.
Working on table column resizing.
carries forward r=brade, sr=kin
Attachment #112402 - Attachment is obsolete: true
QA Contact: sujay → beppe
Comment on attachment 114102 [details] [diff] [review]
final final patch, in answer to all last comments

requesting approval for 1.3final.
Patch carries forward r and sr.
Extensively tested by myself on my windows and linux boxes, and by some others
(Jan, brade, ...) on various platforms including Macintosh.
Attachment #114102 - Flags: approval1.3?
Comment on attachment 114102 [details] [diff] [review]
final final patch, in answer to all last comments

This is the kind of feature work that needs to land in alpha, not final. Please
hold these changes until 1.4alpha opens.
Attachment #114102 - Flags: approval1.3? → approval1.3-
sigh
Target Milestone: mozilla1.3beta → mozilla1.4alpha
as the saying goes: another one bites the dust, and another one's gone, another
one's gone ... don't you love it
Whiteboard: [adt2] EDITORBASE+ → [adt2] EDITORBASE+ edt_x3
Finally.... checked in.
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
grippy is available on images and tables, using the trunk build from 2003040308
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.