47066 - Resize of elements with mouse

Status: VERIFIED FIXED

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

Description

[Kathleen :Brade]

I want to be able to resize an image, hrule, table (columns, rows) by grabbing 
some "handles" and stretching/shrinking it.

Comment 1

[rubydoo123]

moving to future

Comment 2

[Kathleen :Brade]

*** Bug 67266 has been marked as a duplicate of this bug. ***

Comment 3

[Kathleen :Brade]

-->brade

Comment 4

[Syd Logan]

spam composer change

Comment 5

[Kathleen :Brade]

nominate this bug since this is 4.x parity

Comment 6

[Syd Logan]

nsbeta1- per buffy triage

Comment 7

[Syd Logan]

EDITORBASE- per editorbase triage

Comment 8

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

*** Bug 172236 has been marked as a duplicate of this bug. ***

Comment 9

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

Getting nominations after duping of 172236.
Taking ownership.

Comment 10

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

Attachment: work in progress #1

shows the resize handlers but they're not active yet

Comment 11

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

Attachment: screenshot : 8 resize handles, half opaque real-time resizing

Comment 12

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

Attachment: work in progress #2

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.

Comment 13

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

Attachment: work in progress #3

- now handles resizing of the viewport/window
- resizing is now undoable/redoable
- comments added to the code + cleanup

pending : preserve ratio on corner handles

Comment 14

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

Attachment: work in progress #4

this is mostly done; expect a final patch for reviews tomorrow

Comment 15

[Kathleen :Brade]

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?

Comment 16

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

Attachment: fix #1

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

Comment 17

[Charles Manske]

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!

Comment 18

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

Attachment: fix #2, needing reviews

> 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...

Comment 19

[Simon Fraser [no longer active]]

> > 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.

Comment 20

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

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.

Comment 21

[Charles Manske]

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?

Comment 22

[kinmoz]

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

Comment 23

[Kevin McCluskey (gone)]

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.

Comment 24

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

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.

Comment 25

[Kathleen :Brade]

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.

Comment 26

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

Attachment: c++ work in progress #1

Shows the resizing handles when the selection consists of a single image
element
and hides them when needed.

Comment 27

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

Attachment: c++ work in progress #2

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.

Comment 28

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

Attachment: FINAL PATCH #1

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 :-)

Comment 29

[Jan Varga [:janv]]

I just applied this patch on linux and it works great.
Good work!

Comment 30

[Kathleen :Brade]

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.

Comment 31

[Kathleen :Brade]

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.

Comment 32

[kinmoz]

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;}
+

Comment 33

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

> 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 34

[kinmoz]

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.

Comment 35

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

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.

Comment 36

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

Attachment: FINAL PATCH #2, carrying forward r=brade and se=kin

This patch answers to the comments made by Kathy and Kin.

Comment 37

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

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

Comment 38

[Asa Dotzler [:asa]]

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.

Comment 39

[(not reading, please use [e-mail] instead)]

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;

Comment 40

[Kathleen :Brade]

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.

Comment 41

[Samir Gehani]

Composer triage team: -> Editor: Core nsbeta1+/adt2

Comment 42

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

Deletion issue fixed in my tree. Working on the two others.

Comment 43

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

Table resizing and table celle resizing added to my tree.
Layout issues with HR resizing.
Working on table column resizing.

Comment 44

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

Attachment: final final patch, in answer to all last comments

carries forward r=brade, sr=kin

Comment 45

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

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.

Comment 46

[Asa Dotzler [:asa]]

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.

Comment 47

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

sigh

Comment 48

[rubydoo123]

as the saying goes: another one bites the dust, and another one's gone, another
one's gone ... don't you love it

Comment 49

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

Finally.... checked in.

Comment 50

[rubydoo123]

grippy is available on images and tables, using the trunk build from 2003040308