364719 - Resizers of images are positioned wrongly when page is scrolled

Status: RESOLVED FIXED

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

Description

[Martijn Wargers (dead)]

See upcoming testcase, the image resizers you seen in editor is not correctly positioned anymore when the page is scrolled.

This regressed between 2006-03-08 and 2006-03-12, I think a regression of bug 328881.
It seems to me this function is to blame:
http://lxr.mozilla.org/seamonkey/source/editor/libeditor/html/nsHTMLEditor.cpp#5699

The testcase also has an absolutely positioned image in a scrollframe, this also doesn't work well in older builds, but maybe that can be fixed too, while you're fixing this bug?

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase

Comment 2

[Mathieu Fenniak]

Attachment: proposed patch

The proposed patch addresses this bug by changing the code used to determine an editor element's position.  The code is modified to return, rather than the screen position, the position relative to the element's parent (ie. the css left/top of the element).  That modification is in nsHTMLEditor::GetElementOrigin.

The rest of the patch modifies the positioning of the element resizers and the element absolute positioning grabber.  Those elements are modified to be siblings of the element being affected, rather than children of the document root, which allows them to be positioned easily and correctly.

Comment 3

[Peter Van der Beken [:peterv]]

Comment on attachment 262764 [details] [diff] [review]
proposed patch

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

>@@ -342,21 +342,23 @@ nsHTMLEditor::ShowGrabberOnElement(nsIDO

>+  nsCOMPtr<nsIDOMNode> parentNode;
>+  res = aElement->GetParentNode(getter_AddRefs(parentNode));
>+  NS_ENSURE_SUCCESS(res, res);
>+
>+  res = CreateGrabber(parentNode, getter_AddRefs(mGrabber));
>+  NS_ENSURE_SUCCESS(res, res);
> 
>-  res = CreateGrabber(rootElement, getter_AddRefs(mGrabber));

Don't you need to change HideGrabber too?

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

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

>+	nsresult rv;
>+    rv = htmlElement->GetOffsetLeft(&aX);
>+	NS_ENSURE_SUCCESS(rv, rv);
>+    rv = htmlElement->GetOffsetTop(&aY);
>+	NS_ENSURE_SUCCESS(rv, rv);

No tabs please.

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

> nsHTMLEditor::ShowResizers(nsIDOMElement *aResizedElement)

>+  // the resizers and the shadow will be anonymous siblings of the element
>+  nsresult res;
>+  nsCOMPtr<nsIDOMNode> parentNode;
>+  res = aResizedElement->GetParentNode(getter_AddRefs(parentNode));
>+  NS_ENSURE_SUCCESS(res, res);

Same here, don't you need to update HideResizers?

>+nsHTMLEditor::SetResizingInfoPosition(PRInt32 /* aX */, PRInt32 /* aY */, PRInt32 aW, PRInt32 aH)

Please change the function signature.

>+  mHTMLCSSUtils->SetCSSPropertyPixels(mResizingInfo, leftStr,
>+                                      (PRInt32)left + mInfoXIncrement);
>+  mHTMLCSSUtils->SetCSSPropertyPixels(mResizingInfo, topStr,
>+                                      (PRInt32)top + mInfoYIncrement);

Can you remove mInfoXIncrement and mInfoYIncrement?

Comment 4

[Mathieu Fenniak]

Attachment: updated patch

Updated patch based upon feedback.  This updated patch also adjusts the position of the resizing info "tooltip" better -- the previous patch displayed the tooltip in a fixed position, but this patch displays it on the corner of the element being resized.

Comment 5

[Peter Van der Beken [:peterv]]

Sorry for the late reply.

(In reply to comment #4)
> Updated patch based upon feedback.  This updated patch also adjusts the
> position of the resizing info "tooltip" better -- the previous patch displayed
> the tooltip in a fixed position, but this patch displays it on the corner of
> the element being resized.

It does seem to always set the tooltip to the right of and below the grabber? Did it used to always put the tooltip outside of the element (eg to the left of grabbers on the leftside or above grabbers on the top)? If so, would it be hard to keep that behaviour?

Comment 6

[Mathieu Fenniak]

Peter, on IRC you mentioned that "it seems nice to not have the tooltip overlap with the object you're resizing", referring to the old behavior the info tooltip had.  However, the old behavior for the info tooltip was actually to always overlap the object -- if you grabbed the top-left resizer, the tooltip would appear at the bottom-right of the mouse cursor; if you grabbed the bottom-right resizer, the tooltip would appear at the top-left of the mouse cursor.  To the extend that it worked properly, at least.

My personal preference would be the consistent bottom-right-of-grabber approach that this patch takes -- but I am willing to modify the patch if it's a barrier to acceptance.

Comment 7

[Peter Van der Beken [:peterv]]

Comment on attachment 268358 [details] [diff] [review]
updated patch

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

>@@ -5700,45 +5701,26 @@ nsHTMLEditor::CopyLastEditableChildStyle

>+  if (htmlElement)
>+  {

Move the brace to the same line as the if.

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

>@@ -424,69 +425,69 @@ nsHTMLEditor::HideResizers(void)

>+  nsCOMPtr<nsIContent> parentContent;
>+  parentContent = do_QueryInterface(parentNode);

Combine these two lines.

>@@ -691,72 +677,61 @@ nsHTMLEditor::SetResizeIncrements(PRInt3

>+  // Determine the position of the resizing info box based upon the new
>+  // position and size of the element (aX, aY, aW, aH), and the which

s/the which/which/

>+  // resizer is the "activated handle".  For example, place the resizing
>+  // info box at the bottom-right corner of the new element, if the element
>+  // is being resized by the bottom-right resizer.
>+  PRInt32 infoXPosition;
>+  PRInt32 infoYPosition;
> 
>-  NS_NAMED_LITERAL_STRING(rightStr, "right");
>+  if (mActivatedHandle == mTopLeftHandle || mActivatedHandle == mLeftHandle ||
>+    mActivatedHandle == mBottomLeftHandle)

Line up the two mActivatedHandle (also further down).

>+  {

Opening braces should be on same line as the if or else (here and further down).

>+  mHTMLCSSUtils->SetCSSPropertyPixels(mResizingInfo, leftStr, (PRInt32)infoXPosition + 20);
>+  mHTMLCSSUtils->SetCSSPropertyPixels(mResizingInfo, topStr, (PRInt32)infoYPosition + 20);

Long lines.
Why do you need to cast to PRInt32?

Comment 8

[Mathieu Fenniak]

Attachment: updated patch w/ review comments addressed

Updated patch to address style comments in review.

Comment 9

[Mathieu Fenniak]

Comment on attachment 272848 [details] [diff] [review]
updated patch w/ review comments addressed

Roc, Boris suggested you might be a good sr for this  patch.  If you have the time, I would appreciate it.

Comment 10

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

+  nsCOMPtr<nsIDOMNSHTMLElement> htmlElement(do_QueryInterface(aElement));
+  if (htmlElement) {
+    nsresult rv;
+    rv = htmlElement->GetOffsetLeft(&aX);
+    NS_ENSURE_SUCCESS(rv, rv);
+    rv = htmlElement->GetOffsetTop(&aY);
+    NS_ENSURE_SUCCESS(rv, rv);

What are aX and aY supposed to be relative to? the offsetX and offsetY properties are weird and broken; I'd rather not use there anywhere we don't have to.

Other than that it looks good.

Comment 11

[Mathieu Fenniak]

aX and aY are supposed to be relative to htmlElement's container.  ie. if htmlElement had a sibling with the same offsetLeft and offsetTop, they would be in the same position.  This expectation matches the behavior of GetOffsetLeft/Top (they both use GetOffsetRect, which bases its calculations on the parent frame).

Previously, GetElementOrigin would return different values based upon the element's positioning (if I remember correctly).  This new behavior is easier to handle.  A sibling element with { position: relative; left: $aX; right: $aY } will always fall on the top-left corner of htmlElement.

Comment 12

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

which container? The nearest CSS-postioned ancestor? Because GetOffsetRect can return a rect that's relative to the nearest table cell, when htmlElement is not itself positioned. Is that what you really want?

Comment 13

[Mathieu Fenniak]

A quick test with the element being resized inside a table shows a pretty ugly failure of this patch.  GetOffsetRect definitely doesn't work like I thought it would.  Robert, thanks for catching this.  I was pretty confident in this patch, but only due to ignorance. ;-)

I'll try to come up with an approach that a) works better, and b) I understand better.

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Any progress on this?  Would be nice to fix for 1.9.

Comment 15

[spam]

I think this one need to be blocking for the 1.9 release since it breaks existing midas implementations like TinyMCE or FCKEditor this bug was not there in the Firefox 2.0 version.

I've made an example on how it breaks a simple editable area with some paragraphs in it.

http://tinymce.moxiecode.com/gecko/resize_handles2.htm

Comment 16

[Damon Sicore (:damons)]

+'ing with P3.

Comment 17

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

Attachment: Patch - Mathieu's using a variant off GetOffsetRect()

This is pretty much Mathieu's patch, but it uses an adapted version of GetOffsetRect() which doesn't stop early on table cells, it stops at the first positioned frame. I tried using nsIFrame::GetOffsetTo(), but it didn't work in all cases, so I had to roll my own... This patch works inside a table cell, scroll pane, bare document, and div.

Comment 18

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

Why didn't GetOffsetTo work?

Seems to me the thing to do here is to make nsCSSFrameConstructor::GetAbsoluteContainingBlock public, and call it via presShell->FrameConstructor(), check for null, and then use GetOffsetTo.

Comment 19

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

Attachment: Patch - GetAbsoluteContainingBlock() + GetOffsetTo()

* As previous patch, but uses GetAbsoluteContainingBlock() + GetOffsetTo() to implement  nsHTMLEditor::GetElementOrigin().
* Added BeginUpdate()/EndUpdate() to nsHTMLEditor::DeleteRefToAnonymousNode() to stop an assertion failing when Hide{Grabbers,Resizers}() called.
* Changed the loop in nsHTMLEditor::GetAbsolutelyPositionedSelectionContainer() to stop it failing when it was called for an HTML element, triggered when double-clicking on resizers.

Comment 20

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

+  nsCSSFrameConstructor* fc = FrameConstructor();
+  return fc ? fc->GetAbsoluteContainingBlock(aFrame) : nsnull;

FrameConstructor() can't return null (generally getters without a Get can't return null)

+  nsIFrame *container = ps->GetAbsoluteContainingBlock(frame);
+  nsPoint off = frame->GetOffsetTo(container);

frame really should be null-checked here. I don't know what happens when people try to edit display:none content but we shouldn't crash.

All this 8-fold code for manipulating resizers should be replaced with loops over arrays, but that's for another day.

Comment 21

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

oh, and can you document that GetElementOrigin actually returns its position relative to its nearest absolute containing block?

Comment 22

[Robert Stopp (textrinum)]

Maybe a stupid question: What would be the javascript-way to avoid poping up the resizers?

Comment 23

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

(In reply to comment #22)
> Maybe a stupid question: What would be the javascript-way to avoid poping up
> the resizers?
> 

If an image is contenteditable, it will have resizers. The only way to not have resizers on an image is to have the image not be contenteditable. You could set contenteditable="false" on the image and have its siblings and container as contenteditable="true". That may not be the behaviour you want though...

Comment 24

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

Attachment: Patch - with Roc's changes

As previous with requested changes.

Comment 25

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

+  nsIFrame *container = ps->GetAbsoluteContainingBlock(frame);
+  if (!frame) return NS_ERROR_NULL_POINTER;

I don't think that's the right thing to return just because the content doesn't have a frame. You probably need to return 0,0 or something.

Hmm, GetElementOrigin should probably return floats here so that we can get subpixel positioning correct. Another for another day :-(

Comment 26

[Robert Stopp (textrinum)]

@Chris I mean the way googlepages goes with tables. I can´t see a contenteditable attribute in the dom via Firebug, but maybe thats my job to find out.

Another question (feature request): At this time resizers and movers convert every resize/move act into pixels. Is it possible to keep the original scale units (percent, ems, etc.)? Example: Positioned floating divs in a website template.
Nearly same with tables: In my opinion it would be better to clone a table row with all td attributes (exept rowspan and maybe colspan) if the "add row" element is clicked.
And last: Objects, Embeds and Iframes could also have resizers.

I hope not to much days ;-)
Thanks,
Robert

Comment 27

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

(In reply to comment #25)
> +  nsIFrame *container = ps->GetAbsoluteContainingBlock(frame);
> +  if (!frame) return NS_ERROR_NULL_POINTER;
> 
> I don't think that's the right thing to return just because the content doesn't
> have a frame. You probably need to return 0,0 or something.
> 

Ok, I already was setting the reutrn value to 0,0 when I enter the function, so you're saying that we should return NS_OK instead of NS_ERROR_NULL_POINTER when there's no frame, i.e.:

nsresult
nsHTMLEditor::GetElementOrigin(nsIDOMElement * aElement, PRInt32 & aX, PRInt32 & aY)
 {
  aX = 0;
  aY = 0;

  if (!mPresShellWeak) return NS_ERROR_NOT_INITIALIZED;
  nsCOMPtr<nsIPresShell> ps = do_QueryReferent(mPresShellWeak);
  if (!ps) return NS_OK;
 
  nsCOMPtr<nsIContent> content = do_QueryInterface(aElement);
  nsIFrame *frame = ps->GetPrimaryFrameFor(content);
 
  nsIFrame *container = ps->GetAbsoluteContainingBlock(frame);
  if (!frame) return NS_ERROR_NULL_POINTER;
  nsPoint off = frame->GetOffsetTo(container);
  aX = nsPresContext::AppUnitsToIntCSSPixels(off.x);
  aY = nsPresContext::AppUnitsToIntCSSPixels(off.y);
 
  return NS_OK;
}
 

Comment 28

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

  if (!frame) return NS_ERROR_NULL_POINTER;

return NS_OK here, right

Comment 29

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

Attachment: Patch with GetOffsetTo() null check changed

(In reply to comment #28)
>   if (!frame) return NS_ERROR_NULL_POINTER;
> 
> return NS_OK here, right
> 

Oops, yeah that's what I meant. This new patch is identical to the previous except that line 259 now reads:

+  if (!frame) return NS_OK;

It was previously:

+  if (!frame) return NS_ERROR_NULL_POINTER;

Otherwise identical to previous.

Comment 30

[Reed Loden [:reed]]

Checking in editor/libeditor/html/nsHTMLAbsPosition.cpp;
/cvsroot/mozilla/editor/libeditor/html/nsHTMLAbsPosition.cpp,v  <--  nsHTMLAbsPosition.cpp
new revision: 1.14; previous revision: 1.13
done
Checking in editor/libeditor/html/nsHTMLAnonymousUtils.cpp;
/cvsroot/mozilla/editor/libeditor/html/nsHTMLAnonymousUtils.cpp,v  <--  nsHTMLAnonymousUtils.cpp
new revision: 1.17; previous revision: 1.16
done
Checking in editor/libeditor/html/nsHTMLEditor.cpp;
/cvsroot/mozilla/editor/libeditor/html/nsHTMLEditor.cpp,v  <--  nsHTMLEditor.cpp
new revision: 1.564; previous revision: 1.563
done
Checking in editor/libeditor/html/nsHTMLEditor.h;
/cvsroot/mozilla/editor/libeditor/html/nsHTMLEditor.h,v  <--  nsHTMLEditor.h
new revision: 1.239; previous revision: 1.238
done
Checking in editor/libeditor/html/nsHTMLObjectResizer.cpp;
/cvsroot/mozilla/editor/libeditor/html/nsHTMLObjectResizer.cpp,v  <--  nsHTMLObjectResizer.cpp
new revision: 1.31; previous revision: 1.30
done
Checking in layout/base/nsCSSFrameConstructor.h;
/cvsroot/mozilla/layout/base/nsCSSFrameConstructor.h,v  <--  nsCSSFrameConstructor.h
new revision: 1.256; previous revision: 1.255
done
Checking in layout/base/nsIPresShell.h;
/cvsroot/mozilla/layout/base/nsIPresShell.h,v  <--  nsIPresShell.h
new revision: 3.219; previous revision: 3.218
done
Checking in layout/base/nsPresShell.cpp;
/cvsroot/mozilla/layout/base/nsPresShell.cpp,v  <--  nsPresShell.cpp
new revision: 3.1082; previous revision: 3.1081
done

Comment 31

[spam]

I've noticed that the selection in contentEditable elements doesn't work as before I don't know if this has anything to do with this bug/fix but it wasn't present before this bug was resolved but it's present now in the latest snapshot so it's a hunch that these two issues are related.

I added a separate bug report for the new selection issue:
https://bugzilla.mozilla.org/show_bug.cgi?id=413678

Comment 32

[Boris Zbarsky [:bzbarsky]]

Chris, why are the null-checks for |document| needed in nsHTMLEditor::DeleteRefToAnonymousNode?  And more importantly, why use GetDocument() on the editor instead of aShell->GetDocument(), or content->GetCurrentDoc(), which would make all the observer calls use the same document?

Comment 33

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

(In reply to comment #32)
> Chris, why are the null-checks for |document| needed in
> nsHTMLEditor::DeleteRefToAnonymousNode? 

The null checks were added because DeleteRefToAnonymousNode() was being called by HideInlineTableEditingUI() multiple times. I've prevented this double-call in my patch for bug 399046. I was going to leave them in to be paranoid. How about I an assertion that the document isn't null?

> And more importantly, why use
> GetDocument() on the editor instead of aShell->GetDocument(), or
> content->GetCurrentDoc(), which would make all the observer calls use the same
> document?

That's a valid point, though I had assumed that an HTMLEditor would only edit its own document, and also that the presshells and doucments would match up... Can that not be the case?