Closed
Bug 137092
Opened 22 years ago
Closed 21 years ago
Absolute position support in Composer/editor core
Categories
(Core :: DOM: Editor, enhancement, P1)
Core
DOM: Editor
Tracking
()
RESOLVED
FIXED
mozilla1.5alpha
People
(Reporter: thinkerdreamer, Assigned: glazou)
References
()
Details
Attachments
(3 files, 6 obsolete files)
178.41 KB,
image/jpeg
|
Details | |
25.64 KB,
application/octet-stream
|
Details | |
209.43 KB,
patch
|
jst
:
superreview+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•22 years ago
|
||
me agrees with the high interest of the feature... Probably not too hard to do.
Comment 2•22 years ago
|
||
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.
Comment 4•22 years ago
|
||
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.
Comment 6•22 years ago
|
||
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.
Comment 7•22 years ago
|
||
OS -> All NEW
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 98 → All
Reporter | ||
Comment 8•22 years ago
|
||
Reporter | ||
Comment 9•22 years ago
|
||
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.
Assignee | ||
Comment 10•22 years ago
|
||
Mark, I reiterate my comment : not hard to do. Trust me.
Comment 11•22 years ago
|
||
Does that mean that we can expect a patch from you soon?
Assignee | ||
Comment 12•22 years ago
|
||
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.
Comment 13•22 years ago
|
||
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!
Updated•22 years ago
|
Keywords: helpwanted
Hardware: PC → All
Updated•22 years ago
|
Blocks: WYSIWYG
Summary: [RFE] Movable objects over layer of background image → Movable objects over layer of background image
Assignee | ||
Comment 15•21 years ago
|
||
Taking. Nominating.
Assignee: composer → glazman
Priority: -- → P1
Summary: Movable objects over layer of background image → Absolute position support in Composer/editor core
Target Milestone: Future → mozilla1.3final
Updated•21 years ago
|
Comment 19•21 years ago
|
||
wontfix
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → WONTFIX
Comment 20•21 years ago
|
||
If you are going to close this as WONTFIX, can you at least explain why?
Comment 21•21 years ago
|
||
reopen bug
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Target Milestone: mozilla1.3final → Future
Assignee | ||
Comment 23•21 years ago
|
||
Thanks Kathy.
Comment 24•21 years ago
|
||
Sorry, triage mistake on my part :-(
Assignee | ||
Comment 25•21 years ago
|
||
Patch coming.
Status: REOPENED → ASSIGNED
Target Milestone: Future → mozilla1.5alpha
Assignee | ||
Comment 26•21 years ago
|
||
To be uncompressed into mozilla/ui/composer/content/images
Assignee | ||
Comment 27•21 years ago
|
||
This patch corresponds to all my additions to Composer. Absolute positioning, better resizing, snap to grid, inline table editing.
Assignee | ||
Comment 28•21 years ago
|
||
Comment on attachment 123023 [details] [diff] [review] patch brade, r= ? kin, sr= ?
Attachment #123023 -
Flags: superreview?(kin)
Attachment #123023 -
Flags: review?(brade)
Comment 29•21 years ago
|
||
Should we get usability feedback before landing these changes? I'm particularly concerned about the inline table editing stuff.
Assignee | ||
Comment 30•21 years ago
|
||
Simon, yes I agree about the line table editing.
Comment 31•21 years ago
|
||
(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 :-)
Assignee | ||
Comment 32•21 years ago
|
||
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?
Assignee | ||
Comment 34•21 years ago
|
||
> 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.
Assignee | ||
Comment 35•21 years ago
|
||
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 36•21 years ago
|
||
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-
Assignee | ||
Comment 37•21 years ago
|
||
>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
Assignee | ||
Comment 38•21 years ago
|
||
Comment 39•21 years ago
|
||
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 40•21 years ago
|
||
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 41•21 years ago
|
||
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.
Assignee | ||
Comment 42•21 years ago
|
||
> 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.
Assignee | ||
Comment 43•21 years ago
|
||
>(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.
Assignee | ||
Comment 44•21 years ago
|
||
>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.
Assignee | ||
Comment 45•21 years ago
|
||
Attachment #124213 -
Attachment is obsolete: true
Comment 46•21 years ago
|
||
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.
Comment 47•21 years ago
|
||
[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);
Assignee | ||
Comment 48•21 years ago
|
||
>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.
Assignee | ||
Comment 49•21 years ago
|
||
Attachment #124983 -
Attachment is obsolete: true
Comment 50•21 years ago
|
||
Comment on attachment 125409 [details] [diff] [review] fix #4 r=brade please test with midas too
Attachment #125409 -
Flags: review+
Assignee | ||
Updated•21 years ago
|
Attachment #125409 -
Flags: superreview?(dbaron)
Assignee | ||
Comment 51•21 years ago
|
||
*** 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 53•21 years ago
|
||
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).
Assignee | ||
Comment 54•21 years ago
|
||
carries forward r=brade
Assignee | ||
Updated•21 years ago
|
Attachment #125409 -
Attachment is obsolete: true
Assignee | ||
Comment 55•21 years ago
|
||
Assignee | ||
Comment 56•21 years ago
|
||
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 57•21 years ago
|
||
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+
Assignee | ||
Comment 58•21 years ago
|
||
yay!
Status: ASSIGNED → RESOLVED
Closed: 21 years ago → 21 years ago
Resolution: --- → FIXED
Comment 59•21 years ago
|
||
I am glad it was not hard to do.
Reporter | ||
Comment 60•21 years ago
|
||
Thanks for all the hard work.
Attachment #125409 -
Flags: superreview?(dbaron)
Comment 61•21 years ago
|
||
FYI, fix #5.1 (attachment 126365 [details] [diff] [review]) caused regressions bug 210641 and bug 211378.
Updated•21 years ago
|
Attachment #123023 -
Flags: superreview?(sfraser)
You need to log in
before you can comment on or make changes to this bug.
Description
•