Closed Bug 137092 Opened 22 years ago Closed 21 years ago

Absolute position support in Composer/editor core

Categories

(Core :: DOM: Editor, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla1.5alpha

People

(Reporter: thinkerdreamer, Assigned: glazou)

References

()

Details

Attachments

(3 files, 6 obsolete files)

Make it possible to move text, graphics, and links in resizable squares over 
the background image of the HTML page.  Wherever you put them let them stay.  
Thus there will be two layers.  The bottom will be the background and the top 
will be the text, graphics and links objects.  This will facilitate fast as 
well as easy WYSIWYG web page building.
me agrees with the high interest of the feature... Probably not too hard to do.
Basically the way I read it, this is a request for implementing Composer support
for the WYSIWYG creation and manipulation of absolute positioned DIV objects (or
floating boxes as GoLive calls them).

I agree that this would be a very cool feature to add to Composer; I am not
quite in agreement with Daniel's comment that this is "Probably not too hard to
do" but if it would be easy for him then more power to him.

Thanks to Gecko, Composer already properly renders pages with the type of DIV
objects that are needed, but the only way to modify them is by directly editing
the  HTML Source. So the first step would probably be to provide a means of
giving the DIV focus with the mouse in the WYSIWYG View.  Then a right click can
provide a context menu which gives access to a DIV properties panel where
attributes such as size, position, visible, etc can be changed.  The context
menu should also provide the option to delete the box (and everything in it?)
and that should be linked to the delete key when the box has focus.  Then you
would want to implement the drag-n-drop positioning and resizing of the box.

I would cast my vote for this bug for Composer but since it is currently
classified as Product = Browser, I can not (too many other more significant
issues with the Browser right now).
Well actually the way I see it is, drag the element and move it around keeping
track of mouse position. When the element is dropped the div tag is created with
the position and make the element child of that. Done :) So not that hard just
as what Daniel has said.
tseng_mike@yahoo.com -- Which element are you referring to?  

If you are referring to the DIV then as I stated, you first have to make it
possible to give the DIV focus before you can do anything with it.

If you are referring to an image or a section of text as the element then I
would have to disagree with automatically creating a DIV when the image or text
is dropped.  Absolute positioning is not the expected result of dragging and
dropping images and text.  Also using the the "simple" method described, every
time you moved an image, a new DIV would be created; this would certainly result
in too many useless DIV tags that would at best only unnecessarily increase the
HTML file size or at worst severly distort your intended layout.

Since DIV sections can contain a combination of all kinds of HTML tags including
other DIVs, you also can not just assume that when someone drags an image that
the a DIV surrounding it should be moved. So I do not see how to do it unless
the DIV element is able to receive focus.
I am refering to images and objects. This bug _is_ about the ability to absolute 
position objects as I understand the original request. It makes a bad html code, 
but thats what the bug is calling for. Now that I think about it, you don't even 
need div, just use stylesheet attributes.

You might be thinking abou floating boxes in GoLive as you mentioned, but I 
don't think this is what is asked for in this bug. Reporter just want to be able 
to place object anywhere he wants on the page if I'm not mistaken. Basically 
turn composer into something more like photoshop than a word processor.
The reason that I interpret it as using DIV instead of just CSS attributes for
images is because in Matt's initial long description, he says "in resizable
squares."  The only construct that I can think of that would correspond to one
of those squares, unless they actually have to be square,  ;)   would be DIV.

OS -> All

NEW
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 98 → All
Status: NEW → ASSIGNED
Target Milestone: --- → Future
HTML editors like Hotdog, CoffeCup, PageMill and FrontPage don't use drag and 
drop links, pictures and text.  Dreamweaver, GoLive, Fusion, Splash and 
Homestead have at least a little support for what is found in the editor Cool 
Page.  However, Cool Page is the prime example of this feature at its best. 
Therefore, Cool Page is the program that I have taken the idea of the feature 
from.  Download it at http://www.coolpage.com/cpg.html .  As you can see from 
the attachment there is a lot you can do with it in an incredibly fast and easy 
way.  It is great for the beginner or someone with little time. The attachment 
also shows that there are actual resizable boxes around each object.  This is 
crucial. It would be wierd any other way. You can also overlay objects on top 
of one another.  This comes in useful sometimes. I did the page in a few 
minutes so it is very fast.  The advanced user can also modify the code to fit 
his need.  
Mark, I reiterate my comment : not hard to do. Trust me.
Does that mean that we can expect a patch from you soon?
It means that I'll work on it as soon as I can. If this does not fall
into my official tasks basket at Netscape, I'll make it on my personal time.
Cool, sounds good to me.  With this feature and a few others (ie using the mouse
to dynamically resize images), Composer can become the best free WYSIWIG HTML
editor available -- and it will be cross platform!  
Keywords: helpwanted
Hardware: PC → All
Blocks: WYSIWYG
Summary: [RFE] Movable objects over layer of background image → Movable objects over layer of background image
reassign to new account
Assignee: syd → composer
Status: ASSIGNED → NEW
Taking. Nominating.
Assignee: composer → glazman
Keywords: helpwantednsbeta1, topembed
Priority: -- → P1
Summary: Movable objects over layer of background image → Absolute position support in Composer/editor core
Target Milestone: Future → mozilla1.3final
Spec URL attached (work in progress).
Status: NEW → ASSIGNED
Keywords: topembedtopembed+
-> Editor: Core per brade
Component: Editor: Composer → Editor: Core
==> QA = beppe
QA Contact: sujay → beppe
wontfix
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → WONTFIX
If you are going to close this as WONTFIX, can you at least explain why?
reopen bug
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Target Milestone: mozilla1.3final → Future
removing keywords
Keywords: nsbeta1, topembed+
Sorry, triage mistake on my part :-(
Patch coming.
Status: REOPENED → ASSIGNED
Target Milestone: Future → mozilla1.5alpha
To be uncompressed into mozilla/ui/composer/content/images
Attached patch patch (obsolete) — Splinter Review
This patch corresponds to all my additions to Composer.
Absolute positioning, better resizing, snap to grid, inline table editing.
Comment on attachment 123023 [details] [diff] [review]
patch

brade, r= ?
kin, sr= ?
Attachment #123023 - Flags: superreview?(kin)
Attachment #123023 - Flags: review?(brade)
Should we get usability feedback before landing these changes? I'm particularly
concerned about the inline table editing stuff.
Simon, yes I agree about the line table editing.
(one man's opinion:) Well, one way to get some usability testing is to get it
into builds and get people using it and reporting feedback. Glazman's putting
out test builds now and they're (so far) getting great feedback at mozillazine
and elsewhere. This isn't landing into 1.4 so it shouldn't be of immediate
concern for Netscape. Early in 1.5 seems like an ideal time to test out new
features by putting them into the hands of testers and soliciting feedback. 
If Netscape is willing to put resources into conducting usability studies then
I'm all for it.  If that's not gonna happen then I don't think we should not
take these changes. Tabbed browsing came to Mozilla long before Netscape did any
usability studies there and it was a hit :-)
Comment on attachment 123023 [details] [diff] [review]
patch

Kin being on vacation, can you review the patch, Simon?

Asa: just one important detail, all the features can be _fully_
enabled/disabled. If users don't like a feature after landing, we can still
disable it until the UI is fixed.
Attachment #123023 - Flags: superreview?(kin) → superreview?(sfraser)
Comment on attachment 123023 [details] [diff] [review]
patch

>Index: widget/public/nsILookAndFeel.h

>+    eMetric_HorizontalDragThreshold,                      // horizontal drag threshold
>+    eMetric_VerticalDragThreshold                         // vertical drag threshold
>   } nsMetricID;

How does this differ from eMetric_DragThreshold{X,Y}, which is implemented for
real on more platforms than this is, but needs to be stubbed out on the rest?

If there's a reason for the difference and you want to keep these new values,
then:
 * the comment should say what the difference is.
 * Are the units pixels?  If so, how about
   making the comments "// horizontal drag 
   threshold (pixels)", etc.
 * This list begins with the comment:
    // When modifying this list, also modify nsXPLookAndFeel::sIntPrefs
    // in widget/src/xpwidgts/nsXPLookAndFeel.cpp.
   Could you do that?
> How does this differ from eMetric_DragThreshold{X,Y} ?

It does not. Thanks for the pointer David. I was working on a branch where
this code was not present.
David: Speaking of drag threshold, I think that we do not conform to the
specification of what is the system drag threshold on Windows:

MSDN site says in
http://msdn.microsoft.com/library/default.asp?url=/library/en-us/sysinfo/base/getsystemmetrics.asp

  SM_CXDRAG, SM_CYDRAG 	Width and height of a rectangle centered on a drag
                        point to allow for limited movement of the mouse pointer
                        before a drag operation begins.

This means that the drag begins if the difference between the "gestureDownPoint"
and the current mouse position in X or Y is greater than the threshold minus 1
divided by 2...
But in
http://lxr.mozilla.org/seamonkey/source/content/events/src/nsEventStateManager.cpp#1465,
we have:

     // fire drag gesture if mouse has moved enough
     if (abs(aEvent->point.x - mGestureDownPoint.x) > thresholdX ||
         abs(aEvent->point.y - mGestureDownPoint.y) > thresholdY) {

and we don't divide by 2 here...
Comment on attachment 123023 [details] [diff] [review]
patch

in editorOverlay.xul, please leave the blank line in after cmd_align command.

Fix this block:
+  <toolbarbutton id="absolute-position-button" type="checkbox"
observes="cmd_absPos">
+    <observes element="cmd_absPos" type="checkbox" attribute="state"
onbroadcast="onStateButtonUpdate(this.parentNode, 'cmd_absPos', 'absolute')"/>
+  </toolbarbutton>

in EditorOverride.css, can you add a comment for the z-index values (5+
places)?

I'm not very found of "transparent" resizers.

Do we need moz_activated style?

The mozGrabber image should be a res url rather than chrome.  Also the table
images?

I don't think that you need the style for embed "mozCXUIPlugin".

Some of the names for the image files should be renamed to be consistent with
existing style:
  sendtoback-dsbl.gif --> sendtoback-disabled.gif
Should these images be added to themes or remain in editor/ui or ?

changes for these methods, +nsHTMLEditor::SetAbsolutelyPosition()
+nsHTMLEditor::RemoveAbsolutelyPosition()
 * my intuition would expect SetAbsolutelyPosition to take a param (z-index?).  
 * I also think it's more readable to be "SetAbsolutePosition" or
SetAbsolutePositioned" or "EnableAbsolutePositioning" or similar.
 * move the PRBools down to where they are used
 * add a blank space after the !selection line or somewhere in that chunk.

Can you reformat this line (make 2 lines or move resultNode earlier?):
+  nsCOMPtr<nsIDOMNode> node = do_QueryInterface(element), resultNode;

Does GetComputedProperty always clear "positionStr"; can it ever fail?

For this line
+  NS_IF_ADDREF(*_retval = element);
I've heard some super-reviewers prefer to see this broken up while others
prefer the style you use.  I'm not sure which way is preferred.

For the Z-Index methods, why do they take signed ints when you only want
unsigned ints?	I expected the params to be PRUint32's.

For RelativeChangeZIndex, what if aChange is 0?  Can you just return early?  Do
you want an assertion?

In GetZIndexOfAbsolutelyPositionedElement, 
 * you have a line commented out; can it be removed?  Do you want an assertion?
 * you have if (zIndexStr.Equals "auto" and then below that you have if !; can
you make it "else" and save the string comparison?  Maybe comment that block a
little more?

CreateGrabber
 * parameters should be re-ordered; aReturn should be last param.

HideGrabber
 * remove "nsEditor::" from GetRootElement call; you shouldn't need that
(elsewhere in file too)
 * you could remove this line and let the QI detect the failure for you
+  if (!bodyElement)   return NS_ERROR_NULL_POINTER;

ShowGrabber
 * might be better named "ShowGrabberOnElement"

StartMoving
 * you set mIsMoving too early; if you fail to find a body element or create
the shadow or other things, you'll get into a bad state.  I don't think the
flag should be set until near the end of that method.

GrabberClicked
 * I think mGrabberClicked is set too early; I wouldn't do it until you are
guaranteed success.

EndMoving
 * I'm worried about failures causing the booleans not to be reset and
listeners not to be removed.  Can you clean this up?

ToggleAbsolutePositioning might be better named
ToggleAbsolutePositioningForElement.  Actually, it doesn't necessarily toggle
so why not just call it SetAbsolutePositioningOnElement or something like that?

SetGridSize
 * I expect this to take an unsigned int.  It shouldn't be valid to accept
negative numbers nor 0 (assert for 0?)

SetElementPosition
 * I would anticipate the param order to be:
   aElement, aX, aY
 * should "Pixel" be part of the method name?

Should GetPositionElemented return NS_OK if mAbsolutelyPositionedObject is
null?  I assume so but thought I'd ask.

CheckSelectionStateForAnonymousButtons
 * don't declare cellElement and absPosElement until they are needed
 * you declare focusNode but don't seem to use it
 * change "his" to "its" in comment
 * the "Only" part in these booleans seems misleading to me:
refreshOnlyResizing, refreshOnlyPositioning, refreshOnlyTableEditing; can you
remove the "Only" portion of name?

GetPositionAndDimensions
 * put positionStr in its usage's scope
 * do the node QI after you check "HasAttribute" failure
 * nsCOMPtr<nsIDOMViewCSS> viewCSS = nsnull;
 * should the values all be initialized to 0 in case of premature
failure/return?

I'll submit this for now and continue in nsHTMLCSSUtils.cpp
 r- for now since I'll want to see a new diff unless you can convince me I
don't want the above changes.
Attachment #123023 - Flags: review?(brade) → review-
Attached patch fix #2 (obsolete) — Splinter Review
>in editorOverlay.xul, please leave the blank line in after cmd_align command.

Done.

>Fix this block:
>+  <toolbarbutton id="absolute-position-button" type="checkbox"
>observes="cmd_absPos">
>+    <observes element="cmd_absPos" type="checkbox" attribute="state"
>onbroadcast="onStateButtonUpdate(this.parentNode, 'cmd_absPos', 'absolute')"/>

>+  </toolbarbutton>

Done.

>in EditorOverride.css, can you add a comment for the z-index values (5+
>places)?

Done.

>I'm not very found of "transparent" resizers.

Done.

>Do we need moz_activated style?

Yes, we do for drag threshold.

>The mozGrabber image should be a res url rather than chrome.  Also the table
>images?

Will do that later, after the current patch has landed.

>I don't think that you need the style for embed "mozCXUIPlugin".

Done.

>Some of the names for the image files should be renamed to be consistent with
>existing style:
>  sendtoback-dsbl.gif --> sendtoback-disabled.gif
>Should these images be added to themes or remain in editor/ui or ?

Partly done. I need a better graphics designer than myself for the "increase
z-index" and "decrease z-index" buttons.

>changes for these methods, +nsHTMLEditor::SetAbsolutelyPosition()
>+nsHTMLEditor::RemoveAbsolutelyPosition()
> * my intuition would expect SetAbsolutelyPosition to take a param (z-index?).


No. It should position the selection without setting the z-index. In other
words, it should leave it to "auto".
 
> * I also think it's more readable to be "SetAbsolutePosition" or
>SetAbsolutePositioned" or "EnableAbsolutePositioning" or similar.

Done.

> * move the PRBools down to where they are used

Done.

> * add a blank space after the !selection line or somewhere in that chunk.

Done.

>Can you reformat this line (make 2 lines or move resultNode earlier?):
>+  nsCOMPtr<nsIDOMNode> node = do_QueryInterface(element), resultNode;

Done.

>Does GetComputedProperty always clear "positionStr"; can it ever fail?

Yes it does; no it can't.

>For this line
>+  NS_IF_ADDREF(*_retval = element);
>I've heard some super-reviewers prefer to see this broken up while others
>prefer the style you use.  I'm not sure which way is preferred.

Done.

>For the Z-Index methods, why do they take signed ints when you only want
>unsigned ints? I expected the params to be PRUint32's.

Done.

>For RelativeChangeZIndex, what if aChange is 0?  Can you just return early? 
Do
>you want an assertion?

Done.

>In GetZIndexOfAbsolutelyPositionedElement, 
> * you have a line commented out; can it be removed?  Do you want an
assertion?

Removed.

> * you have if (zIndexStr.Equals "auto" and then below that you have if !; can

>you make it "else" and save the string comparison?  Maybe comment that block a

>little more?

hmmm, no.

>CreateGrabber
> * parameters should be re-ordered; aReturn should be last param.

Done.

>HideGrabber
> * remove "nsEditor::" from GetRootElement call; you shouldn't need that
>(elsewhere in file too)

Done.

> * you could remove this line and let the QI detect the failure for you
>+  if (!bodyElement)	return NS_ERROR_NULL_POINTER;
>
>ShowGrabber
> * might be better named "ShowGrabberOnElement"

Done.

>StartMoving
> * you set mIsMoving too early; if you fail to find a body element or create
>the shadow or other things, you'll get into a bad state.  I don't think the
>flag should be set until near the end of that method.

Done.

>GrabberClicked
> * I think mGrabberClicked is set too early; I wouldn't do it until you are
>guaranteed success.

Done.

>EndMoving
> * I'm worried about failures causing the booleans not to be reset and
>listeners not to be removed.  Can you clean this up?

Done.

>ToggleAbsolutePositioning might be better named
>ToggleAbsolutePositioningForElement.  Actually, it doesn't necessarily toggle
>so why not just call it SetAbsolutePositioningOnElement or something like
that?

Done.

>SetGridSize
> * I expect this to take an unsigned int.  It shouldn't be valid to accept
>negative numbers nor 0 (assert for 0?)

Done.

>SetElementPosition
> * I would anticipate the param order to be:
>   aElement, aX, aY

Done.

> * should "Pixel" be part of the method name?

Done.

>Should GetPositionElemented return NS_OK if mAbsolutelyPositionedObject is
>null?	I assume so but thought I'd ask.

Yes it should.

>CheckSelectionStateForAnonymousButtons
> * don't declare cellElement and absPosElement until they are needed

Done.

> * you declare focusNode but don't seem to use it

Removed.

> * change "his" to "its" in comment

Done.

> * the "Only" part in these booleans seems misleading to me:
>refreshOnlyResizing, refreshOnlyPositioning, refreshOnlyTableEditing; can you
>remove the "Only" portion of name?

Done.

>GetPositionAndDimensions
> * put positionStr in its usage's scope

Done.

> * do the node QI after you check "HasAttribute" failure

Done.

> * nsCOMPtr<nsIDOMViewCSS> viewCSS = nsnull;

Assignment removed.

> * should the values all be initialized to 0 in case of premature
>failure/return?

Not needed.

Thanks a lot for your comments Kathy!
Attachment #123015 - Attachment is obsolete: true
Attachment #123023 - Attachment is obsolete: true
Comment on attachment 124213 [details] [diff] [review]
fix #2

in editor/ui/jar.mn (first block around line 35), why do the lines you are
adding not line up with the lines before and after it?

>No. It should position the selection without setting the z-index. In other
>words, it should leave it to "auto".

How about if we somehow incorporate "auto" into the name? If not, be sure to
comment in the idl file on this.

In RelativeChangeZIndex, I'd move the declaration for this down until after the
check for !selection:
+  PRBool cancel, handled;

my personal preference is to always clear return values first thing in a
method.  In GetZIndexOfAbsolutelyPositionedElement, you declare zIndexStr
before setting *aZindex = 0;

Simon found quite a bit of savings in other parts of editor code by using
C-strings when possible (avoiding NS_LITERAL_STRING, etc.).  Is it possible to
do that with any of your code?

In RefreshGrabber, you call SetAnonymouseElementPosition with
mPositionedObject* and do some offsetting there.  Please add a comment to
explain what +12 and -14 do and/or why those are 'special' (would it affect
someone who was editing text with a very large font?).

Could the grid.snap and grid.size prefs be combined to just check if grid.size
is > 0?  That would reduce the code in InitSnapToGrid as well as elsewhere.

I think this line should return NS_ERROR_OUT_OF_MEMORY:
+    if (!mMouseMotionListenerP) {return NS_ERROR_NULL_POINTER;}
Also, drop the {} since you don't use them elsewhere with similar structure.

In GrabberClicked, in the case where NS_FAILED(res), you call
HandleEventListenerError and then set mGrabberClicked to true; is that
intentional?  Is a comment helpful?

In EndMoving, you return NS_OK but do you want to return res if it fails to
remove the event listener?

fix alignment of 2nd param for this:
+nsHTMLEditor::SetAbsolutePositioningOnElement(nsIDOMElement * aElement,
+					 PRBool aEnabled)

1 set of parens not needed on this line:
+  PRBool isPositioned = (positionStr.Equals(NS_LITERAL_STRING("absolute")));

In GetPositionAndDimensions, doyou really need to check the result for
aElement->HasAttribute(_moz_abspos)?
if it fails, you can just check the "expensive" way.

in the "isPositioned" block, you set mresizedObjectIsAbsolutelyPositioned to
true but there may be some failures afterward, is it ok to set it early?  Same
question essentially for the corresponding else also.  Maybe move the
assignments out and set it to isPositioned?

I think all of the out params for GetPositionAndDimensions should be set early
so they aren't random junk (potentially).  Someone else might disagree with me
on this one.

I don't like the almost-javadoc-style comment formatting in nsHTMLCSSUtils.h;
make them real javadoc-style or drop the ** or something so they aren't so
close but not the same.

We should have a check for QI success in MouseClick before calling this line:
+    inlineTableEditing->DoInlineTableEditingAction(element);
Maybe you can get rid of htmlEditor and QI mEditor to get inlineTableEditing
outside of loop?

typo in comment in WillAbsolutePosition: reseter

Do you really wantn to set *aHandled to true if it fails later (such as
GetAbsolutelyPositionedSelectionContainer )?  Why bother setting it before the
end?

Stopping at nsHTMLObjectResizer.cpp for now.
Comment on attachment 124213 [details] [diff] [review]
fix #2

Continuing in nsHTMLObjectResizer.cpp...

At the end of CreateResizer (around line 245?), return res instead of NS_OK
(saving the NS_FAILED check)

In this chunk:
@@ -853,7 +925,7 @@
you add "emptyString" but then you never use it.

You have extra spaces in this line:
+      look->GetMetric(nsILookAndFeel::eMetric_DragThresholdY,	 yThreshold);
Comment on attachment 124213 [details] [diff] [review]
fix #2

I don't see any usage for GetResizedObject; can it be removed?

in ShowInlineTableEditingUI, you set mInlineEditedCell before creating the
anonymous elements, is that ok if there is some catastrophic failure or should
it be set later?

In HideInlinetableEditingUI, should we try to cleanup everything possible
rather than fail when we encounter a problem? (Remove MouseClick listeners? set
member buttons to null?)

I'd like to see a check for evtTarget in AddMouseClickListener and
RemoveMouseClickListener.

I'd split up this line so it's easier to read:
+  PRInt32  xHoriz = xCell + wCell/2, yVert  = yCell + hCell/2;

In nsIHTMLAbsolutePositioning.idl

Need some java-doc style comments.

I think it's fine to combine/overload "enableSnapToGrid" and "setGridSize" such
that disabling is done by setting a negative number or 0 and enabling is done 
with positive grid size.  I defer to you and others.  I see pros/cons each way.

RelativeChangeZIndex should start with lower case 'r'

Some of the names in here seem inconsistent with others:
  setElementZIndex
  getZIndexOfAbsolutelyPositionedElement

I wish the selection APIs were more distinguishable in name so I didn't have to
look at params.

Should "CheckSelectionStateForAnonymousButtons" be in nsIHTMLEditor.idl or in a
different idl file?  It should begin with 'c'

Please add java-doc style comments to the new methods in nsIHTMLEditor.idl,
nsIHTMLObjectResizeEventListener.idl, nsIHTMLObjectResizer.idl, and
nsIHTMLTableEditing.idl.

In nsIHTMLTableEditing.idl, I expect the show and hide methods to be next to
each other.

in nsComposerCommands.cpp, I'd like to see those commands moved into a new
file.  I'm not sure about a file name.	If I don't get back to you on that
soon, just go ahead with the current location.

in nsTextEditRules.h, I believe jfrancis has a patch that will conflict with
yours (he added 3014 I think).

Neil recently changed the classic theme image so you should make sure you don't
regress his changes.
> Continuing in nsHTMLObjectResizer.cpp...
>
> At the end of CreateResizer (around line 245?), return res instead of NS_OK
> (saving the NS_FAILED check)

Done.

> In this chunk:
> @@ -853,7 +925,7 @@
> you add "emptyString" but then you never use it.

Oops.

> You have extra spaces in this line:
>+      look->GetMetric(nsILookAndFeel::eMetric_DragThresholdY,	 yThreshold);

Done.
>(From update of attachment 124213 [details] [diff] [review])
>I don't see any usage for GetResizedObject; can it be removed?

Yes. For embeddors.

>in ShowInlineTableEditingUI, you set mInlineEditedCell before creating the
>anonymous elements, is that ok if there is some catastrophic failure or should
>it be set later?

Moved.

>In HideInlinetableEditingUI, should we try to cleanup everything possible
>rather than fail when we encounter a problem? (Remove MouseClick listeners? set
>member buttons to null?)

I don't think it's a problem. If we could create the UI, we should really be able
get the presShell, the docObserver and all what's needed to destroy the anonymous
nodes. That's exactly what was needed to created them after all.

>I'd like to see a check for evtTarget in AddMouseClickListener and
>RemoveMouseClickListener.

Added?

>I'd split up this line so it's easier to read:
>+  PRInt32  xHoriz = xCell + wCell/2, yVert  = yCell + hCell/2;

Done.

>In nsIHTMLAbsolutePositioning.idl
>
>Need some java-doc style comments.

Done... Pfiuuuu.

>I think it's fine to combine/overload "enableSnapToGrid" and "setGridSize" such
>that disabling is done by setting a negative number or 0 and enabling is done 
>with positive grid size.  I defer to you and others.  I see pros/cons each way.
>
>RelativeChangeZIndex should start with lower case 'r'

Oops. Good catch.

>Some of the names in here seem inconsistent with others:
>  setElementZIndex
>  getZIndexOfAbsolutelyPositionedElement
>
>I wish the selection APIs were more distinguishable in name so I didn't have to
>look at params.

Sure. Modified.

>Should "CheckSelectionStateForAnonymousButtons" be in nsIHTMLEditor.idl or in a
>different idl file?  It should begin with 'c'

I think it should stay there. Initial fixed.

Will continue later, the baby needs food. Thanks for all your comments.
>Please add java-doc style comments to the new methods in nsIHTMLEditor.idl,
>nsIHTMLObjectResizeEventListener.idl, nsIHTMLObjectResizer.idl, and
>nsIHTMLTableEditing.idl.

Done. Pfiiuuuu again ;-)

>In nsIHTMLTableEditing.idl, I expect the show and hide methods to be next to
>each other.

Moved.

>in nsComposerCommands.cpp, I'd like to see those commands moved into a new
>file.  I'm not sure about a file name.	If I don't get back to you on that
>soon, just go ahead with the current location.

Waiting :-) Please note that's polish that can be done later.

>in nsTextEditRules.h, I believe jfrancis has a patch that will conflict with
>yours (he added 3014 I think).

That's life :-) I'll deal with a conflict if he checks in before me and vice
versa :-)

>Neil recently changed the classic theme image so you should make sure you don't
>regress his changes.

Sure.
Comment on attachment 124983 [details] [diff] [review]
fix #3, in answer to all Kathy's comments

in nsHTMLEditor::SetFinalPosition(PRInt32 aX, PRInt32 aY) you declare  but
never use these:
+  NS_NAMED_LITERAL_STRING(leftStr,  "left");
+  NS_NAMED_LITERAL_STRING(topStr,   "top");

in nsHTMLEditor::SetAbsolutePositioningOnElement, you declare
NS_NAMED_LITERAL_STRING(emptyStr, "");
Why not an autostring?

In CheckPositionedElementBGandFG you declare "colorStr" but never use it.

Typo in this comment in WillAbsolutePosition (contruct):
+  // use these ranges to contruct a list of nodes to act on.

In SetResizingInfoPosition you can get rid of this and let the !doc catch the
error:
+  if (!domdoc)
+    return NS_ERROR_UNEXPECTED;

typo in nsIHTMLAbsolutePositioning.idl:  enavled

This doesn't make sense to me:
  "please note that a negative value is allowed"
but then you have this:
  in unsigned long aZorder

explain where grabber is (it's a gif? css?)

I'm not sure the javadoc style you use is correct.  Are both styles acceptable?
 I've always seen this style used:
  /**
   * text here
   */

typo in nsIHTMLObjectResizer.idl: objet

in nsAbsolutePositioningCommand::IsCommandEnabled, why bother with "isEnabled"
local var?

in nsAbsolutePositioningCommand::GetCurrentState, the string handling at end
seems clumsy; can we get rid of outStateString?

In nsDecreaseZIndexCommand::IsCommandEnabled, should this line:
+    isCmdEnabled = (z != 0);
be:
+    isCmdEnabled = (z > 0);

Can we get rid of the local variables "isEnabled" and "isCmdEnabled"? (or at
least one of them) (both Increase and Decrease ZIndex commands)

Please add #ifdef MOZ_COMPOSER around the new commands you are adding in
nsComposerCommands.cpp.
[see also feedback in comment 46]

in RelativeChangeZIndex, this line can move down a few lines:
+  PRBool cancel, handled;

I don't see where mZIndex is used at all.

Is it possible to somehow merge these sets of variables:
  mResizedObject*
  mPositionedObject*
Are the values identical if you resize an absolutely positioned object?
Do we need comments about coordinate system (related to these members)?

Change the order of params in WillRelativeChangeZIndex such that the boolean out
values are at the end (not in the middle).

I don't understand the last line of the comment in this block:
+  nsresult res = WillInsert(aSelection, aCancel);
+  if (NS_FAILED(res)) return res;
+
+  // initialize out param
+  // we want to ignore result of WillInsert()

in nsHTMLEditor::SetAllResizersPosition(), put these on separate lines to
improve readability:
+  PRInt32 x = mResizedObjectX, y = mResizedObjectY;
+  PRInt32 w = mResizedObjectWidth, h = mResizedObjectHeight;

In HideInlineTableEditingUI, you can remove a check or two:
+  nsresult res = nsEditor::GetRootElement(getter_AddRefs(bodyElement));
+  if (NS_FAILED(res)) return res;
+  if (!bodyElement)   return NS_ERROR_NULL_POINTER;
+
+  nsCOMPtr<nsIContent> bodyContent( do_QueryInterface(bodyElement) );
+  if (!bodyContent) return NS_ERROR_FAILURE;

Wouldn't you want to RemoveMouseClickListener regardless of bodyContent?

In RemoveMouseClickListener, you can remove the check for if (aElement)

The comment here seems incorrect; should it be "IN" instead of "OUT"?
+  /**
+    * enables Snap To Grid in the editor.
+    * @param aIsEnabled [OUT] PR_TRUE if it is enabled
+    */
+  void enableSnapToGrid(in boolean aIsEnabled);

For this line, please add a @return line in comment explaining what the return
value is:
+  long relativeChangeElementZIndex(in nsIDOMElement aElement, in long aChange);
>in nsHTMLEditor::SetFinalPosition(PRInt32 aX, PRInt32 aY) you declare  but
>never use these:
>+  NS_NAMED_LITERAL_STRING(leftStr,  "left");
>+  NS_NAMED_LITERAL_STRING(topStr,   "top");

Oops, removed.

>in nsHTMLEditor::SetAbsolutePositioningOnElement, you declare
>NS_NAMED_LITERAL_STRING(emptyStr, "");
>Why not an autostring?

Changed.

>In CheckPositionedElementBGandFG you declare "colorStr" but never use it.

Removed.

>Typo in this comment in WillAbsolutePosition (contruct):
>+  // use these ranges to contruct a list of nodes to act on.

Fixed.

>In SetResizingInfoPosition you can get rid of this and let the !doc catch the
>error:
>+  if (!domdoc)
>+    return NS_ERROR_UNEXPECTED;

Fixed.

>typo in nsIHTMLAbsolutePositioning.idl:  enavled

b an v are linguistically equivalent phonemes, right ?-) Fixed.

>This doesn't make sense to me:
>  "please note that a negative value is allowed"
>but then you have this:
>  in unsigned long aZorder

Fixed.

>explain where grabber is (it's a gif? css?)

Done.
>
>I'm not sure the javadoc style you use is correct.  Are both styles acceptable?
> I've always seen this style used:
>  /**
>   * text here
>   */

Fixed.

>typo in nsIHTMLObjectResizer.idl: objet

Fixed.

>in nsAbsolutePositioningCommand::IsCommandEnabled, why bother with "isEnabled"
>local var?

Removed.

>in nsAbsolutePositioningCommand::GetCurrentState, the string handling at end
>seems clumsy; can we get rid of outStateString?

Fixed.

>In nsDecreaseZIndexCommand::IsCommandEnabled, should this line:
>+    isCmdEnabled = (z != 0);
>be:
>+    isCmdEnabled = (z > 0);

Fixed.

>Can we get rid of the local variables "isEnabled" and "isCmdEnabled"? (or at
>least one of them) (both Increase and Decrease ZIndex commands)

Done.


>Please add #ifdef MOZ_COMPOSER around the new commands you are adding in
>nsComposerCommands.cpp.

You said: <brade> don't bother for now

>in RelativeChangeZIndex, this line can move down a few lines:
>+  PRBool cancel, handled;

Done.

>I don't see where mZIndex is used at all.

Removed.

>Is it possible to somehow merge these sets of variables:
>  mResizedObject*
>  mPositionedObject*
>Are the values identical if you resize an absolutely positioned object?

Yes they are but it is not possible to merge. The two objects can be
totally different, for instance if you select a resizable image inside
an absolutely positioned div.

>Do we need comments about coordinate system (related to these members)?

I don't think so.

>Change the order of params in WillRelativeChangeZIndex such that the boolean out
>values are at the end (not in the middle).

Done.

>I don't understand the last line of the comment in this block:
>+  nsresult res = WillInsert(aSelection, aCancel);
>+  if (NS_FAILED(res)) return res;
>+
>+  // initialize out param
>+  // we want to ignore result of WillInsert()

We don't care if WillInsert changed the value of aCancel.

>in nsHTMLEditor::SetAllResizersPosition(), put these on separate lines to
>improve readability:
>+  PRInt32 x = mResizedObjectX, y = mResizedObjectY;
>+  PRInt32 w = mResizedObjectWidth, h = mResizedObjectHeight;

Done.

>In HideInlineTableEditingUI, you can remove a check or two:
>+  nsresult res = nsEditor::GetRootElement(getter_AddRefs(bodyElement));
>+  if (NS_FAILED(res)) return res;
>+  if (!bodyElement)   return NS_ERROR_NULL_POINTER;
>+
>+  nsCOMPtr<nsIContent> bodyContent( do_QueryInterface(bodyElement) );
>+  if (!bodyContent) return NS_ERROR_FAILURE;

Done.

>Wouldn't you want to RemoveMouseClickListener regardless of bodyContent?

Done.

>In RemoveMouseClickListener, you can remove the check for if (aElement)

Right. Removed.

>The comment here seems incorrect; should it be "IN" instead of "OUT"?
>+  /**
>+    * enables Snap To Grid in the editor.
>+    * @param aIsEnabled [OUT] PR_TRUE if it is enabled
>+    */
>+  void enableSnapToGrid(in boolean aIsEnabled);

Oops, right.

>For this line, please add a @return line in comment explaining what the return
>value is:
>+  long relativeChangeElementZIndex(in nsIDOMElement aElement, in long aChange);

Done.
Thanks for your excellent review, Kathy.
Attached patch fix #4 (obsolete) — Splinter Review
Attachment #124983 - Attachment is obsolete: true
Comment on attachment 125409 [details] [diff] [review]
fix #4

r=brade
please test with midas too
Attachment #125409 - Flags: review+
Attachment #125409 - Flags: superreview?(dbaron)
*** Bug 199150 has been marked as a duplicate of this bug. ***
Comment on attachment 125409 [details] [diff] [review]
fix #4

Here are comments on the part of the patch that I've looked at so far:

The new interface names don't really meet the "is a" test.  I'd suggest:

 s/nsIHTMLTableEditing/nsIHTMLTableEditor/  (or maybe something with "Inline"?)
 s/nsIHTMLAbsolutePositioning/nsIHTMLAbsPosEditor/

so that it makes sense to say the object is an instance of the
interface (e.g., "is an HTMLTableEditor" vs. "is an HTMLTableEditing").

No need to edit the MANIFEST files -- they've been cvs-removed.

EditorOverride.css has moved to
editor/composer/src/res/EditorOverride.css -- you need to move your
diffs to the new location.

(Essentially, the previous two comments suggest that this patch probably
needs to be merged to the tip and that such merging might require a bit
of work.)


EditorOverride.css:

 * There are a few selectors here (_moz_abspos attribute selectors) that
   are going to be slow, but I guess you need these.  (Is the reason for
   the _moz_ prefix on attributes that they don't get serialized?)

 * There are a bunch of occurrences of 'position: absolute; display:
   block'.  The 'display: block' is redundant (CSS2 9.7) and should be
   removed.

nsIHTMLAbsolutePositioning.idl:

 * As I said above, I think this should be renamed.

 * This combination:
+  readonly attribute boolean	    isAbsolutePositioningEnabled;
+  void enableAbsolutePositioning(in boolean aIsEnabled);
   should be better implemented as:
+  attribute boolean absolutePositioningEnabled;
   which translates to C++ methods
   NS_IMETHOD GetAbsolutePositioningEnabled(PRBool**);
   NS_IMETHOD SetAbsolutePositioningEnabled(PRBool*);

 * I think |readonly attribute boolean
   isSelectionContainerAbsolutelyPositioned| could probably lose the
   "is" off the front (and thus lowercase 's').  Naming attributes
   starting with "is" is generally unnecessary.

+   * @param aZorder  [IN] the z-index; please note that a negative value is
allowed
+   *			   here to be conformant to CSS grammar but a negative
z-index
+   *			   is an error according to spec.

It's not an error, and the spec will likely be fixed so it does
something more reasonable than now.

+   * @param aEnabled [IN] PR_TRUE to absolutely position the element,
+   *			   PR_FALSE to put it back in the normal flow

You shouldn't refer to specific C++-isms in idl -- use "true" and
"false" rather than "PR_TRUE" and "PR_FALSE".

Some of the method names are rather long.  Maybe
setAbsolutePositioningOnElement could be absolutelyPositionElement
and maybe setElementPositionInPixels could be setElementPosition?

Also, the selection-based methods and element-based methods don't seem
as parallel as they could be -- you have setAbsolutePosition() and
removeAbsolutePosition() for the former and a single function taking a
boolean for the latter.


nsIHTMLEditor.idl:
  I'm assuming the indentation changes are just indentation changes --
  if not, could you attach a |diff -uw| of the file?

nsIHTMLObjectResizeEventListener.idl:

  I think this interface should probably be called
  nsIHTMLObjectResizeListener -- I don't see anything about "resize
  events".

nsIHTMLObjectResizer.idl:

  More typical naming for listener methods would be "onStartResizing"
  and "onEndResizing" rather than "startsResizing" and "endsResizing".

  +  readonly attribute boolean       isObjectResizingEnabled;
  [...]
  +  void enableObjectResizing(in boolean aEnable);
  These looks like they should be combined into a single:
  attribute boolean objectResizingEnabled;
  (note no "is").

  All the "resize event listener" stuff should probably be renamed
  "resize listener".

nsIHTMLTableEditing.idl:

  Like I said above, I think this should be renamed.  (Also, should it
  have the word "inline" in it somewhere?)

  +  readonly attribute boolean isInlineTableEditingEnabled;
  [...]
  +  void enableInlineTableEditing(in boolean aIsEnabled);
  This looks like another pair that could be combined into:
  attribute boolean inlineTableEditingEnabled

  This could use a clear description of how it differs from
  nsITableEditor.  (The same for nsHTMLTableEditing.cpp and
  nsTableEditor.cpp)


nsHTMLAbsPosition.cpp:

 * It's somewhat unusual to split the implementation of a single class
   across multiple files.  Then again, there already seems to be a
   precedent for this in nsTableEditor.cpp and nsHTMLEditorStyle.cpp.
   However, I think you should to add a comment to nsHTMLEditor.h saying
   where the methods are implemented.

 * Why are EnableSnapToGrid (and the grid size) implemented via a
   preference (which also has no observer for preference changes), while
   EnableAbsolutePositioning via a member variable?  If something should
   really be a pref, I'd think you'd want to implement pref change
   observers, and not have any other API.

 * in nsHTMLEditor::RemoveAbsolutePosition(), check NS_FAILED(res)
   before cancel to avoid reading uninitialized memory.

 * in nsHTMLEditor::GetAbsolutelyPositionedSelectionContainer,
   |node = parentNode| could be |node.swap(parentNode)|.

 * is there a reason
   nsHTMLEditor::GetAbsolutelyPositionedSelectionContainer breaks out of
   the loop if the node doesn't implement nsIDOMHTMLElement?  (What if
   this is used on a mix of XHTML and other XML?)
Comment on attachment 125409 [details] [diff] [review]
fix #4

In addition to David's comments, here's what I found while going through this,
mostly nits, but some real issues...

+  text-decoration: none ! important;
+  border: none 0px ! important;
...
+  -moz-user-select: none !important;
+  -moz-user-focus: none !important;

Inconsistent use of space-after-'!', either include a space, or not, but be
consistent.

- In editor/ui/jar.mn

	content/editor/StructBarContextMenu.js			
(composer/content/StructBarContextMenu.js) 
+      content/editor/images/grabber.gif		       
(composer/content/images/grabber.gif)

Doesn't show up here, but the indentation used here is a tab, you used spaces.

- In nsHTMLEditor::SetAbsolutePosition():

+  nsTextRulesInfo ruleInfo(nsTextEditRules::kSetAbsolutePosition);
+  PRBool cancel, handled;
+  res = mRules->WillDoAction(selection, &ruleInfo, &cancel, &handled);
+  if (cancel || NS_FAILED(res))
+    return res;
+  
+  return mRules->DidDoAction(selection, &ruleInfo, res);

Maybe this is just an editorism, I see this pattern a few times, but this seems
odd to me. You call WillDoAction() and then DidDoAction(), but there's no code
that obviously does actually *do* the action...

- In nsHTMLEditor::RefreshGrabber():

+  nsresult res = GetPositionAndDimensions(mAbsolutelyPositionedObject,
+				  mPositionedObjectX,
+				  mPositionedObjectY,
...

Fix next-line argument indentation.

- In nsHTMLEditor::SnapToGrid():

+    newX = (PRInt32) floor( ((float)newX / (float)mGridSize) + 0.5 ) *
mGridSize;
+    newY = (PRInt32) floor( ((float)newY / (float)mGridSize) + 0.5 ) *
mGridSize;

Don't you want 0.5f there to ensure it's a float and not a double?

- In nsHTMLEditor::CheckPositionedElementBGandFG():

+  nsCOMPtr<nsIDOMNode> node = do_QueryInterface(aElement);

No need to QI here, nsIDOMElement inherits nsIDOMNode.

- In nsHTMLEditor::CreateAnonymousElement():

+{
+  NS_ENSURE_ARG_POINTER(aParentNode);
+  NS_ENSURE_ARG_POINTER(aReturn);
+
+  nsCOMPtr<nsIContent> parentContent( do_QueryInterface(aParentNode) );
+  if (parentContent) {
...
+  }
+  return NS_OK;

Maybe return early if (!parentContent) in stead to avoid indenting the whole
method?

- In nsHTMLEditor::GetPositionAndDimensions():

+  nsCOMPtr<nsIDOMNode> node = do_QueryInterface(aElement);

No need to QI here, and it looks like you could eliminate node all together
here and just use aElement directly (which is an nsIDOMElement, which is an
nsIDOMNode).

- In nsHTMLEditor.cpp

@@ -248,6 +250,16 @@
     NS_ADDREF_THIS();
     return NS_OK;
   }
+  if (aIID.Equals(NS_GET_IID(nsIHTMLAbsolutePositioning))) {
+    *aInstancePtr = NS_STATIC_CAST(nsIHTMLAbsolutePositioning*, this);
+    NS_ADDREF_THIS();
+    return NS_OK;
+  }
+  if (aIID.Equals(NS_GET_IID(nsIHTMLTableEditing))) {
+    *aInstancePtr = NS_STATIC_CAST(nsIHTMLTableEditing*, this);
+    NS_ADDREF_THIS();
+    return NS_OK;
+  }

Why not save us some code and make this use the nsISupports macros in stead of
adding more and more hand written QI code?

+    EnableInlineTableEditing(!PRBool(aFlags & eEditorMailMask));

Why the PRBool cast? More readable w/o it, IMO...

- In nsHTMLEditor::GetSelectionContainer():

+  nsCOMPtr<nsIDOMElement> focusElement = do_QueryInterface(focusNode);
+  *aReturn = focusElement;
+  NS_ADDREF(*aReturn);

How about just this in stead?

+ CallQueryInteface(focusNode, aReturn);

- In nsHTMLEditor.h:

+  PRPackedBool mGrabberClicked;
+  PRPackedBool mIsMoving;
+
+  PRInt32	mGridSize;
+
+  nsresult CreateGrabber(nsIDOMNode * aParentNode, nsIDOMElement ** aReturn);
... more functions ...
+  nsresult CheckPositionedElementBGandFG(nsIDOMElement * aElement,
+					  nsAString & aReturn);
+  /* INLINE TABLE EDITING */
+
+  PRPackedBool mIsInlineTableEditingEnabled;

If you'd move mGridSize above mGrabberClicked, you'd save 4 bytes by making the
3 PRPackedBool's sit side by side in this class...

- In editor/idl/Makefile.in

		nsIHTMLEditor.idl \
		nsIHTMLObjectResizer.idl \
+		 nsIHTMLAbsolutePositioning.idl \
+		 nsIHTMLObjectResizeEventListener.idl \
+		 nsIHTMLTableEditing.idl \
		nsIPlaintextEditor.idl \

Use tabs like the existing lines do...

- In nsIHTMLAbsolutePositioning.idl:

+  /**
+   * enables Absolute Positioning handling in the editor.
+   * @param aIsEnabled [IN] PR_TRUE if it is enabled
+   */
+  void enableAbsolutePositioning(in boolean aIsEnabled);
+
+  /**
+   * enables Snap To Grid in the editor.
+   * @param aIsEnabled [IN] PR_TRUE if it is enabled
+   */
+  void enableSnapToGrid(in boolean aIsEnabled);
+
+  /**
+   * sets the grid size in pixels.
+   * @param aSizeInPixels [IN] the size of the grid in pixels
+   */
+  void setGridSize(in unsigned long aSizeInPixels);
+
+  /* Selection-based methods */
+
+  /**
+   * returns the deepest absolutely positioned container of the selection
+   * if it exists or null.
+   */
+  nsIDOMElement getAbsolutelyPositionedSelectionContainer();

Shouldn't all of the above be attributes?

- In nsIHTMLObjectResizer.idl

+  /**
+   * enables or disables object resizing in the editor
+   * @param aEnable [IN] true if object resizing is enabled
+   */
+  void enableObjectResizing(in boolean aEnable);

Shouldn't that be an attribute, i.e.

+  attribute boolean objectResizingEnabled;

- In nsIHTMLTableEditing.idl:

+  /**
+   * Enables or disables inline table editing in the editor
+   * @param aIsEnabled [IN] true to enable inline table editing
+   */
+  void enableInlineTableEditing(in boolean aIsEnabled);

Same thing, make that:

+  attribute boolean inlineTableEditingEnabled;

Have a look at that, and what David said, and once you have a new patch, I'll
sr (unless David beats me to it).
Attachment #125409 - Attachment is obsolete: true
Comment on attachment 126363 [details] [diff] [review]
fix #5, in answer to _all_ dbaron's and jst's comments

forgot to obsolete previous patch, sorry for spam
Attachment #126363 - Attachment is obsolete: true
Comment on attachment 126365 [details] [diff] [review]
fix #5.1

The place where I asked you to use CallQueryInterface() looked like this:

+  nsCOMPtr<nsIDOMElement> focusElement = do_QueryInterface(focusNode);
+  *aReturn = focusElement;
+  NS_ADDREF(*aReturn);

I assumed that you knew for a fact that focusNode was non-null here, (since you
do NS_ADDREF(), not NS_IF_ADDREF()), but it appears that that's not the case.
In stead of doing a null check and then calling CallQueryInterface() you
probably want to do what you're doing, except you want that NS_ADDREF() to be
an NS_IF_ADDREF().

sr=jst
Attachment #126365 - Flags: superreview+
yay!
Status: ASSIGNED → RESOLVED
Closed: 21 years ago21 years ago
Resolution: --- → FIXED
I am glad it was not hard to do.
Thanks for all the hard work.
FYI, fix #5.1 (attachment 126365 [details] [diff] [review]) caused regressions bug 210641 and bug 211378.
Depends on: 211378
Depends on: 210641
Attachment #123023 - Flags: superreview?(sfraser)
Depends on: 512226
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: