Closed Bug 237964 (contenteditable) Opened 20 years ago Closed 16 years ago

Allow editable areas in browser (contentEditable)

Categories

(Core :: DOM: Editor, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla1.9alpha6

People

(Reporter: glazou, Assigned: peterv)

References

(Depends on 17 open bugs, Blocks 3 open bugs, )

Details

(Whiteboard: PRD:GKO-019)

Attachments

(5 files, 14 obsolete files)

1.02 KB, text/html
Details
1.90 KB, text/plain
Details
1.90 KB, text/plain
Details
1.75 KB, text/html
Details
132.29 KB, patch
Details | Diff | Splinter Review
===> Please refrain from Cc:ing the whole world on this bug. Thanks. <===

Copy of a mail I sent to Brendan:

  There are two ways of doing contentEditable, imho:

  1. if a document contains one or more contentEditable areas, make the
     whole document editable using Midas and use -moz-user-* properties
     to make *only* the contentEditable areas really editable.
     I am using this mechanism in my implementation of templates in Nvu.
     To do that, we need to fix a known bug : caret movement does not
     obey to -moz-user-select... You can see that in Composer: make two
     paragraphs and make the first one
           style="-moz-user-select: none ! important"
     place the caret in second and press the up-arrow key...

  2. use an editor on a per-contentEditable-area basis. Again, two ways
     of doing, the expensive-simple and the clean one. The former is
     one editor instance per area, the latter is one editor instantiated
     only when the selection is entirely contained in such an area.
     I'm sure you see what I mean here.
     The hard thing here is that the editor won't deal with a document
     but with a document fragment... I have to think about the impact on
     our code.

  Sidenote, thinking at loud: if we have a new XUL tag for editing a
  fragment, then we can probably do contentEditable with XBL only... 

Extra information: having more than one editor in the same window is not
a problem. I have done it quite easily in Nvu (for tabs) and I must say
that the work the editor team did removing the editorShell and adding
GetCurrentEditor() helped a lot. It's easy to switch from one editor
instance to another depending on the focus|selection.
Taking.
Assignee: general → daniel
Priority: -- → P1
Target Milestone: --- → mozilla1.8beta
Working on it.

1. hack nsGenericHTMLElement to switch to designMode="on" when a contenteditable=
   "true" is reached if it's not already "on". This could have to be deferred to
   after the onload event.
2. apply an extra override stylesheet to the document making the whole document
   read-only but the elements with contenteditable="true" (trivial)
3. hack nsEditor to refuse inserting and deleting a node if the container is
   read-only, and possibly nsHTMLEditor to refuse operations
4. check if the caret placement is correctly done by layout, ie if it refuses to
   place the caret in read-only frames
Status: NEW → ASSIGNED
step 1 done, works fine so far.
Attached patch work in progress #2 (obsolete) — Splinter Review
For those who want to give a try to contenteditable, this fix is almost fully
functional. Just assign the attribute contenteditable="true" to an element in
your document instance to see it become content-editable while the rest of the
document remains read-only.

Still on the todo list:

1. test keypress/deletion/CR in read-only tables
2. try to fix layout so collapsed selection cannot be placed in a read-only
frame
Attachment #156777 - Attachment is obsolete: true
Warning, the current patch works only in the browser. Editing a document having
a contenteditable element will fail (blank page in the editor); I'm on it.
For the purpose of this bug, I have blocked caret movement from one editable
frame to a non-editable one, outside of caret browsing mode of course. The ideal
solution would be a loop to find the nearest editable frame in the requested
direction but I have no time right now to do that, it's a significant change in
nsSelection.
State of the work in progress:

- designMode="off" now works
- contenteditable attribute with value "true" on an element makes the element
  become editable in a read-only document. This is based on Midas, so only one
  editor is instantiated even if there are multiple contenteditable elements
  in the document
- setting contenteditable to anything but "true" or removing it makes such an
  element become read-only again but the editor above it is not shutdown
- designMode="off" tears down the editor above a document containing a
  contenteditable="true" element
- moving the caret from an editable frame to a non-editable one is refused
- placing the caret in a non-editable frame is refused but non-collapsed
  selection is still of course possible
- patch is caret-browsing friendly **BUT** 
- bug 198155 is fixed by my patch :-)
- all existing tests about Midas and contenteditable run OK but one because it
  is setting a contenteditable for MSIE and after that designMode="on" for
  Gecko... The last one takes precedence so the whole doc becomes editable.
- bug 209020 is _not_ fixed by my patch
Comment on attachment 156803 [details] [diff] [review]
work in progress #2

The patch is looking good.  A few nits:

You have this block in a few places:
+  PRBool isUserInputDisabledNode;
+  IsUserInputDisabledNode(aNode, &isUserInputDisabledNode);
+  if (isUserInputDisabledNode)

Initialize isUserInputDisabledNode to false if you are going to ignore the
return value.

Remove the added spaces/tabs from Index: editor/libeditor/html/nsHTMLEditor.h
(around line 390)--prototype for FindUserSelectAllNode.

I'm guessing you need to finish nsEditor::IsUserInputDisabledNode.

I'm anxious to see your final patch; great work!  :-)
Attached patch fix #1 (obsolete) — Splinter Review
This is a complete patch for the feature described above. Reviews welcome.
Attachment #156803 - Attachment is obsolete: true
Ok, here are some comments:

I used the demo attached to this bug for testing and compiled fresh checkout
with the patch applied.

Loading the demo document, selecting the full editable text or only a word/part
of it and then typing any letter does not delete the selection but inserts the
letter right before the selection. Now select the text again and again press any
letter  and the text get's properly replaced.

Selecting text inside the editable area and pressing 'Backspace' or 'Del' does
not delete the text. You have to type a letter first, then you can select a text
and delete it. This also happens if you simply place the cursor inside the
editable text and press backspace.

If you delete the complete editable text ( carret/cursor blinking "at the start"
) and then press 'Backspace' the whole editable Div-tag disappears. IMHO in that
case pressing 'Backspace' should not delete the tag but it should simply be ignored.

What I noted is that in normal textareas/input fields the carret/cursor is no
longer visible. I don't know if that is caused by this patch or by some other
regression in the tree.

So far my comments after a first testrun. Keep up the great work.
(In reply to comment #12)

> Loading the demo document, selecting the full editable text or only a word/part
> of it and then typing any letter does not delete the selection but inserts the
> letter right before the selection. Now select the text again and again press any
> letter  and the text get's properly replaced.

Good catch. Part of the responsability is probably mine, part is editor core's.
Nothing I can do I'm afraid on the latter.

> Selecting text inside the editable area and pressing 'Backspace' or 'Del' does
> not delete the text. You have to type a letter first, then you can select a text
> and delete it. This also happens if you simply place the cursor inside the
> editable text and press backspace.

Hmm.... Ok.

> If you delete the complete editable text ( carret/cursor blinking "at the start"
> ) and then press 'Backspace' the whole editable Div-tag disappears. IMHO in that
> case pressing 'Backspace' should not delete the tag but it should simply be
ignored.

Yes, this is caused by the Selection code and I'm not sure we can fix it.

> What I noted is that in normal textareas/input fields the carret/cursor is no
> longer visible. I don't know if that is caused by this patch or by some other
> regression in the tree.

Ooooh, that's a bad one. I think it's very easily fixed but it's an excellent
catch, thanks a lot!
What about images, table cells and absolute positioned elements? Are they
resizable? If yes, that wouldn't be a good choice because when using
contenteditable within a CMS the user can move/resize objects everywhere on the
screen.
(In reply to comment #14)
> What about images, table cells and absolute positioned elements? Are they
> resizable? If yes, that wouldn't be a good choice because when using
> contenteditable within a CMS the user can move/resize objects everywhere on the
> screen.

Of course they are!!!
Hmm, a switch would be the better option.
OK, I've done some initial investigation mainly as it relates to fixing bug 198155.

I've found two major problems.

1. Once you have gone to a midas page (see page in bug 198155), links no longer
load properly.

I went to http://www.squarefree.com/contenteditable.html and then
http://www.yahoo.com

Links on www.yahoo.com brought up blank pages.

2. In the same scenario as above (going to the test page), special keys don't
work (home, end, page up, page down)

If you create a new window, the problems go away, so these are problems on the
root docshell.
(In reply to comment #17)
> OK, I've done some initial investigation mainly as it relates to fixing bug
198155.
> 
> I've found two major problems.

Working on it.
We'll need to change nsIFrame::IsFocusable() so that it returns PR_FALSE and
*aTabIndex = -1 for any frame in the contenteditable area. Links and form
controls in the editable part of the document shouldn't receive focus.
Product: Browser → Seamonkey
Summary: Allow editable areas in browser → Allow editable areas in browser (contentEditable)
There are a couple problems I've noticed so far:

-If you reload the page, the contenteditable areas are no longer
editable. If you open a new tab or window though, the contenteditable
areas are editable in that tab/window. It seems like that woulde be a
problem with the docshell?
-You can paste text into read-only areas.
-If you make a selection in a read-only area that *starts* at the beginning of
a word, when you type, it inserts an extra space.
-There is no cursor in input fields and textareas in tabs that have
contenteditable elements. I'm guessing this is a problem with the CSS files for
contenteditable. They probably should not set inputs and textareas as
read-only.
-I haven't explicitly looked into it, but I think the last comment in
the bug about changing isFocusable on Iframes to return PR_FALSE is
still unimplemented.

Any suggestions on where to look to fix any of these things are welcome. As are
any bugs you notice.

P.S. I've never made a CVS patch before. Let me know if you have problems
patching it in.
Goes with attachment 176217 [details] [diff] [review]. I don't have CVS write access to add files to CVS.
Goes with attachment 176217 [details] [diff] [review]. I don't have CVS write access to add files to CVS.
Goes in mozilla/editor/composer/src/res/
ojanster@gmail.com, you can use cvsdo add as long as the directory exists, see
http://viper.haque.net/~timeless/redbean/
Shouldn't this bug be moved to Core?
-> Core
Component: General → Editor
Product: Mozilla Application Suite → Core
Is this bug just for the browser in Editor or would it also be applicable for Firefox?
While I don't completely understand the question in comment 27, this bug does apply to Firefox and any other mozilla-based browsers.  I don't believe it applies to Composer or Nvu.
*** Bug 316284 has been marked as a duplicate of this bug. ***
Depends on: 287707
Taking.
Assignee: daniel → peterv
Status: ASSIGNED → NEW
Target Milestone: mozilla1.8beta1 → mozilla1.9alpha
Daniel Glazman says he's got complete code for this which he will integrate:

http://glazman.org/weblog/dotclear/index.php?2006/06/07/1856-contenteditable
Daniel: could you please use the bug instead of your blog? As you'll note, I took this bug a couple of weeks ago because it laid dormant for more than a year and I have started working on it. If you plan to do this yourself, at least mention it in the bug (take it back) and post patches here. I have some comments about your approach (I think we should not be adding a new value to .designMode for example), and this bug is the right forum to discuss that.
Here is a possible implementation. Just so you know, you took the bug
precisely when I had a meeting with hish at Roissy CDG about it
and we extensivley discussed it.

I used a new value for designMode absolutely on purpose, so people willing to make an area editable in a web page have two options : use the contenteditable attribute OR manually turn on designMode="partial" and apply CSS -moz-user-modify properties as they wish.
I don't think we should make designMode and contenteditable different from
an implementation point of view. From my perspective, designMode and contenteditable are the same thing, only the set of editable areas differ.

* add an unsigned int mContentEditableAreaCount private member to
  nsIDocument with two helpers SetContentEditableAreaCount(PRUint32
  aCount) and GetContentEditableAreaCount()
* implement them in nsDocument.h and nsDocument.cpp
* hack nsHTMLDocument.cpp to make SetDesignMode() and
  GetDesignMode() accept a new "partial" value
      o add boolean mPartialEditing to nsHTMLDocument.h
      o make SetDesignMode() call
        nsIEditingSession::MakeWindowEditable() with "html" for
        designMode="on" and "html-partial" for designMode="partial" ; reflect
        on mPartialEditing
      o make GetDesignMode() deal with mPartialEditing
* add a new mask eEditorContentEditableMask to nsIPlaintextEditor.idl
* make the nsEditingSession add
  nsIPlaintextEditor::eEditorContentEditableMask to editor flags for
  "html-partial" editorType
* hack nsHTMLEditor
      o so the link handler is not nulled out if the flags have
        nsIPlaintextEditor::eEditorContentEditableMask
      o also make sure JS and plugins are still allowed
      o and use a different override stylesheet named
        resource:/res/ContentEditableOverride.css if the flags have
        nsIPlaintextEditor::eEditorContentEditableMask
* hack findBar.js so it does not work on designMode="partial" and if
  the caret is in an editable area
* hack nsFrame.cpp so editor content is not focusable only if we're
  not in a partially editable document like designMode="partial"
* hack nsFrameFrame.cpp to deal nicely with designMode="partial"
* create a new stylesheet resource:/res/ContentEditableOverride.css
  from resource:/res/EditorOverride.css making sure form elements and
  links outside of contenteditable areas keep a normal rendering ; add -
  moz-user-modify: read-write to all contenteditable areas and children
  and -moz-user-modify: read-only to all others (that should already be
  the case) ; don't forget to update the Makefile.in about it
* hack nsGenericHTMLElement.cpp to let it deal with the
  contenteditable attribute
      o in SetAttr()  if the attribute is contenteditable and the
        value is true, increment mContentEditableAreaCount on the document.
        If we are not called from the content sink, then check if the
        designMode is already partial or on. If it's not, turn it on (note to
        self : if it was on, just change the override stylesheet)
      o in SetAttr(), if the attribute is contenteditable and the
        value is not true, decrement mContentEditableAreaCount on the
        document. If we are not called from the sink, and if
        mContentEditableAreaCount is zero, turn designMode off
      o same thing in UnsetAttr()
* hack nsDocument::EndLoad to turn designmode="partial" on if it's
  not on yet and if mContentEditableAreaCount is greater than zero
* hack nsTextEditorKeyListener::KeyPress() to trash key presses if
  the selection contains non editable nodes if designMode is partial
so peterv, what's your proposal here ?
Attached patch work in progress #1 (obsolete) — Splinter Review
This is a work in progress. Works already fine for contenteditable attributes dynamically added to the document. Not handling yet removal of the attribute nor attribute present in the document at load time, but that's almost trivial to add, the big part of the patch is already here.
Works fine with the 1.8.0 branch (FF15) and modulo a trivial conflict to solve in findBar.js with the 1.8.1 branch (FF20). The only interface change is a new editor flag mask added to nsIPlaintextEditor.idl. Changes just nothing to designMode="on".
Attachment #157052 - Attachment is obsolete: true
Attached file Test Case/Demo #2
test for the patch just above
My approach is slightly different. I've implemented intrinsic state for read/write in elements. The editable status of a node is stored in a flag on nsINode, which means setting/detecting designMode just sets/checks the flag on the document.

I've mapped the contentEditable attribute into a style rule (which sets -moz-user-modify). I'm not entirely sure this is the right thing. The alternative is to have 'editable roots', which are either the root node (for an editable document, for example with designMode="on" or in a real editor), a contentEditable 'editing host' or input/textarea/textbox with the readonly attribute not "true". We could then map the editable roots to -moz-user-modify: read-write for example in the nsHTMLStyleSheet. I don't know which approach would be best, I need to check with style/layout peers.

(In reply to comment #33)
> I used a new value for designMode absolutely on purpose, so people willing to
> make an area editable in a web page have two options : use the contenteditable
> attribute OR manually turn on designMode="partial" and apply CSS
> -moz-user-modify properties as they wish.

That would break :read-write/:read-only though, right?

My worry about the new value is scripts written for other browsers that don't expect it and fail. Would setting contentEditable to true on the documentElement and then using stylerules be an alternative?

> * hack nsTextEditorKeyListener::KeyPress() to trash key presses if
>   the selection contains non editable nodes if designMode is partial

Your previous patch dealt with this in the transactions I think. Do you think this is the better approach? I haven't dealt with all the changes to the editor myself.

(In reply to comment #35)
> The only interface change is a new
> editor flag mask added to nsIPlaintextEditor.idl.

Well, and a change to nsIDocument, which will need to be moved to a separate interface to get this into the 1.8 branches.

I don't understand how you handle focus when there are contentEditable attributes. I also don't see how you deal with toggling designMode when there are contentEditable attributes.

I'll try to get my patch to a point where I can attach it.
Attached patch work in progress #2 (obsolete) — Splinter Review
Attachment #225973 - Attachment is obsolete: true
For some reason I thought this was already on the blocker list, and IMO should be. It's a major win for the web.
Flags: blocking1.8.1?
Personally I don't think it's going to be feasible to get this on the branch, some of the reasons are in comment 37: we don't want to change interfaces, we don't want to introduce a new value for designMode, there's issues with toggling designMode when there's contentEditable regions.
(In reply to comment #39)
> For some reason I thought this was already on the blocker list, and IMO should
> be. It's a major win for the web.

Agreed, but is it enough of a win to block the release of Firefox 2.0 beta 1? I don't think we can block on this, but if you really believe in it, get it together, reviewed and tested on trunk by .. uh .. next week? :)
Flags: blocking1.8.1? → blocking1.8.1-
As someone who has struggled with rich-text editing in Firefox, I'm really excited to see this work (finally) getting done. Thanks for working on it!

As a developer who needs to create a decent rich-text editing experience on top of designMode right now, however, I'd like to impress upon you how urgent contentEditable support really is. Current implementations have to use an iframe(s), which has all of the problems iframes have:
a) They don't size to their content.
b) They don't inherit CSS.
c) They take time and (buggy) code to initialize.
d) They mess with the browser history.
d) Having a lot of them on a single page can be a performance problem.

Working around those things takes a lot of code and simply cannot be done reliably. I work on an application (pages.google.com) that has gone head over heels to mimic contentEditable in Firefox. And it only kind of works. I could write a long list of bugs that would just go away if some form of contentEditable support existed. As a start, compare the load of the editor page in Firefox to the load of the editor page in IE. That's just the tip of the iceberg in terms of bugs/issues that arise trying to mimic basic contentEditable support in Firefox.

While I appreciate the desire to write this in a complete and reliable way, I would encourage you to consider fixing the bugs as an incremental improvement in future releases if there is any chance of getting this into Firefox 2. The current state of affairs is that every rich-text editing app needs to perform a series of difficult and unreliable hacks to function in Firefox. Even a buggy implementation of contentEditable would be so much better. Firefox is now the only major browser that does not support contentEditable. It would be a shame to have to wait another 1-2 years before seeing contentEditable in Firefox.
I thought we used 1.8.1+ and a TM of Fx 2b1 to indicate "blocks beta 1", so I don't know why it would be minused just because it's not b1 fodder.  1.8.1+ and TM of Fx2b2, mayhap? ;)
Flags: blocking1.8.1- → blocking1.8.1?
(In reply to comment #42)
> The
> current state of affairs is that every rich-text editing app needs to perform a
> series of difficult and unreliable hacks to function in Firefox.

Yeah, bad scene, but there could certainly be worse.

> Even a buggy
> implementation of contentEditable would be so much better.

That depends very much on the bugs.  Periodic crashes when using contentEditable, security issues, or otherwise unpredictable behaviour would be worse, IMO, than not having an implementation in Fx2.  We need to be careful to let authors write safely to whatever editable solution we provide.

> It would be a shame
> to have to wait another 1-2 years before seeing contentEditable in Firefox.

It would, indeed, but we're less than a year from the planned release of Firefox 3, so happily "another 1-2 years" is not the only other option.

Daniel: if you're well enough yet to answer, it would be very helpful to get an ETA for a review-ready patch against trunk.  That will give us a lot of useful data with which to evaluate branch suitability and our interface options.

Adding another value to designMode doesn't seem like an API break that's beyond the realm of consideration, though it's possible that I'm not understanding all the implications.

Peter: do you have an ETA for your patch?  Having two options here would certainly be helpful, especially if there are techniques from one that we can graft into the other to resolve some of the outstanding issues.  (Cuisinart software development, gotta love it!)
Peter, do you believe that you will have a patch for this that can make the FF2 release?  Is it realistic, in your opinion, to block the FF2 release for such a patch?
I'd love this in product as soon as possible - but it is getting really late in the release and it is not clear that we'll be able to get this in time and quality that we'd like.  I'd concentrate on getting a good-high quality impl on the trunk with lots of testing and then we can figure out whether it makes sense to ship this in a later update release.
Flags: blocking1.8.1? → blocking1.8.1-
Attached patch Approach 2 - WIP (obsolete) — Splinter Review
Here's my WIP patch. Still need a bunch of work, in particular making sure that read-only parts can't be changed.
I mostly followed the WHATWG spec for contentEditable, real-life testcases would be nice so we can evaluate whether that spec is good enough or if we need to emulate IE more.
sorry guys, I was on summer break, let me (In reply to comment #48)
> Created an attachment (id=231339) [edit]
> Approach 2 - WIP
> 
> Here's my WIP patch. Still need a bunch of work, in particular making sure that
> read-only parts can't be changed.
> I mostly followed the WHATWG spec for contentEditable, real-life testcases
> would be nice so we can evaluate whether that spec is good enough or if we need
> to emulate IE more.
> 

(back from summer break, sorry for lag)

I understand exactly what you are trying to do here and how it solves edge
cases my patch probably does not deal with, but I think you are adding quite
a lot of editor knowledge to nsHTMLDocument, selection knowledge to
nsGenericHTMLElement while I tried to keep the additions _absolutely_ minimal.
At the cost of a light interface change, yes.
Daniel, Peter, are either of you still working on your respective patches? Just checking in. This is really a huge feature that would be sad to lose momentum on again.
Hey guys, how are things going with this? Are people still working on it?
Attached patch Approach 2 - WIP (obsolete) — Splinter Review
Hooked up spellchecking.
Attachment #231339 - Attachment is obsolete: true
What is the status of this bug?

Are you going to implement the contenteditable attribute?
If yes, what is the time estimate for it?

The current implementation of the various -moz-user-[editable/focus/input/select] is completely not usable in designMode as the selection is allowed to exists where it shouldn't when blocked by CSS.
So we still must use an IFRAME to hold the edited content.

I am not sure if reporting bugs on this issue would make any difference on the short term.

I really cannot find any reference on your priority for the MIDAS bugs.

http://wiki.mozilla.org/Gecko_1.9_Alpha_Planning
http://spreadsheets.google.com/pub?key=p4kVYBRbEKKgFPCn5XmfrXw
http://wiki.mozilla.org/Firefox3/Gecko_Feature_List

It seems the Editor is left behind.
(In reply to comment #53)
> What is the status of this bug?

It's assigned to me and I'm working on it.

> If yes, what is the time estimate for it?

Firefox 3/Gecko1.9

> I really cannot find any reference on your priority for the MIDAS bugs.

Unrelated to this bug.

Status: NEW → ASSIGNED
(In reply to comment #54)
I am very happy to hear this.

I don't know the details of your implementation, the specs or the intended
final behavior for contenteditable.

But here are a few basic functionality which IMHO contenteditable should offer: 

-editable elements should be part of the natural tabbing order (I know there's
a heated discussion inside a bug somewhere; let the developer override this
behavior if he wants);

-the selection should never be allowed to contain BOTH editable and
not-editable nodes (catch mouse/keyboard selection and prevent keyboard
navigation outside); 

-more, (do NOT allow DOM changes generated by keyboard/paste/drop) or (block
keyboard input/paste/drop) when the selection is not inside an editable
element;

-the editing behavior should be similar to IE: DOM node/attributes changes
should be confined inside the edited element and should NOT touch the edited
element in any way.
    -Usecase:
      -press ENTER inside a P having id="myeditableelement1"
      -the editor should not split the P in 2 Ps having the same ID and both
beeing editable; this will ruin the purpose of contenteditable and push
developers into crazy hacks to prevent it
      -contenteditable should really mean "content" editable; treat the
editable element like document.body, which cannot be deleted;

-pressing DELETE/BACKSPACE should not delete the currently edited element, nor
not-editable siblings;

-execCommand('indent/outdent') should not indent the currently editable LI or
its parent;

-execCommand('any_id') shold not alter the editable node;

-execCommand('selectall') should select all the contents of the currently
focused editable node, not all BODY;

The editable element should really act as a TEXTAREA with rich editing
capabilities.

Best luck,
Dan POPA
Comment on attachment 254956 [details]
simple test for -moz-user-modify and alikes

This bug is about contentEditable, not designMode.
Attachment #254956 - Attachment is obsolete: true
Flags: blocking1.9?
QA Contact: general → editor
Flags: blocking1.9? → blocking1.9-
Whiteboard: [wanted-1.9]
Attached patch v1 (obsolete) — Splinter Review
Since a patch has been attached by peterv, there's time enough to make 1.9B1, and we have more resources to help us now, I propose we make this a blocker for 1.9 (currently wondering why it isn't already).
Attached patch v1.1 (obsolete) — Splinter Review
Attachment #243146 - Attachment is obsolete: true
Attachment #266747 - Attachment is obsolete: true
Attached patch v1.2 (obsolete) — Splinter Review
Attachment #267427 - Attachment is obsolete: true
Attachment #267627 - Flags: review?(jonas)
Attached patch v1.2.1 (obsolete) — Splinter Review
Fixes a minor build problem on windows (have to use NS_IMETHOD macros even for [notxpcom] methods)
Attachment #176217 - Attachment is obsolete: true
Attachment #227048 - Attachment is obsolete: true
Attachment #267627 - Attachment is obsolete: true
Attachment #267767 - Flags: review?(jonas)
Attachment #267627 - Flags: review?(jonas)
Attached patch v1.2.1 (obsolete) — Splinter Review
Previous patch was missing two CSS files.
Attachment #267767 - Attachment is obsolete: true
Attachment #268483 - Flags: superreview?(jonas)
Attachment #268483 - Flags: review?(jonas)
Attachment #267767 - Flags: review?(jonas)
Comment on attachment 268483 [details] [diff] [review]
v1.2.1

Random comments

> class nsINode_base : public nsPIDOMEventTarget {
>@@ -596,6 +598,16 @@ public:
>     *flags &= ~aFlagsToUnset;
>   }
> 
>+  void SetEditableFlag(PRBool aEditable)
>+  {
>+    if (aEditable) {
>+      SetFlags(NODE_IS_EDITABLE);
>+    }
>+    else {
>+      UnsetFlags(NODE_IS_EDITABLE);
>+    }
>+  }

Add also method IsEditable, so that the flag doesn't need to be used
anywhere else except in nsINode. 
Drop "Flag" from the method name.


>+void
>+nsIContent::UpdateEditableState()
>+{
>+  PRBool editable;
>+  nsIContent *parent = GetParent();
>+  if (parent) {
>+    editable = parent->HasFlag(NODE_IS_EDITABLE);
>+  }
>+  else {
>+    parent = GetBindingParent();
>+    editable = parent && parent->HasFlag(NODE_IS_EDITABLE);
>+  }
>+
>+  SetEditableFlag(editable);
>+}

Could you explain why GetBindingParent is used when parent is null?


>+nsGenericHTMLElement::SetAttr(PRInt32 aNameSpaceID, nsIAtom* aName,
>+                              nsIAtom* aPrefix, const nsAString& aValue,
>+                              PRBool aNotify)
>+{

Could you perhaps use Before/AfterSetAttr here?


>+nsresult
>+nsGenericHTMLElement::SetContentEditable(const nsAString& aContentEditable)
>+{
>+  if (aContentEditable.LowerCaseEqualsLiteral("inherit")) {
>+    UnsetAttr(kNameSpaceID_None, nsGkAtoms::contentEditable, PR_TRUE);
>+  }

Is this behavior documented somewhere? I mean unsetting the attribute.
Also, what if the document is xhtml, should it still be LowerCaseEqualsLiteral?


>@@ -772,7 +772,11 @@ nsWebShell::OnLinkClick(nsIContent* aCon
...
>-  
>+
>+  if (aContent->HasFlag(NODE_IS_EDITABLE)) {
>+    return NS_OK;
>+  }
>+
...
>@@ -800,6 +804,10 @@ nsWebShell::OnLinkClickSync(nsIContent *
...
>+  if (aContent->HasFlag(NODE_IS_EDITABLE)) {
>+    return NS_OK;
>+  }
>+

I don't quite like this. Spreading the information about contentEditable all over the tree.
Not sure how to deal this though.


>@@ -562,4 +562,7 @@ interface nsIEditor  : nsISupports
> 
>   /* Run unit tests. Noop in optimized builds */
>   void debugUnitTests(out long outNumTests, out long  outNumTestsFailed);
>+
>+  /* checks if a node is read-only or not */
>+  [notxpcom] boolean isModifiableNode(in nsIDOMNode aNode);

 Why not call the method then IsReadOnlyNode or IsEditableNode or something?

> private:
>   DeleteElementTxn();
>@@ -83,6 +84,9 @@ protected:
>   /** next sibling to remember for undo/redo purposes */
>   nsCOMPtr<nsIDOMNode> mRefNode;
> 
>+  /** the editor for this transaction */
>+  nsIEditor* mEditor;
>+

Is it guaranteed that mEditor has always a valid value? Raw pointers to XPCOM objects
look always a bit scary.
(In reply to comment #64)
> Add also method IsEditable, so that the flag doesn't need to be used
> anywhere else except in nsINode. 
> Drop "Flag" from the method name.

I'll add IsEditable, but I'll leave Flag on SetEditableFlag, it also unsets the flag but an element can be editable without the flag.

> Could you perhaps use Before/AfterSetAttr here?

No, AfterSetAttr doesn't have access to the old value.

> Is this behavior documented somewhere? I mean unsetting the attribute.
> Also, what if the document is xhtml, should it still be LowerCaseEqualsLiteral?

http://www.whatwg.org/specs/web-apps/current-work/#contenteditable
Hmm, I still need to add the code to ignore invalid values.

> I don't quite like this. Spreading the information about contentEditable all
> over the tree.
> Not sure how to deal this though.

I guess we could wedge an nsILinkHandler between the prescontext and the real nsILinkHandler (webshell), and only forward the call if the node is editable. Seems a bit fragile though.

> Is it guaranteed that mEditor has always a valid value? Raw pointers to XPCOM
> objects
> look always a bit scary.

There are other transactions that hold a raw pointer to the editor already.
Comment on attachment 268483 [details] [diff] [review]
v1.2.1

>Index: content/base/src/nsGenericElement.cpp
>+nsIContent::UpdateEditableState()
>+{
>+  PRBool editable;
>+  nsIContent *parent = GetParent();
>+  if (parent) {
>+    editable = parent->HasFlag(NODE_IS_EDITABLE);
>+  }
>+  else {
>+    parent = GetBindingParent();
>+    editable = parent && parent->HasFlag(NODE_IS_EDITABLE);
>+  }
>+
>+  SetEditableFlag(editable);
>+}

Why do you need the GetBindingParent part? The binding parent should always either be the node itself (for certain native anonymous content) or a node which you'd hit if you walk the parent chain.

I take it the idea here is that as BindToTree recurses down a subtree we also propagate the NODE_IS_EDITABLE flag?

>Index: content/html/content/src/nsGenericHTMLElement.cpp
>@@ -1175,6 +1183,13 @@ nsGenericHTMLElement::BindToTree(nsIDocu
>                                              aCompileEventHandlers);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>+  if (aDocument && HasFlag(NODE_IS_EDITABLE) && IsContentEditable()) {

Is the HasFlag test needed here? When can IsContentEditable ever return true while HasFlag returns false?

More to come...
FWIW, HTML5 currently doesn't reflect implementations. If the attribute value is invalid it should be handled the same as if the attribute was not specified.
Comment on attachment 268483 [details] [diff] [review]
v1.2.1

>Index: content/base/src/nsGkAtomList.h
>@@ -207,6 +207,7 @@ GK_ATOM(commandupdater, "commandupdater"
> GK_ATOM(comment, "comment")
> GK_ATOM(compact, "compact")
> GK_ATOM(concat, "concat")
>+GK_ATOM(contentEditable, "contenteditable")

This should be named 'nsGkAtoms::contenteditable'. Usually uppercase is used when the string contains a '-' or ' '.

>Index: content/html/content/src/nsGenericHTMLElement.h
>@@ -768,6 +783,72 @@ protected:
>    * spellchecking.
>    */
>   static void SyncEditorsOnSubtree(nsIContent* content);
>+
>+  /**
>+   * Returns PR_TRUE if the element has a contentEditable attribute.
>+   * aIsEditable will be set to PR_TRUE if the value of the contentEditable
>+   * attribute is "true" or an empty string.
>+   */
>+  PRBool HasContentEditableAttr(PRBool *aIsEditable) const;
>+
>+  /**
>+   * Returns nsIContent::ATTR_MISSING if the element has no contentEditable
>+   * attribute. Returns nsIContent::ATTR_VALUE_NO_MATCH if the element has a
>+   * contentEditable attribute and its value is not "true" or an empty string.
>+   * Returns 0 if the element has a contentEditable attribute and its value is
>+   * "true". Returns 1 if the element has a contentEditable attribute and its
>+   * value is an empty string.
>+   */
>+  PRInt32 GetContentEditableValue() const
>+  {
>+    static const nsIContent::AttrValuesArray values[] =
>+      { &nsGkAtoms::_true, &nsGkAtoms::_empty, nsnull };
>+
>+    return FindAttrValueIn(kNameSpaceID_None, nsGkAtoms::contentEditable,
>+                           values, eIgnoreCase);
>+  }
>+
>+private:
>+  /**
>+   * Returns whether this is a contenteditable root.
>+   */
>+  PRBool IsContentEditableRoot(PRBool aParentIsEditable) const
>+  {
>+    // If the attribute is missing then aContent is not a root, else aContent is
>+    // a root if the attribute has a value different from true or empty and the
>+    // parent is editable or the attribute has true or empty as the value and
>+    // the parent is not editable.
>+    PRBool editable;
>+    return HasContentEditableAttr(&editable) && editable == aParentIsEditable;

Sounds like this should say |editable != aParentIsEditable| by the description. Though why is it a root if it explicitly sets contenteditable to false? Actually, this function looks unused, is it needed at all?

>+  }
>+
>+  /**
>+   * Returns whether this element is an editable root. An editable root is
>+   * defined as an element that is editable and whose parent is either a
>+   * non-editable element or an editable document (so if the whole document is
>+   * editable, then there is only one editable root, namely the
>+   * documentElement).
>+   */
>+  PRBool IsEditableRoot() const;
>+
>+  /**
>+   * Returns the first node amongst this node and its ancestors that is an
>+   * editable root.
>+   *
>+   * @see IsEditableRoot for a definition of an editable root.
>+   */
>+  nsIContent* FindEditableRoot();
>+
>+  /**
>+   * Returns PR_TRUE if this element has a contentEditable attribute with true
>+   * or an empty string as its value.
>+   */
>+  PRBool IsContentEditable() const
>+  {
>+    return GetContentEditableValue() > nsIContent::ATTR_MISSING;
>+  }
>+
>+  void ChangeEditableState(PRInt32 aChange);

There's way too many different functions with almost the same name and only slightly different functionality here.
Could we get away with having:

GetExplicitContentEditable()

returning a true/false/notset tristate. That could replace HasContentEditableAttr, GetContentEditableValue, and IsContentEditable.

>Index: content/html/content/src/nsHTMLInputElement.cpp
\>@@ -238,6 +238,11 @@ public:
> 
>   virtual nsresult Clone(nsINodeInfo *aNodeInfo, nsINode **aResult) const;
> 
>+  void UpdateEditableState()
>+  {
>+    return UpdateEditableFormControlState();
>+  }

Add 'virtual' here to show that you're overriding a base method. Same thing in textarea.

>Index: content/html/document/src/nsHTMLDocument.cpp
>@@ -2207,14 +2218,14 @@ nsHTMLDocument::OpenCommon(const nsACStr
>     mRootContent = root;
>   }
> 
>-  if (mEditingIsOn) {
>+  if (IsEditingOn()) {
>@@ -3717,7 +3728,7 @@ nsHTMLDocument::GenerateParserKey(void)
> NS_IMETHODIMP
> nsHTMLDocument::GetDesignMode(nsAString & aDesignMode)
> {
>-  if (mEditingIsOn) {
>+  if (HasFlag(NODE_IS_EDITABLE)) {

Sometimes you replace mEditingIsOn with IsEditingOn() and sometimes with HasFlag(NODE_IS_EDITABLE)

still more on the way
Comment on attachment 268483 [details] [diff] [review]
v1.2.1

looks good otherwise
Attachment #268483 - Flags: superreview?(jonas)
Attachment #268483 - Flags: superreview+
Attachment #268483 - Flags: review?(jonas)
Attachment #268483 - Flags: review+
Regarding attachment 268483 [details] [diff] [review] :

I think that we discovered a problem with patch 268483. We have checked out the trunk of FF, applied the patch and built it on a Windows XP machine. It seems that contentEditable works OK, except for some selection problems.

See for details: http://www.mister-pixel.com/ever/bugs/firefox/content_editable_testcase2.237964.html

When having the caret on the first line inside an editable element, if you press the UP key, the caret remains visible in the same position, but you cannot type anymore until you press the DOWN key. So it appears that the actual caret position is outside of the editable element. The same thing happens for the last line and the DOWN key. This also happens when using the Ctrl+HOME and Ctrl+END key combinations.

Dan POPA
(In reply to comment #66)
> Why do you need the GetBindingParent part? The binding parent should always
> either be the node itself (for certain native anonymous content) or a node
> which you'd hit if you walk the parent chain.

I'll figure this out and remove GetBindingParent if I don't need it.

> Is the HasFlag test needed here? When can IsContentEditable ever return true
> while HasFlag returns false?

This was an optimisation to avoid calling IsContentEditable because that does string compares.(In reply to comment #68)

> (From update of attachment 268483 [details] [diff] [review])
> > nsHTMLDocument::GetDesignMode(nsAString & aDesignMode)
> > {
> >-  if (mEditingIsOn) {
> >+  if (HasFlag(NODE_IS_EDITABLE)) {
> 
> Sometimes you replace mEditingIsOn with IsEditingOn() and sometimes with
> HasFlag(NODE_IS_EDITABLE)

There is a difference: IsEditingOn will be true when the document is in designMode or an element has contentEditable set to true, HasFlag(NODE_IS_EDITABLE) on the document means the document is in designMode. I ges I could add an inline function |IsInDesignMode| to make the latter clearer?

(In reply to comment #70)
> When having the caret on the first line inside an editable element, if you
> press the UP key, the caret remains visible in the same position, but you
> cannot type anymore until you press the DOWN key. So it appears that the actual
> caret position is outside of the editable element. The same thing happens for
> the last line and the DOWN key. This also happens when using the Ctrl+HOME and
> Ctrl+END key combinations.

I'll try to address this, but if I can't figure it out I'll defer this to a new bug. Thanks for testing!
(In reply to comment #71)
> > Sometimes you replace mEditingIsOn with IsEditingOn() and sometimes with
> > HasFlag(NODE_IS_EDITABLE)
> 
> There is a difference: IsEditingOn will be true when the document is in
> designMode or an element has contentEditable set to true,
> HasFlag(NODE_IS_EDITABLE) on the document means the document is in designMode.
> I ges I could add an inline function |IsInDesignMode| to make the latter
> clearer?

naah, leave as is, mostly wanted to make sure that the changes were intentional.
Attached patch v1.2.2Splinter Review
Comments mostly addressed and a small hack to keep editors that load new documents working, I'll put a proper fix for that in a new bug. Doesn't fix the bug mentioned in comment 70, will file a new bug for that one too.
Attachment #268483 - Attachment is obsolete: true
In nsHTMLInputElement.cpp:

+        mozAutoDocUpdate(document, UPDATE_CONTENT_STATE, PR_TRUE);

needs a variable name, or the destructor will be called before the update occurs. 
There are a few more instances of that pattern too I noticed.
Sorry if reported already but with  Test Case/Demo #1 in the attachments, I can edit the text but once I set to read-only and then click to make it editable again, I can't edit the text anymore.
XULRunner trunk on Windows appears to be busted from this:

mozilla/embedding/browser/activex/src/control/MozillaBrowser.cpp(1295) : error C2660: 'nsIEditingSession::MakeWindowEditable' : function does not take 4 arguments
Depends on: 386253
(In reply to comment #78)
> XULRunner trunk on Windows appears to be busted from this:
> 
> mozilla/embedding/browser/activex/src/control/MozillaBrowser.cpp(1295) : error
> C2660: 'nsIEditingSession::MakeWindowEditable' : function does not take 4
> arguments

I spun this off as bug 386255.

Hey guys - totally awesome that we are making progress on this.  Where are the unit tests?  This is large change to go in without automated test coverage...
Depends on: 386300
(In reply to comment #80)
> Where are the
> unit tests?  This is large change to go in without automated test coverage...

Absolutely, I will keep this bug open for tracking that.
Does bug 386189 depend on this?
Depends on: 386189
Comment on attachment 270047 [details] [diff] [review]
v1.2.2

>Index: content/xul/content/src/nsXULElement.cpp
>+    const nsIAtom* tag = Tag();
>+    if (GetNameSpaceID() == kNameSpaceID_XUL &&
>+        (tag == nsGkAtoms::textbox || tag == nsGkAtoms::textarea) &&
>+        !HasAttr(kNameSpaceID_None, nsGkAtoms::readonly)) {
>+        state |= NS_EVENT_STATE_MOZ_READWRITE;
>+        state &= ~NS_EVENT_STATE_MOZ_READONLY;
>+    }

There is no textarea element in XUL so no need to check for this. In XUL textarea is done with <textbox multiline="true">
No longer depends on: 386253
Are we sure script can't run under the ContentStatesChanged notification in MakeContentDescendantsEditable?  I _think_ that's true right now, but in general I'm not sure we enforce that or anything.

As far as testing goes, please test the sort of thing that bug 315920 covers?
Though I guess this didn't implement CSS selectors for read-write/read-only, so there's nothing to test there...
Blocks: 333869
Blocks: mobb-4
Depends on: 386470
Depends on: 386495
Depends on: 386496
(In reply to comment #76)
> With  Test Case/Demo #1 in the attachments, I can
> edit the text but once I set to read-only and then click to make it editable
> again, I can't edit the text anymore.

More odd things: when you first press the second button and then the first to prevent from editing, you can still edit the content. And right-clicking anywhere in the page gives you a editable area menu.
Albert, please file a new bug for this issue and make it blocking this bug.
No longer depends on: 386637
Depends on: 386728
Depends on: 386729
Depends on: 386730
Depends on: 386731
Filed four bugs and made them blocking.

#386728 : Setting the contenteditable attribute through js twice doesn't work
#386729 : Textarea context menu appears on page with contenteditable node
#386730 : After reloading a page with a single contenteditable element, every element on that page is editable
#386731 : After a session restore, a contenteditable element cant be changed through js
Depends on: 386782
Filed another one.

Bug 198155: After going back to a page, contenteditable elements aren't editable.
Filed another one.

Bug 386782: After going back to a page, contenteditable elements aren't editable.
Comment on attachment 270047 [details] [diff] [review]
v1.2.2

>+  virtual void UpdateEditableState()
>+  {
>+    return UpdateEditableFormControlState();
>+  }
I know this has been made legal C++ but it still looks weird to me.
Depends on: 386838
Depends on: 386996
Depends on: 387380
Filed another one (might be related to the current changes):
bug 387534: Caret not visible in Iframe with designMode="on"
Depends on: 387534
Blocks: 387883
Filed another 387883 for breakage in thunderbird trunk -- cursor ignores
arrow keystrokes in compose window.
No longer blocks: 387883
Blocks: 335854
Depends on: 335856
Depends on: 388172
Depends on: 388175
Depends on: 388183
Depends on: 388422
Depends on: 388646
Depends on: 388655
Depends on: 388659
Depends on: 388980
Depends on: 389199
Depends on: 389317
No longer depends on: 389317
Depends on: 389321
Depends on: 389324
Depends on: 389337
Depends on: 389342
Depends on: 389348
Depends on: 389097
Depends on: 389350
Depends on: 389351
Depends on: 389352
Depends on: 389354
Depends on: 389372
No longer depends on: 389199
Depends on: 390246
Depends on: 390278
I think that the checkin for this bug is killing XForms.  When we query the intrinsic state on our xtf-based elements, we are always getting back that they are readonly whereas this wasn't previously the case.
Depends on: 390446
Leaving fixed bugs open for weeks for unit tests is confusing :(
Target Milestone: mozilla1.9alpha1 → mozilla1.9alpha6
Depends on: 390934
Alias: contenteditable
Depends on: 390767
Depends on: 393568
Depends on: 393936
Depends on: 394473
Depends on: 386872
Depends on: 395312
(In reply to comment #96)
> Leaving fixed bugs open for weeks for unit tests is confusing :(

s/weeks/months/

Wouldn't it be better to get the unit tests in at the same time as the patch, so incremental fixes afterward don't break initially-correct functionality?
Flags: in-testsuite?
Depends on: 395345
Filed bug 395345 on a regression of the behaviour of the <editor> element.
Depends on: 394562
Depends on: 395340
Depends on: 401990
Depends on: 401993
Depends on: 403148
Depends on: 404154
Depends on: 406596
Blocks: 406762
No longer blocks: 406762
Depends on: 406762
Depends on: 406916
Depends on: 408231
Depends on: 409766
Whiteboard: [wanted-1.9] → PRD:GKO-019 [wanted-1.9]
Blocks: 411686
No longer blocks: 411686
Depends on: 411686
Flags: wanted1.9+
Whiteboard: PRD:GKO-019 [wanted-1.9] → PRD:GKO-019
Depends on: 412949
Depends on: 413678
Depends on: 413687
Depends on: 413697
This was fixed months ago.  If you need a bug open for making unit tests, please file another.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Depends on: 413712
No longer depends on: 413697
Depends on: 412920
Depends on: 417533
Depends on: 421640
No longer depends on: 420987
No longer depends on: 409766
Depends on: 433692
Depends on: 433860
Depends on: 414223
Depends on: 440692
The patch for this bug was checked in at 2007-06-27 19:48 (this is to prevent me from having to find out when this was checked in, every time I try to find a regression range).
Depends on: 440614
Depends on: 442801
Depends on: 442808
Depends on: 442811
Depends on: 449685
Depends on: 439808
Blocks: 468835
Blocks: 478095
Depends on: 482481
Depends on: 482484
Depends on: 488094
Depends on: 498518
Depends on: 669026
Depends on: 674770
Depends on: 670807
Depends on: 670809
Depends on: 685445
Depends on: 685452
Depends on: 703185
Depends on: 721038
No longer depends on: 721038
Depends on: richtext2
Depends on: 747469
Blocks: 768344
Depends on: 911205, 970363
Depends on: 991537
Depends on: 873883
Should this really be marked as :resolved/fixed"? contenteditable is not working correctly at all and hasn't been doing so for about 5 years or so.
(In reply to comment #102)
> Should this really be marked as :resolved/fixed"? contenteditable is not
> working correctly at all and hasn't been doing so for about 5 years or so.

Please file bugs for the cases that it doesn't work correctly.
Depends on: 1193517
See Also: → 1175418
Depends on: 1297069
Blocks: 1261296
Depends on: 1486179
Depends on: 1615852
No longer depends on: 1615852
Blocks: 1749163
You need to log in before you can comment on or make changes to this bug.