Last Comment Bug 462758 - elements with contenteditable=true and position:absolute can be moved around the page
: elements with contenteditable=true and position:absolute can be moved around ...
Status: RESOLVED FIXED
: html5, testcase
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: mozilla9
Assigned To: Fabien Cazenave [:kaze]
:
:
Mentors:
: 498518 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-11-02 08:49 PST by Louis-Rémi BABE
Modified: 2014-05-06 09:14 PDT (History)
9 users (show)
ehsan: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
demonstration of the bug (709 bytes, text/html)
2008-11-02 08:57 PST, Louis-Rémi BABE
no flags Details
patch proposal (1.94 KB, patch)
2011-08-23 10:07 PDT, Fabien Cazenave [:kaze]
no flags Details | Diff | Splinter Review
patch proposal (4.76 KB, patch)
2011-08-25 01:58 PDT, Fabien Cazenave [:kaze]
no flags Details | Diff | Splinter Review
patch proposal (5.57 KB, patch)
2011-08-25 03:46 PDT, Fabien Cazenave [:kaze]
ehsan: review+
Details | Diff | Splinter Review
patch proposal (5.55 KB, patch)
2011-09-07 08:02 PDT, Fabien Cazenave [:kaze]
ehsan: review+
Details | Diff | Splinter Review
patch proposal (5.55 KB, patch)
2011-09-07 08:33 PDT, Fabien Cazenave [:kaze]
no flags Details | Diff | Splinter Review
patch proposal (5.54 KB, patch)
2011-09-07 08:35 PDT, Fabien Cazenave [:kaze]
no flags Details | Diff | Splinter Review

Description Louis-Rémi BABE 2008-11-02 08:49:26 PST
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; fr; rv:1.9.0.3) Gecko/2008101315 Ubuntu/8.10 (intrepid) Firefox/3.0.3
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; fr; rv:1.9.0.3) Gecko/2008101315 Ubuntu/8.10 (intrepid) Firefox/3.0.3

The new contenteditable feature implemented if firefox3 has an unexpected behavior on absolut positionned elements: when enabled (contenteditable="true"), those elements become draggable and resizable on the entire page.
Therefore it is not only the content of this element that is editable, but some properties of the element itself (both position and dimensions).
This behavior is specific to mozilla's implementation of contenteditable. It is not featured in the original IE's implentation, opera nor webkit implementation. It is not described in the WHATWg draft either: http://www.whatwg.org/specs/web-apps/current-work/#contenteditable
This behavior should be removed.

Reproducible: Always

Steps to Reproduce:
1. open an html document containing an element with position: absolute and contenteditable="true"
2. click on this element
Actual Results:  
you can drag and resize the element in the page

Expected Results:  
You should only be able to edit the _content_ of this element.
You should be able to drag and resize absolute positioned element only _inside_ this element.
You shouldn't be able to drag and resize the element itself
Comment 1 Louis-Rémi BABE 2008-11-02 08:57:18 PST
Created attachment 345949 [details]
demonstration of the bug
Comment 2 Ahmad Syukri 2009-03-29 00:10:17 PDT
This particular feature probably has its roots in Composer's layer implementation. Now that contenteditable has become a part of proposed specification, Mozilla probably should look to make this behaviour Mozilla-specific and disabled by default.

Currently, this can be worked around by setting the absolute-positioned element's contenteditable to false, putting the contents of the element into a seperate container element and setting the container's contenteditable to true. It should have no side effect on other browsers.
Comment 3 Carlos Alén Silva 2009-09-23 16:25:15 PDT
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.3) Gecko/20090824 Firefox/3.5.3 GTB5

Confirmed in Firefox 3.5.3 on Win XP SP 3.
Comment 4 Nickolay_Ponomarev 2010-04-19 14:27:38 PDT
Same as bug 498518?
Comment 5 Evertjan Garretsen 2011-06-25 14:35:32 PDT
Still not resolved. Confirmed in Firefox 5.0 using Ubuntu linux.

Absolute positioned elements with attribute contentEditable can be moved across the page. Absolute positioned elements with setting contentEditable can also be resized. There is no, non workaround, way to disable this. At least for the resizing part there is a javascript solution: document.execCommand("enableObjectResizing", false, false);

An icon is shown to indicate the possibility of moving the element. Setting the new HTML5 attribute draggable to false (for example: "<article draggable="false" contenteditable="true" style="position:absolute;height:500px;height:200px;">testcontent</div>") has no effect, the element is still movable.

Please look at this, this is not expected behaviour i agree with Louis-Rémi BABE

In the meantime i will try the workaround provided above. PS this bug was really hard to find in google, searched for an hour, so i hope that i added some keywords to make this bug easier to find.
Comment 6 Fabien Cazenave [:kaze] 2011-08-23 09:08:30 PDT
*** Bug 498518 has been marked as a duplicate of this bug. ***
Comment 7 Fabien Cazenave [:kaze] 2011-08-23 10:07:33 PDT
Created attachment 555122 [details] [diff] [review]
patch proposal

Not ready for review, I still have to write the tests.

Ehsan: given the number of such bugs, should I add an optional boolean argument to these two functions (false by default) that would allow to check that the node is a strict descendant of the active editing host?
Comment 8 Fabien Cazenave [:kaze] 2011-08-23 10:23:15 PDT
oops. By these two functions I meant:
  nsHTMLEditor::IsModifiableNode(nsIDOMNode *aNode)
  nsHTMLEditor::IsNodeInActiveEditor(nsIDOMNode* aNode)
Comment 9 Fabien Cazenave [:kaze] 2011-08-25 01:58:34 PDT
Created attachment 555675 [details] [diff] [review]
patch proposal

http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=327a6ccd7f4a
Comment 10 Fabien Cazenave [:kaze] 2011-08-25 03:39:39 PDT
This patch raises two “unexpected-pass” on crashtest/debug:

REFTEST TEST-UNEXPECTED-PASS
  | file:///[…]/layout/base/crashtests/479360-1.xhtml
  | assertion count 0 is less than expected 6 assertions
REFTEST TEST-UNEXPECTED-PASS
  | file:///[…]/layout/base/crashtests/481806-1.html
  | assertion count 0 is less than expected 6 assertions

These assertions are related to bug 439258, see layout/base/crashtests/crashtests.list: https://mxr.mozilla.org/mozilla-central/source/layout/base/crashtests/crashtests.list#261
  # 479360-1.xhtml will assert 6 times due to bug 439258 and then make the test
  # after the test after it also assert 6 times.
  asserts(6) load 479360-1.xhtml # Bug 439258
  load 480686-1.html
  asserts(6) load 481806-1.html  # Bug 439258

This patch does not solve bug 439358: it just prevents the table editing widgets to be displayed on an editable <td> (see 479360-1.xhtml), so these `asserts(6)' can be removed now.
Comment 11 Fabien Cazenave [:kaze] 2011-08-25 03:40:23 PDT
s/bug 439358/bug439258/
Comment 12 Fabien Cazenave [:kaze] 2011-08-25 03:46:25 PDT
Created attachment 555692 [details] [diff] [review]
patch proposal

http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=b14da492b5e2
Comment 13 :Ehsan Akhgari 2011-08-31 16:20:52 PDT
Comment on attachment 555692 [details] [diff] [review]
patch proposal

Review of attachment 555692 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/reftests/editor/reftest.list
@@ +67,5 @@
>  == 338427-1.html 338427-1-ref.html
>  skip-if(Android) == 674212-spellcheck.html 674212-spellcheck-ref.html
>  skip-if(Android) == 338427-2.html 338427-2-ref.html
>  skip-if(Android) == 338427-3.html 338427-3-ref.html
> +skip-if(Android) == 462758-grabbers-resizers.html 462758-grabbers-resizers-ref.html

Why are you skipping the test on Android?
Comment 14 :Ehsan Akhgari 2011-08-31 17:15:35 PDT
(In reply to Fabien Cazenave (:kaze) from comment #7)
> Created attachment 555122 [details] [diff] [review]
> patch proposal
> 
> Not ready for review, I still have to write the tests.
> 
> Ehsan: given the number of such bugs, should I add an optional boolean
> argument to these two functions (false by default) that would allow to check
> that the node is a strict descendant of the active editing host?

How about adding another helper function called IsEditingHost or something?
Comment 15 :Ehsan Akhgari 2011-09-06 14:03:38 PDT
ping?
Comment 16 Fabien Cazenave [:kaze] 2011-09-07 08:02:44 PDT
Created attachment 558811 [details] [diff] [review]
patch proposal

(In reply to Ehsan Akhgari [:ehsan] from comment #13)
> Why are you skipping the test on Android?

I thought that these grabbers and resizers wouldn’t show up in Fennec. Here’s an updated version of this patch.
Comment 17 Fabien Cazenave [:kaze] 2011-09-07 08:06:29 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #14)
> How about adding another helper function called IsEditingHost or something?

That could be an option as well but I see two main drawbacks:
 • the code returning the active editing host would be called twice (I don’t know if that would be expensive wrt the perfs);
 • an attribute of a "contenteditable" element should not be modifiable, so we need a bit more than just `IsEditingHost'.

However, bug 677752 has been landed short after I’ve proposed to add an optional boolean param to IsModifiableNode / IsNodeInActiveEditor, and I think most cases where we need to check if the current node is a strict descendant of the editing host should be covered now. At this current point, I think modifying IsModifiableNode / IsNodeInActiveEditor would be mostly a matter of factorizing a few lines of code.
Comment 18 :Ehsan Akhgari 2011-09-07 08:09:14 PDT
Comment on attachment 558811 [details] [diff] [review]
patch proposal

Review of attachment 558811 [details] [diff] [review]:
-----------------------------------------------------------------

If this is ready to land, I'll land the patch for you when you attach a version which addresses the nit below.

Thanks!

::: editor/libeditor/html/nsHTMLAnonymousUtils.cpp
@@ +367,5 @@
>      NS_ASSERTION(!mInlineEditedCell, "HideInlineTableEditingUI failed");
>    }
>  
>    // now, let's display all contextual UI for good
> +  nsCOMPtr<nsIContent> hostContent = GetActiveEditingHost();

Nit: change this to an nsIContent* to avoid an unnecessary QI.
Comment 19 Fabien Cazenave [:kaze] 2011-09-07 08:33:05 PDT
Created attachment 558823 [details] [diff] [review]
patch proposal
Comment 20 Fabien Cazenave [:kaze] 2011-09-07 08:35:21 PDT
Created attachment 558825 [details] [diff] [review]
patch proposal

Forgot to `hg qrefresh' again. *sigh*
Comment 22 :Ehsan Akhgari 2011-09-07 12:48:02 PDT
It turns out that the test fails on Android, so I had to mark it as such: http://hg.mozilla.org/integration/mozilla-inbound/rev/b1fbce31b7f2
Comment 24 ejsanders 2014-05-06 09:14:36 PDT
This is happening in FF 29. Is it a regression?

Note You need to log in before you can comment on or make changes to this bug.