399046 - Resizers of centered tables are positioned wrongly in designmode

Status: RESOLVED WORKSFORME

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

Description

[Robert Stopp (textrinum)]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.8.1.7) Gecko/20070914 Firefox/2.0.0.7
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.8.1.7) Gecko/20070914 Firefox/2.0.0.7

The resizers of a focused Table in designmode are wrong positioned. In Gecko 1.8 they are displayed over the full width of the body, in 1.9 they have the correct size but are left aligned.
Maybe this is only a duplicate of bug 364719 but here we have no relative or absolute positioned parent-elements.

Reproducible: Always

Steps to Reproduce:
1. see the attachment
2 [review].
3.



see the attachment

Comment 1

[Robert Stopp (textrinum)]

(In reply to comment #0)
> User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.8.1.7)
> Gecko/20070914 Firefox/2.0.0.7
> Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.8.1.7)
> Gecko/20070914 Firefox/2.0.0.7
> 
> The resizers of a focused Table in designmode are wrong positioned. In Gecko
> 1.8 they are displayed over the full width of the body, in 1.9 they have the
> correct size but are left aligned.
> Maybe this is only a duplicate of bug 364719 but here we have no relative or
> absolute positioned parent-elements.
> 
> Reproducible: Always
> 
> Steps to Reproduce:
> 1. see the attachment
> 2 [details].
> 3.
> 
> 
> 
> see the attachment
> 

Comment 2

[Robert Stopp (textrinum)]

Attachment: shows some of the behavior with resizers and movers in designmode

sorry for the previous post...

if the "mover" of the absolute positioned div is over the div parent-element he 
doesnt intercept the click-event

Comment 3

[Martijn Wargers (dead)]

Yeah, I suspect this is a duplicate of bug 364719, but I'll make a dependency for now.

Comment 4

[Robert Stopp (textrinum)]

bug 364719 is fixed but the resizers of centered tables are still wrong positioned.

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

Damn, more work to do here.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Something to do with table inner frame/table outer frames, probably. I'll explain when we meet up in person.

Comment 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Slightly nastier testcase

This is a slightly nastier version of the existing testcase.

Comment 8

[Robert Stopp (textrinum)]

(In reply to comment #7)
> 
> This is a slightly nastier version of the existing testcase.
> 
Only in Gecko 1.9, in 1.8 the add/delete - elements are correct positioned.

There is another problem: If the body has no padding and margin the add/ delete elements are sometimes unreachable. I would prefer a position a bit more (1-2 pixels) over the focused element - more like absolute positioned elements.

Comment 9

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Sadistic test case

This is a nastier test case, with variable sized margins, borders, and padding.

Comment 10

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Work in progress patch #1

This is my work in progress patch.

* Object resizers now appear at the edge of the object being resized's border.
* nsHTMLEditor::GetElementOrigin() now checks to see if it's in a tableOuterFrame, and if so defers to the table inner frame.
* Handles variable sized borders, paddings, and margins.

This pretty much fixes the resizers, but there's one problem remaing: when an image with thick borders is resized, the image "shadow" expands to the size of the borders+image, i.e. the borders aren't present in the shadow, the shadow expands to the size of the resized object's border. I'll fix this next, shouldn't take long. Anyone (i.e. Roc) who wants to provide feedback on the patch is welcome.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

You should share your tableOuterFrame checking code with nsGenericHTMLElement::GetStyledFrameFor, probably via a helper function in nsContentUtils, say

  /**
   * Dig through wrapper frames to find the frame whose border-box is the true
   * border-box for the content rendered by aFrame. Currently just converts
   * a tableOuterFrame to the inner table frame.
   */
  static nsIFrame* GetBorderBoxFrame(nsIFrame* aFrame);

Comment 12

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Resizable images with nasty border testcase

This is a nasty testcase similarly sadistic to the previous, but with images with variable width borders rather than tables. This also exposes a bug with the grabber; it's background color isn't set, so it's padding is transparent.

Comment 13

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Testcase - resizing absolute positioned objects

This testcase tests resizing of absolutely positioned image and tables.

Comment 14

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v1 - resizers on tables and images in correct place

This makes resizers appear correctly for images and tables, including those that are absolutely positioned. Also when you reposition absolutely positioned objects, the shadow now includes the border of the thing it's shadowing - so if you reposition an image, as well as seeing a shadow of the image, you see its border shadowed as well.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Peterv, do you mind if I review this one? I'm not sure if I'm "allowed" to review this editor stuff.

Comment 16

[Peter Van der Beken [:peterv]]

(In reply to comment #15)
> Peterv, do you mind if I review this one? I'm not sure if I'm "allowed" to
> review this editor stuff.

Go for it.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  if (!mPresShellWeak) return nsnull;

You don't actually need this, do_QueryReferent is null-safe.

nsHTMLEditor::GetPrimaryFrameFor should call FlushPendingNotifications(Flush_Layout) unless there's a flush already on the way to this function (if that's so, you'd better mention where that flush is).

+  // Converts an nsMargin from app units to pixels.
+  static nsSize& AppUnitsToPixels(nsSize &aSize);

Converts an *nsSize*

+  struct nsSizeDimInfo {
+    PRInt32 mContainerX;
+    PRInt32 mContainerY;
+    PRInt32 mContainerWidth;
+    PRInt32 mContainerHeight;

How about using nsIntRects? What are the rectangles positioned relative to? That should be documented at least. I think they're positioned relative to the element's absolute containing block, right? mContainer* should more accurately be called mBorderBox because that's what it is.

Hmm, should these be in appunits instead? Converting to pixels is a lossy operation? I guess that's a bigger change that we need here, so never mind.

+  struct nsSizeDimInfo {

Call it "nsElementGeometry", maybe?

+  PRInt32 grabberHeight = nsPresContext::AppUnitsToIntCSSPixels(size.height);

Why do we use the grabber height here but we hardcode the width? Shouldn't we do the same thing in both directions? What's the -2 mean in the Y direction anyway? I see the old code adds for X and subtracts for Y, why is that? Document it.

We really shouldn't have top and left resizers on tables, since dragging those doesn't make the table move to the top or left. Food for another bug.

+  aInfo->mContentX = aInfo->mContainerX
+                   - aInfo->mPaddingLeft
+                   - aInfo->mBorderLeft;

Shouldn't these be adding? I'm confused.

Instead of computing the content and border-box rects like this, just get them directly from the frame using frame->GetRect() and frame->GetContentRect(). You'll need to find the nearest absolute containing block ancestor frame and use nsIFrame::GetOffsetTo to convert the rects to the right coordinate system before you convert them to pixels.

+  // Find the parent of our owning <table>.
+  while (parentNode && !childIsTable && nsHTMLEditUtils::IsTableElement(parentNode)) {
+    // Handle table inside table.
+    childIsTable = nsHTMLEditUtils::IsTable(parentNode);
+    res = parentNode->GetParentNode(getter_AddRefs(parentNode));
+    NS_ENSURE_SUCCESS(res, res);
+  }

What exactly is this code looking for?

+  static nsString Properties[] = {
+    NS_LITERAL_STRING("border-top-style"),
+    NS_LITERAL_STRING("border-bottom-style"),
+    NS_LITERAL_STRING("border-left-style"),
+    NS_LITERAL_STRING("border-right-style"),
+
+    NS_LITERAL_STRING("border-top-color"),
+    NS_LITERAL_STRING("border-bottom-color"),
+    NS_LITERAL_STRING("border-left-color"),
+    NS_LITERAL_STRING("border-right-color"),
+
+    EmptyString()

You can't have static nsStrings, that shows up as leaks. I'd just use a char* array and use NS_ConvertUTF8toUTF16 to make nsStrings where you need to pass them in.

+  while (!propName->IsEmpty()) {

Dispense with the empty sentinel and use NS_ARRAY_LENGTH.

+  NS_NAMED_LITERAL_STRING(borderTopWidth, "border-top-width");
+  NS_NAMED_LITERAL_STRING(borderBottomWidth, "border-bottom-width");
...
+  mHTMLCSSUtils->SetCSSPropertyPixels(retval, marginLeft, margin.left);
+  mHTMLCSSUtils->SetCSSPropertyPixels(retval, marginRight, margin.right);

You could shrink the code here by defining arrays of 4 char* property names, and then have a helper function that takes an array of 4 char* property names, an nsMargin, converts it to pixels (doesn't need to round!) and sets the properties from the margin.

A big comment in CreateShadow to explain the UI you're trying to achieve would be useful.

Big-picture question: is this shadow really the right way to go? Wouldn't it be much easier to implement, and better UI, to just change the size of the real object as the user drags?

Comment 18

[Peter Van der Beken [:peterv]]

Resizing the real content seems fine to me, but I'm not a good person to ask about UI stuff ;-).

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Let's just do that, it's much simpler and I think the editing UI has never been a usability focus, shall we say :-)

Comment 20

[Robert Stopp (textrinum)]

I think (don't tried this (Open Office,IE)) the flickering would troubling more than the better preview would rewarding. 
But iam with Roc that tables have to much resizers. Most of them, especially the North-South and East-West are completly useless.

Comment 21

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to comment #20)
> I think (don't tried this (Open Office,IE)) the flickering would troubling more
> than the better preview would rewarding. 
> But iam with Roc that tables have to much resizers. Most of them, especially
> the North-South and East-West are completly useless.
> 

The shadow doesn't flicker (on windows at least), so resizing the actual image/table/div should be ok.

Resizer display rules:
1. For absolutely positioned resizables, show all resizers.
2. For center-aligned resizables, show middle-left, middle-right, bottom-left, bottom-middle, and bottom-right resizers.
3. For left-aligned resizables, show middle-right, bottom-middle, and bottom-right resizers.
4. For right-aligned resizables, show middle-left, bottom-left, and bottom-middle.

Comment 22

[Robert Stopp (textrinum)]

That sounds very good. Useful could be a border on tables with no borders (border=0) to avoid wondrousness at some users.

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think we should make a separate bug for hiding the "useless" resizers ... one that probably shouldn't block 1.9.

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Unless fixing that bug makes fixing this bug easier, which I doubt.

Comment 25

[Martijn Wargers (dead)]

It might very well be possible that a similar thing in IE is already possible, enabling/disabling resizers, by one of the execCommand commands:
http://msdn2.microsoft.com/en-us/library/ms533049(VS.85).aspx

Comment 26

[Robert Stopp (textrinum)]

Attachment: Shows the problem if borders on table are not present (assisting the devil...)

If resizers are present on tables, a border to join them (like on images/divs) could be useful in my opinion.

Comment 27

[Robert Stopp (textrinum)]

Is this issue still in work?
There was no progress to see in the last two betas.

Comment 28

[Chris Pearce [:cpearce (Not reading bugmail)]]

I have done some work on this, but got sidetracked by higher priority bugs. I found a couple more cases which break involving scroll frames, so it still needs some more work. Given that this bug is P3 non-blocking, I don't think this will make it into FF3. Sorry Robert. :/

Comment 29

[Robert Stopp (textrinum)]

Very bad news :(
But what means that? FF4? Is it that what the Flag "wanted‑next" means?
Isn't there a quick and dirty solution that resolves the origin point (resizers of centered tables) and ignores the bordered, positioned parents for later?
Some of the stuff in the testcases is a bit away from real life ;-)
I think this bug is from netscape days!

Comment 30

[Chris Pearce [:cpearce (Not reading bugmail)]]

Note to self: don't regress bug 428489, bug 424027 and bug 420439 with the patch for this.

Comment 31

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: V2 - Resize/move without shadow

* Removes "shadow" on resizing/moving, so you now resize/move the element in real time.
* Resize/moves are now done by a mergeable transaction.
* Did a bit of cleanup in the resizing/moving code.
* Added pretty solid mochitest case, which includes the key sadistic cases of all the testcases I've been working with.
* I'm sorry this is quite a big patch, I couldn't really do anything by parts here, since it is so tightly interrelated.

Comment 32

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: V2 - Resize/move without shadow - Compiles under linux

I had to make a slight change to the patch to get it to compile under linux.

You can download a TryServer build here if you want to test this:

https://build.mozilla.org/tryserver-builds/2008-05-14_18:[e-mail]-cpearce-resizers-2/

Comment 33

[Robert Stopp (textrinum)]

Realy good work and the flickering is acceptable!
Some minor stuff:
* Images and tables become a left/top CSS-Rule after resizing (with sometimes negative values) - for what? Is that the mergeable transaction?
* Proportional resizing of images seems sometimes not proportional (more width than height). Striking on 16:9 and wider or the google pics in the testcase.
* Realy minor: the value indicator goes out of document if right side is reached.
* In my implemantation (iframe in document in frameset in iframe in document) the caret is not visible under the move cursor when moving images and other stuff (the old way without shadow was more accurate). Maybe another bug.

Thanks

Comment 34

[Chris Pearce [:cpearce (Not reading bugmail)]]

Thanks for your feedback Robert, you've picked up on some things I missed. I didn't check carefully enough for the non-absolutely positioned resize case, so this patch needs some more work. :(

Comment 35

[Robert Stopp (textrinum)]

Attachment: Table with collapsed borders computing problem.

With pleasure Chris :)
there is another special case: Tables can have "collapsed" borders (nicer, without 3D). The resize function from the patch has some trouble with the correct computing of that borders i think. Play a bit with the testcase to see it live.
Sorry for more work, it isn't much anymore, i think.

My caret was catched by an eventhandler, forget it.

Comment 36

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: V3

Patch v3. Fixes issues described above, includes more mochitests. I will make a TryServer build for everyone's testing pleasure, it should be ready in a few hours...

Comment 37

[Chris Pearce [:cpearce (Not reading bugmail)]]

Build with patch v3 is here: https://build.mozilla.org/tryserver-builds/2008-05-27_21:[e-mail]-cpearce-399046-v3/

Comment 38

[Robert Stopp (textrinum)]

No pleasure with this build because nothing to nag...
Thanks for fixing and tweaking that!

Hopefully in the next Release Candidate!

Comment 40

[Robert Stopp (textrinum)]

Chris: Is this issue still in work?
Can't get it reviewed/patched because it's blocked by bug 436004 ?

Comment 41

[Chris Pearce [:cpearce (Not reading bugmail)]]

Yeah, Peter V is reviewing it, I've chased him up about it. Hopefully this will make 3.1 or 3.2.

Comment 42

[Robert Stopp (textrinum)]

Attachment: Shows the behaviour with disabled ObjectResizing for an table.

There is an additional issue with resizers. If object-resizing is disabled with the (undocumented) "enableObjectResizing" command, the resizers remains visible if the selection changes. Example: We want only images resizable and tables not. See the testcase.
A solution for bug 436004, and perhaps here too, would be: Setting the selection to the resized object after resizing?

In some rare cases the inline table editing controls shows up more out of (left) the cell as they should with the tryserver build. Click on a textnode after resizing the table smaller with this: https://bugzilla.mozilla.org/attachment.cgi?id=337902

Comment 43

[Peter Van der Beken [:peterv]]

Comment on attachment 322728 [details] [diff] [review]
V3

>Index: editor/libeditor/html/nsHTMLAbsPosition.cpp
>===================================================================

>@@ -275,33 +276,40 @@ nsHTMLEditor::CreateGrabber(nsIDOMNode *

>   nsresult res = GetPositionAndDimensions(mAbsolutelyPositionedObject,
>-                                         mPositionedObjectX,
>-                                         mPositionedObjectY,
>-                                         mPositionedObjectWidth,
>-                                         mPositionedObjectHeight,
>-                                         mPositionedObjectBorderLeft,
>-                                         mPositionedObjectBorderTop,
>-                                         mPositionedObjectMarginLeft,
>-                                         mPositionedObjectMarginTop);
>+                                          &mPositionedObjectInfo);
>+  NS_ENSURE_SUCCESS(res,res);

Space after ,

>+  PRInt32 grabberX = mPositionedObjectInfo.mBorderBox.x
>+                   + grabberWidth;
>+
>+  PRInt32 grabberY = mPositionedObjectInfo.mBorderBox.y
>+                   - grabberHeight
>+                   - 2; // _moz_abspos outline width.

Operators at end of line.
Can this end up with grabberY < 0?

>Index: editor/libeditor/html/nsHTMLAnonymousUtils.cpp
>===================================================================

>@@ -56,29 +58,49 @@

>+  nsCOMPtr<nsIDOMViewCSS> viewCSS;
>+  nsresult res = mHTMLCSSUtils->GetDefaultViewCSS(aObject, getter_AddRefs(viewCSS));

Long line.

>+  // Check the type of the returned CSSValue; we handle here only
>+  // pixel and enum types.

...; we only deal with pixel and enum types.

>@@ -397,84 +419,177 @@ nsHTMLEditor::CheckSelectionStateForAnon

>+nsHTMLEditor::GetPrimaryFrameFor(nsIDOMElement *aElement)

>+  document->FlushPendingNotifications(Flush_Layout);
>+
>+  nsCOMPtr<nsIContent> content = do_QueryInterface(aElement);

Might be safer to take the strong ref before FlushPendingNotifications.

>+nsHTMLEditor::GetEditingUiParentFrameFor(nsIDOMElement *aElement)

Maybe make this take nsIContent*.

>+  document->FlushPendingNotifications(Flush_Layout);
>+
>+  nsCOMPtr<nsIContent> bodyContent = do_QueryInterface(GetRoot());
>+  nsIFrame *bodyFrame = ps->GetPrimaryFrameFor(bodyContent);
>+
>+  nsCOMPtr<nsIContent> content = do_QueryInterface(aElement);

Might be safer to take the strong ref before FlushPendingNotifications.

>+  nsIFrame* frame = ps->GetPrimaryFrameFor(content);
>+  nsIFrame* child = frame; // nsnull;

What's the comment about?

>+
>+  while (frame != bodyFrame) {
>+    nsIScrollableFrame* scrollable=nsnull;

Spaces around =

>+    CallQueryInterface(frame, &scrollable);
>+    if (scrollable)
>+      return child;

The comment for GetEditingUiParentFor seems to imply that this needs to return scrollable?

>+    child = frame;
>+    frame = frame->GetParent();
>+  }
>+
>+  return bodyFrame;

>+nsHTMLEditor::GetEditingUiParentFor(nsIDOMElement *aElement)
>+{
>+  nsIFrame * uiParent = GetEditingUiParentFrameFor(aElement);
>+  if (uiParent)
>+    return uiParent->GetContent();
>+
>+  // If for whatever reason we fail to get the Ui parent above,
>+  // just return the body content.
>+  nsCOMPtr<nsIPresShell> ps = do_QueryReferent(mPresShellWeak);
>+  if (!ps) return nsnull;
>+
>+  nsIDocument *document = ps->GetDocument();
>+  document->FlushPendingNotifications(Flush_Layout);

GetEditingUiParentFrameFor already flushed.

>+nsHTMLEditor::GetOffsetToUiParent(nsIDOMElement *aElement)
>+{
>+  nsCOMPtr<nsIPresShell> ps = do_QueryReferent(mPresShellWeak);
>+  if (!ps) return nsPoint(0,0);

Space after comma.

>+
>+  nsIDocument *document = ps->GetDocument();
>+  document->FlushPendingNotifications(Flush_Layout);
>+
>+  nsCOMPtr<nsIContent> absObjContent = do_QueryInterface(aElement);

Might be safer to take the strong ref before FlushPendingNotifications.

>+  nsIFrame *frame = GetBorderBoxFrame(ps->GetPrimaryFrameFor(absObjContent));
>+
>+  nsIFrame *uiParentFrame = GetEditingUiParentFrameFor(aElement);
>+  if (!uiParentFrame) {
>+    NS_WARNING("No editing UI frame");
>+    return nsPoint(0,0);

Space after comma.

>+  }
>+  nsIContent *parentContent = uiParentFrame->GetContent();
>+
>+  nsIFrame *ancestorFrame = nsnull;
>+
>+  nsCOMPtr<nsIContent> root = do_QueryInterface(GetRoot());
>+  if (parentContent == root)
>+    ancestorFrame = ps->GetPrimaryFrameFor(document->GetRootContent());

Why? Doesn't GetEditingUiParentFrameFor handle this case?

> nsHTMLEditor::GetPositionAndDimensions(nsIDOMElement * aElement,

>+  if (nsHTMLEditUtils::IsTableElement(mResizedObject))
>+  {
>+    // AbsPos tables CSS widths/heights include their borders and padding,
>+    // so use the 'outer' rect for the size.
>+    aInfo->mBorderBox = borderFrame->GetRect();

Just store the size here

> 
>-    aBorderLeft = 0;
>-    aBorderTop  = 0;
>-    aMarginLeft = 0;
>-    aMarginTop = 0;
>+  } else {
>+    // Non-tables's CSS widths/heights do not include borders/padding,
>+    // so the size is the content rect.
>+    aInfo->mBorderBox = borderFrame->GetContentRect();

and here

>+    
>+    // But we still need to correct for the border/pading when laying
>+    // out resizers, so remember the border/padding sizes here, else
>+    // resizers will not appear on the edge of the borders.
>+    nsMargin border = borderFrame->GetUsedBorderAndPadding();
>+    border = AppUnitsToPixels(border);
>+    aInfo->mBorderAndPaddingWidth = border.left + border.right;
>+    aInfo->mBorderAndPaddingHeight = border.top + border.bottom;
>+  }
>+
>+  // Position needs to be relative to the UI parent, which is the
>+  // closest scroll frame, else the resizers won't be positioned correctly
>+  // when they're in a scroll frame.
>+  nsPoint off = GetOffsetToUiParent(aElement);

and do
  aInfo->mBorderBox.MoveTo(off);
  aInfo->mBorderBox.SizeTo(size);

>+  aInfo->mBorderBox.x = off.x;
>+  aInfo->mBorderBox.y = off.y;
>+
>+  AppUnitsToPixels(aInfo->mBorderBox);
>+
>+  aInfo->mIsAbsPos = PR_FALSE;

No need to set this to PR_FALSE.

>+  nsresult res = aElement->HasAttribute(NS_LITERAL_STRING("_moz_abspos"), &aInfo->mIsAbsPos);

Long line.

> nsHTMLEditor::SetAnonymousElementPosition(PRInt32 aX, PRInt32 aY, nsIDOMElement *aElement)
> {
>-  mHTMLCSSUtils->SetCSSPropertyPixels(aElement, NS_LITERAL_STRING("left"), aX);
>-  mHTMLCSSUtils->SetCSSPropertyPixels(aElement, NS_LITERAL_STRING("top"), aY);
>+  // We pass true here to suppress the underlying txn so that no extra
>+  // transactions are added to the undo stack - else when we resize and
>+  // update the resizers' positions we'd be adding txns for the
>+  // repositioning of the resizers, and the resize txns wouldn't merge.
>+  mHTMLCSSUtils->SetCSSPropertyPixels(aElement, nsEditProperty::cssLeft, aX, PR_TRUE);
>+  mHTMLCSSUtils->SetCSSPropertyPixels(aElement, nsEditProperty::cssTop, aY, PR_TRUE);

Long lines.

>Index: editor/libeditor/html/nsHTMLEditor.cpp
>===================================================================

>+nsHTMLEditor::GetBorderBoxFrame(nsIFrame* aFrame)
>+{
>+  if (aFrame->GetType() == nsGkAtoms::tableOuterFrame) {
>+    // Must take the inner table cell so we get the position right
>+    nsIFrame* child = aFrame->GetFirstChild(nsnull);
>+    while (child && child->GetType() != nsGkAtoms::tableFrame) {
>+      child = child->GetNextSibling();
>+    }
>+    return child;
>+  }
>+  return aFrame;

Return early if aFrame->GetType() != nsGkAtoms::tableOuterFrame

> nsHTMLEditor::GetElementOrigin(nsIDOMElement * aElement, PRInt32 & aX, PRInt32 & aY)

>   if (!ps) return NS_ERROR_NOT_INITIALIZED;
>-
>-  nsCOMPtr<nsIContent> content = do_QueryInterface(aElement);
>-  nsIFrame *frame = ps->GetPrimaryFrameFor(content);
>-
>+  

Drop the spaces.

>Index: editor/libeditor/html/nsHTMLEditor.h
>===================================================================

>+  nsresult GetPositionAndDimensions(nsIDOMElement * aElement, nsElementGeometry *aInfo);

Long line.

>+  // Size in pixels of mResizedObject before its resize starts.
>+  nsSize mOriginalSize;

Is this different from the size of mOriginalGeometry? Maybe mention that in the comment?

>+  // (x,y,w,h) before we started to resize, in pixels.
>+  nsIntRect mOriginalGeometry;
>+
>+  // When a mouse-down on a resizable or grabbable object is detected, these
>+  // are set with the mouse client pixel coords. We then calculate the move or
>+  // resize as the differnece between the mouse and this saved value.

difference

>+  void SetAnonymousElementPosition(PRInt32 aX, PRInt32 aY, nsIDOMElement *aResizer);

Long line.

>Index: editor/libeditor/html/nsHTMLInlineTableEditor.cpp
>===================================================================

>@@ -71,37 +72,37 @@ nsHTMLEditor::ShowInlineTableEditingUI(n

>+  nsCOMPtr<nsIDOMElement> parentNode = do_QueryInterface(GetEditingUiParentFor(aCell));

Long line.

>@@ -109,47 +110,45 @@ nsHTMLEditor::ShowInlineTableEditingUI(n

>+  NS_ASSERTION(mInlineEditedCell, "Hiding inline table editing UI called more than once");

Long line.

>+  nsresult res = mAddColumnBeforeButton->GetParentNode(getter_AddRefs(parentNode));

Long line.

>@@ -224,31 +226,35 @@ nsHTMLEditor::RemoveMouseClickListener(n

>+  PRInt32 xCell=offset.x, yCell=offset.y;

Spaces around =

>+  PRInt32 xHoriz = offset.x + cellWidth/2;
>+  PRInt32 yVert  = offset.y + cellHeight/2;

Spaces around /

>Index: editor/libeditor/html/nsHTMLObjectRepositionTxn.cpp
>===================================================================

>+  mSetPosition = aMode & kSetPosition;
>+  mSetSize = aMode & kSetSize;

Why not take these as boolean arguments to Init?

>+
>+  // Backup old values for undo.
>+  if (mSetPosition) {
>+    mBeforeGeometry.x = mEditor->GetCSSFloatValue(mNode, NS_LITERAL_STRING("left"));
>+    mBeforeGeometry.y = mEditor->GetCSSFloatValue(mNode, NS_LITERAL_STRING("top"));

Long lines.

>+nsHTMLObjectRepositionTxn::Merge(nsITransaction *aTransaction, PRBool *aDidMerge)

Long line.

>+    aTransaction->QueryInterface(nsHTMLObjectRepositionTxn::GetCID(), getter_AddRefs(otherTxn));

Long line.

>+nsresult CreateHTMLObjectRepositionTxn(nsIDOMElement* aNode,
>+                                       PRInt32 aLeft,
>+                                       PRInt32 aTop,
>+                                       PRInt32 aWidth,
>+                                       PRInt32 aHeight,
>+                                       eRepositionMode aMode,
>+                                       nsHTMLEditor *aEditor,
>+                                       nsHTMLEditor::nsElementGeometry& aGeometry,
>+                                       nsHTMLObjectRepositionTxn ** aTxn)

Do we need this? I'd just move this code to the one caller.

>+  result = (*aTxn)->Init(aNode, aLeft, aTop, aWidth, aHeight, aMode, aEditor, aGeometry);

Long line.

>Index: editor/libeditor/html/nsHTMLObjectRepositionTxn.h
>===================================================================

>+  * A transaction that resizes and/or repositions an element. An 
>+  * nsHTMLObjectRepositionTxn will merge with subsequent repos txns provided

repositioning transactions

>+  * they're for the same node, and are both the same resize/repo/both

resize/repositioning

>+  nsHTMLObjectRepositionTxn(){}

No need for this.

>+  void SetAllowMerges(PRBool aAllowMerges) {mAllowMerges = aAllowMerges;}

Spaces after { and before }

>Index: editor/libeditor/html/nsHTMLObjectResizer.cpp
>===================================================================

>@@ -269,91 +289,93 @@ nsHTMLEditor::CreateResizingInfo(nsIDOME

>+  PRInt32 halfRW = (rw+1)/2;
>+  PRInt32 halfRH = (rh+1)/2;

Spaces

>+
>+  // Calculate where on edges we will put the resizers.
>+  PRInt32 leftX = x - 1;
>+  PRInt32 middleX = x + w/2 - halfRW;

Spaces

>+  PRInt32 rightX = x + w - rw - 1;

maybe leftX + w - rw?

>+
>+  PRInt32 topY = y - 1;
>+  PRInt32 middleY = y + h/2 - halfRH;
>+  PRInt32 bottomY = y + h - rw - 1;

maybe topY + h - rh?

>+  mOriginalSize = GetBorderBoxFrame(GetPrimaryFrameFor(mResizedObject))->GetSize();

Long line.

>+nsHTMLEditor::SaveOriginalGeometry(PRInt32 aClientX,
>+                                   PRInt32 aClientY,
>+                                   nsIDOMElement *aObject,
>+                                   nsHTMLEditor::nsElementGeometry& aGeometry)
>+{
>+  mOriginalMouseX = aClientX;
>+  mOriginalMouseY = aClientY;
>+  mOriginalGeometry.x = GetCSSFloatValue(aObject, NS_LITERAL_STRING("left"));
>+  mOriginalGeometry.y = GetCSSFloatValue(aObject, NS_LITERAL_STRING("top"));

Make a note why we can't use aGeometry.mBorderBox.x and y here

>+  mOriginalGeometry.width = aGeometry.mBorderBox.width;
>+  mOriginalGeometry.height = aGeometry.mBorderBox.height;

>+nsHTMLEditor::HideResizingInfo() {

Brace on new line.

>+  if (mResizingInfo)
>+    mResizingInfo->SetAttribute(NS_LITERAL_STRING("class"), NS_LITERAL_STRING("hidden"));

Long line.

> nsHTMLEditor::MouseUp(PRInt32 aClientX, PRInt32 aClientY,
>                       nsIDOMElement *aTarget)

>+  } else if (mIsMoving || mGrabberClicked) {

else on new line.

>+nsHTMLEditor::RefreshResizingInfoPosition()

>+  mHTMLCSSUtils->SetCSSPropertyPixels(mResizingInfo, nsEditProperty::cssTop,
>+                                      infoYPosition + mouseCursorOffset, PR_TRUE);

Long lines.

>+  PRInt32 diffWidth  = w + mResizedObjectInfo.mBorderAndPaddingWidth - mOriginalSize.width;
>+  PRInt32 diffHeight = h + mResizedObjectInfo.mBorderAndPaddingHeight - mOriginalSize.height;

Long lines

> nsHTMLEditor::GetNewResizingIncrement(PRInt32 aX, PRInt32 aY, PRInt32 aID)

>   float objectSizeRatio = 
>-              ((float)mResizedObjectWidth) / ((float)mResizedObjectHeight);
>+              ((float)(mOriginalSize.width) /
>+              ((float)(mOriginalSize.height)));

Too many braces

                (float)mOriginalSize.width / (float)mOriginalSize.height;

> nsHTMLEditor::GetNewResizingX(PRInt32 aX, PRInt32 aY)

>+  PRInt32 resized = mOriginalGeometry.x
>+                  + GetNewResizingIncrement(aX, aY, kX) * mXIncrementFactor;

Operators at end of line.

>+
>+  PRInt32 max = mResizedObjectInfo.mBorderBox.x + mResizedObjectInfo.mBorderBox.width;

Long line.

> nsHTMLEditor::GetNewResizingY(PRInt32 aX, PRInt32 aY)

>+  PRInt32 resized = mOriginalGeometry.y
>+                  + GetNewResizingIncrement(aX, aY, kY) * mYIncrementFactor;
>+
>+  PRInt32 max = mResizedObjectInfo.mBorderBox.y + mResizedObjectInfo.mBorderBox.height;

See above.

>+
>+  PRInt32 rv = PR_MIN(resized, max);
>+
>+  return rv;

Just |return PR_MIN(resized, max);|

> nsHTMLEditor::GetNewResizingWidth(PRInt32 aX, PRInt32 aY)

>+  PRInt32 resized = mOriginalGeometry.width
>+                  + GetNewResizingIncrement(aX, aY, kWidth) * mWidthIncrementFactor;

Operator at end of line and long line.

> nsHTMLEditor::GetNewResizingHeight(PRInt32 aX, PRInt32 aY)

>+  PRInt32 resized = mOriginalGeometry.height
>+                  + GetNewResizingIncrement(aX, aY, kHeight) * mHeightIncrementFactor;

See above.

>+DoRepositionTxn(nsIDOMElement *aObject,
>+                PRInt32 aX,
>+                PRInt32 aY,
>+                PRInt32 aWidth,
>+                PRInt32 aHeight,
>+                eRepositionMode aMode,

Two booleans here too.

> nsHTMLEditor::MouseMove(nsIDOMEvent* aMouseEvent)

>+  nsCOMPtr<nsIDOMMouseEvent> mouseEvent ( do_QueryInterface(aMouseEvent) );

No spaces around the braces.

>+    NS_ENSURE_SUCCESS(res,res);

Spaces after ,

> 
>-    SnapToGrid(newX, newY);
>+    // Move the object.
>+    nsCOMPtr<nsIContent> content = do_QueryInterface(mAbsolutelyPositionedObject);

Long line.

>+nsHTMLEditor::FinishRepositioning() {

Brace on new line.

>+  // If the top txn on the undo stack is a reposition txn, set it to
>+  // not merge with any subsequent reposition txns.
>+  nsRefPtr<nsHTMLObjectRepositionTxn> reposTxn = GetUndoStackTopAsRepositionTxn(mTxnMgr);

Long line.

>+nsHTMLEditor::NotifyEndResizing()

>+  for (index = 0; index < listenersCount; index++) {

++index

>Index: layout/style/contenteditable.css
>===================================================================

>@@ -231,16 +220,18 @@ span[\_moz_anonclass="mozGrabber"] {

>+  background-color: white;

Do we really want to do this? Seems the background-image should be enough?

Comment 44

[Joel Lehtonen]

There is a related bug 466650 where misalignment occurs when caption is on top of a table.

Comment 45

[Joel Lehtonen]

Attachment: Alignment problem of tables with a caption

Comment 46

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v4

Updated patch! Fixed review comments. Carrying forward peterv's r+.

* As v3, but updated to trunk.
* Editor mochitests pass on Windows and Linux. I will try to test on Mac, and will report back if any tests fail.
* There's still a problem, in that if you resize an abspos element smaller than its intrinsic size, it will move rather than resize. I think we should leave that fix for another bug. We should at least get this patch in, as it's been broken for over a year, and this at least gives us something that works.
* All peterv's formatting/style comments dealt with.

Response to peterv's comments:

(In reply to comment #43)
> >Index: editor/libeditor/html/nsHTMLAbsPosition.cpp
> >+  PRInt32 grabberX = mPositionedObjectInfo.mBorderBox.x
> >+                   + grabberWidth;
> >+
> >+  PRInt32 grabberY = mPositionedObjectInfo.mBorderBox.y
> >+                   - grabberHeight
> >+                   - 2; // _moz_abspos outline width.
> 
> Operators at end of line.
> Can this end up with grabberY < 0?

Yes. In this case the grabber is hidden by the tab bar. The grabber's z-index is already 2147483647, but that's not enough for it to spill out of the content area to be drawn on top of the tab bar. Not much we can do about that I think.

> The comment for GetEditingUiParentFor seems to imply that this needs to return
> scrollable?

Updated comment. We now return nsIScrollableFrame::GetScrolledFrame(), which is equivalent to the previous behaviour.

> >Index: editor/libeditor/html/nsHTMLEditor.h
> >===================================================================
> >+  // Size in pixels of mResizedObject before its resize starts.
> >+  nsSize mOriginalSize;
> 
> Is this different from the size of mOriginalGeometry? Maybe mention that in the
> comment?

It turns out they store equivalent info, I've removed mOriginalSize.

> >Index: layout/style/contenteditable.css
> >===================================================================
> 
> >@@ -231,16 +220,18 @@ span[\_moz_anonclass="mozGrabber"] {
> 
> >+  background-color: white;
> 
> Do we really want to do this? Seems the background-image should be enough?

Yes. With "padding: 2px;" style on the grabber, the background image of the grabber icon doesn't quite fill the grabber box, so the background is visible. If we don't set the color to something, the padding is transparent, and it looks funny. I think we should leave the padding there, the grabber is 12 pixels wide, and without the padding, it's quite small.

Comment 47

[Chris Pearce [:cpearce (Not reading bugmail)]]

Damn it. Tests don't pass on Mac OS 10.4.

Comment 48

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v5 - tests pass on all platforms

As v4, but fixed tests on Mac. Problem was that test relied on getBoundingClientRect() to verify that a resize had worked, and on mac that returns float (subpixel?) values, and rounding errors were causing failure. I've made the tests fuzzy now.

Editor mochitests pass on Mac, Windows, and Linux.

Comment 49

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 368354 [details] [diff] [review]
Patch v5 - tests pass on all platforms

Just style nits:

+  nsIntRect mBeforeGeometry;
+  nsIntRect mAfterGeometry;

Document what these are and what they're relative to.

+  // Flags whether this txn will allow other txns to merge with it.
+  // True by default. This should be disabled upon mouse-up.
+  PRBool mAllowMerges;
+
+  // Does this transaction set the position, size, or both?
+  PRBool mSetPosition;
+  PRBool mSetSize;

These should be PRPackedBool.

+    if (mResizedObjectInfo.mIsAbsPos)
+      SetPositionAndSize(mResizedObject, newX, newY,
+                         newWidth, newHeight, mResizedObjectInfo);
+    else
+      SetSize(mResizedObject, newWidth, newHeight, mResizedObjectInfo);

{} around these statements

+    if (topTxn)
+      topTxn->QueryInterface(nsHTMLObjectRepositionTxn::GetCID(),
+                             (void **)&reposTxn);

Here too

+  if (reposTxn)
+    reposTxn->SetAllowMerges(PR_FALSE);

And here

+    // Is this object absolutely positioned.
+    PRBool mIsAbsPos;

PRPackedBool

+  nsIContent* GetEditingUiParentFor(nsIDOMElement *aElement);

GetEditingUIParentFor

+  nsIFrame* GetEditingUiParentFrameFor(nsIContent *aContent);

GetEditingUIParentFrameFor

+  // Returns the offset from aElement's frame to its UI editing parent's
+  // absolute containing block.
+  nsPoint GetOffsetToUiParent(nsIDOMElement *aElement);

Should be called GetOffsetToUIParentAbsPosContainingBlock perhaps?

Comment 50

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v6

With Roc's nit's picked... Tests pass on Mac, Windows, and Linux.

Comment 51

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  // The geometry of mNode before resize. This is:
+  // {css left, css top, border box width, border box height}.

Doesn't say what "CSS left" and "CSS top" are relative to.

Comment 52

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to comment #51)
> +  // The geometry of mNode before resize. This is:
> +  // {css left, css top, border box width, border box height}.
> 
> Doesn't say what "CSS left" and "CSS top" are relative to.

I don't understand why you want this... CSS left is defined by the CSS spec to be relative to the element's containing block. We have no control over what it is relative to, so why is it noteworthy?

Comment 53

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 368811 [details] [diff] [review]
Patch v6

OK, just put 'left' and 'top' in single quotes to make it more obvious they're property names.

Comment 54

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v7

As v6. As requested by Roc, now the comment is:

+  // The geometry of mNode before resize. This is:
+  // {CSS 'left', CSS 'top', border box width, border box height}.

r+ peterv, sr+ roc.

Comment 55

[Chris Pearce [:cpearce (Not reading bugmail)]]

Hmm... Mochitest failed on TryServer, better not check in just yet...

Linux:
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1237778375.1237786546.9347.gz
Win32:
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1237778375.1237786001.8605.gz

OsX passed.

Comment 59

[defender]

What the hell is taking so long to get this fixed??

Comment 60

[William Woodward]

Ditto previous comment. Advanced web2.0 apps offering rich interfaces require that this bug get fixed ASAP.

Comment 61

[Chris Pearce [:cpearce (Not reading bugmail)]]

I haven't had time to finish this off, so if someone wants to take my patch and work on it, that would be fine with me. The existing patch almost fixes the problem, the tests didn't pass reliably lastime I rebased it though.

Comment 62

[defender]

If you don't get it working then remove this feature or provide a more open api so the user/developer can fix it. 

It is a pain in the neck for every user. As I have no hope in Mozilla to get this fixed in short time. We are starting developement on a plugin for tinymce to fix this stupid issue by placing our own "resizers".

As William said rich interfaces in web2.0 require this to be fixed ASAP!!!

Comment 63

[Chris Pearce [:cpearce (Not reading bugmail)]]

Reassigning to Ehsan, he's Mr Editor now. Ehsan, the patch is almost certainly bitrotten, but mostly worked last time I tried it. I had trouble getting the tests it adds to pass though.

Comment 64

[Spocke]

Any chance of this getting fixed for the Firefox 4.0 final? It's been in there for a couple of years. :)

Comment 65

[(no longer active)]

Attachment: Unbitrotted patch

Unbitrotted version of attachment 368831 [details] [diff] [review], which seems to compile.  Posted to the try server, but haven't tried it manually at all yet.

Comment 66

[(no longer active)]

Here are the failure logs based on the try server run, for future reference.  <https://people.mozilla.com/~eakhgari/399046-logs/>

Comment 67

[Martijn Wargers (dead)]

Were the failures caused by the patch?

Comment 68

[Spocke]

There are a few depending bugs: #265568, #575752

It would be nice to see this fixed in the near future since it's now nearly 4 years old and we still see a lot of complaints regarding this issue. :(

Comment 69

[Martijn Wargers (dead)]

Spocke, if you could update the latest patch to current trunk, that would be great! You could also look into the failures mentioned in comment 66.

From the failure logs:
The Mo3 and Md3 files show similar failures in test_bug399046-2.html .
That test is part of the patch, so there might be something wrong with the test.

The Cd logs:
REFTEST TEST-UNEXPECTED-FAIL | file:///e:/builds/moz2_slave/tryserver-win32-debug-unittest-crashtest/build/reftest/tests/editor/libeditor/html/crashtests/431086-1.xhtml | assertion count 10 is more than expected 0 assertions

Here is the crashtest file:
http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/html/crashtests/431086-1.xhtml

Comment 70

[Spocke]

Seems to be working fine now in the latest nightly. So I think this one can be closed. The caption bug #466650 is still happening though.

Could you add these two bugs as duplicates:
https://bugzilla.mozilla.org/show_bug.cgi?id=649632
https://bugzilla.mozilla.org/show_bug.cgi?id=575752

And a depend on this one:
https://bugzilla.mozilla.org/show_bug.cgi?id=265568

Thanks

Comment 71

[Alin Ilea, Desktop Test Engineering]

Based on comment 70, I'll close this as Resolved > Worksforme, also since the last real activity on this issue was 10 years ago and it might not be relevant anymore. Feel free to re-open if the issue is still reproducible on your end in the latest FF versions.