Resize of elements with mouse

VERIFIED FIXED in mozilla1.4alpha

Status

()

Core
Editor
P3
normal
VERIFIED FIXED
17 years ago
14 years ago

People

(Reporter: Kathleen Brade, Assigned: glazou)

Tracking

({helpwanted, polish, topembed+})

Trunk
mozilla1.4alpha
helpwanted, polish, topembed+
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [adt2] EDITORBASE+ edt_x3)

Attachments

(2 attachments, 10 obsolete attachments)

66.62 KB, image/gif
Details
78.21 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

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

Comment 1

17 years ago
moving to future
Status: NEW → ASSIGNED
Target Milestone: --- → Future
(Reporter)

Comment 2

17 years ago
*** Bug 67266 has been marked as a duplicate of this bug. ***
(Reporter)

Comment 3

16 years ago
-->brade
Assignee: beppe → brade
Status: ASSIGNED → NEW
Keywords: 4xp, helpwanted

Comment 4

16 years ago
spam composer change
Component: Editor: Core → Editor: Composer
(Reporter)

Comment 5

15 years ago
nominate this bug since this is 4.x parity
Keywords: nsbeta1
Whiteboard: EDITORBASE

Comment 6

15 years ago
nsbeta1- per buffy triage

Comment 7

15 years ago
EDITORBASE- per editorbase triage
Whiteboard: EDITORBASE → EDITORBASE-
(Reporter)

Updated

15 years ago
Blocks: 172236
(Assignee)

Comment 8

15 years ago
*** Bug 172236 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 9

15 years ago
Getting nominations after duping of 172236.
Taking ownership.
Assignee: brade → glazman
No longer blocks: 172236
Keywords: polish, topembed+
Whiteboard: EDITORBASE- → EDITORBASE
(Assignee)

Comment 10

15 years ago
Created attachment 109974 [details] [diff] [review]
work in progress #1

shows the resize handlers but they're not active yet
(Assignee)

Comment 11

15 years ago
Created attachment 110005 [details]
screenshot : 8 resize handles, half opaque real-time resizing
(Assignee)

Comment 12

15 years ago
Created attachment 110006 [details] [diff] [review]
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.
(Assignee)

Updated

15 years ago
Attachment #109974 - Attachment is obsolete: true
(Assignee)

Comment 13

15 years ago
Created attachment 110189 [details] [diff] [review]
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
Attachment #110006 - Attachment is obsolete: true
(Assignee)

Comment 14

15 years ago
Created attachment 110778 [details] [diff] [review]
work in progress #4

this is mostly done; expect a final patch for reviews tomorrow
Attachment #110189 - Attachment is obsolete: true
(Reporter)

Comment 15

15 years ago
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?
(Assignee)

Comment 16

15 years ago
Created attachment 110871 [details] [diff] [review]
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
Attachment #110778 - Attachment is obsolete: true

Comment 17

15 years ago
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!
(Assignee)

Comment 18

15 years ago
Created attachment 110943 [details] [diff] [review]
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...
(Assignee)

Updated

15 years ago
Attachment #110943 - Flags: superreview?(kin)
Attachment #110943 - Flags: review?(cmanske)

Comment 19

15 years ago
> > 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.
(Assignee)

Comment 20

15 years ago
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 21

15 years ago
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+

Comment 22

15 years ago
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.

Updated

15 years ago
Attachment #110943 - Flags: superreview?(kin) → superreview-
(Assignee)

Comment 24

15 years ago
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.
(Reporter)

Comment 25

15 years ago
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
(Assignee)

Updated

15 years ago
Attachment #110871 - Attachment is obsolete: true
(Assignee)

Comment 26

15 years ago
Created attachment 111395 [details] [diff] [review]
c++ work in progress #1

Shows the resizing handles when the selection consists of a single image
element
and hides them when needed.
Attachment #110943 - Attachment is obsolete: true
(Assignee)

Comment 27

15 years ago
Created attachment 112053 [details] [diff] [review]
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.
Attachment #111395 - Attachment is obsolete: true
(Assignee)

Comment 28

15 years ago
Created attachment 112288 [details] [diff] [review]
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 :-)
Attachment #112053 - Attachment is obsolete: true

Comment 29

15 years ago
I just applied this patch on linux and it works great.
Good work!
(Reporter)

Comment 30

15 years ago
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)
(Reporter)

Comment 31

15 years ago
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 32

15 years ago
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;}
+
(Assignee)

Comment 33

15 years ago
> 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

15 years ago
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+
(Assignee)

Comment 35

15 years ago
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.
(Assignee)

Comment 36

15 years ago
Created attachment 112402 [details] [diff] [review]
FINAL PATCH #2, carrying forward r=brade and se=kin

This patch answers to the comments made by Kathy and Kin.
Attachment #112288 - Attachment is obsolete: true
(Assignee)

Comment 37

15 years ago
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 38

15 years ago
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;
(Reporter)

Comment 40

15 years ago
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

15 years ago
Composer triage team: -> Editor: Core nsbeta1+/adt2
Component: Editor: Composer → Editor: Core
Keywords: nsbeta1 → nsbeta1+
Whiteboard: EDITORBASE+ → [adt2] EDITORBASE+
(Assignee)

Comment 42

15 years ago
Deletion issue fixed in my tree. Working on the two others.
(Assignee)

Comment 43

15 years ago
Table resizing and table celle resizing added to my tree.
Layout issues with HR resizing.
Working on table column resizing.
(Assignee)

Comment 44

15 years ago
Created attachment 114102 [details] [diff] [review]
final final patch, in answer to all last comments

carries forward r=brade, sr=kin
Attachment #112402 - Attachment is obsolete: true

Updated

15 years ago
QA Contact: sujay → beppe
(Assignee)

Comment 45

15 years ago
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 46

15 years ago
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-
(Assignee)

Comment 47

15 years ago
sigh
Target Milestone: mozilla1.3beta → mozilla1.4alpha

Comment 48

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

Updated

15 years ago
Whiteboard: [adt2] EDITORBASE+ → [adt2] EDITORBASE+ edt_x3
(Assignee)

Comment 49

15 years ago
Finally.... checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED

Comment 50

14 years ago
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.