Closed Bug 364719 Opened 15 years ago Closed 14 years ago

Resizers of images are positioned wrongly when page is scrolled

Categories

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

x86
Windows XP
defect

Tracking

()

RESOLVED FIXED
mozilla1.9beta3

People

(Reporter: martijn.martijn, Assigned: cpearce)

References

Details

(Keywords: regression, testcase)

Attachments

(2 files, 6 obsolete files)

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?
Attached file testcase
QA Contact: editor
Attached patch proposed patch (obsolete) — Splinter Review
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.
Attachment #262764 - Flags: superreview?(peterv)
Attachment #262764 - Flags: review?(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?
Attachment #262764 - Flags: superreview?(peterv)
Attachment #262764 - Flags: review?(peterv)
Attachment #262764 - Flags: review-
Attached patch updated patch (obsolete) — Splinter Review
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.
Attachment #262764 - Attachment is obsolete: true
Attachment #268358 - Flags: review?(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?
Assignee: nobody → mfenniak-moz
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 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?
Attachment #268358 - Flags: review?(peterv) → review+
Updated patch to address style comments in review.
Attachment #272848 - Flags: superreview?
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.
Attachment #272848 - Flags: superreview? → superreview?(roc)
+  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.
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.
Status: NEW → ASSIGNED
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?
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.
Attachment #268358 - Attachment is obsolete: true
Attachment #272848 - Attachment is obsolete: true
Attachment #272848 - Flags: superreview?(roc)
Flags: blocking1.9?
Any progress on this?  Would be nice to fix for 1.9.
Flags: blocking1.9? → blocking1.9-
Whiteboard: [wanted-1.9]
Blocks: 399046
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
Flags: blocking1.9- → blocking1.9?
Assignee: mfenniak-moz → nobody
Status: ASSIGNED → NEW
+'ing with P3.
Flags: blocking1.9? → blocking1.9+
Priority: -- → P3
Assignee: nobody → chris
Whiteboard: [wanted-1.9]
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.
Attachment #297452 - Flags: review?(roc)
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.
* 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.
Attachment #297452 - Attachment is obsolete: true
Attachment #298157 - Flags: review?(roc)
Attachment #297452 - Flags: review?(roc)
+  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.
oh, and can you document that GetElementOrigin actually returns its position relative to its nearest absolute containing block?
Maybe a stupid question: What would be the javascript-way to avoid poping up the resizers?
(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...
Attached patch Patch - with Roc's changes (obsolete) — Splinter Review
As previous with requested changes.
Attachment #298157 - Attachment is obsolete: true
Attachment #298189 - Flags: review?(roc)
Attachment #298157 - Flags: review?(roc)
+  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 :-(
@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
(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;
}
 
  if (!frame) return NS_ERROR_NULL_POINTER;

return NS_OK here, right
(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.
Attachment #298189 - Attachment is obsolete: true
Attachment #298349 - Flags: review?(roc)
Attachment #298189 - Flags: review?(roc)
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
Status: NEW → RESOLVED
Closed: 14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9 M11
No longer blocks: 399046
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
Depends on: 413712
Flags: in-testsuite?
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?
Blocks: 406115
(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?
You need to log in before you can comment on or make changes to this bug.