Last Comment Bug 442186 - execCommand justify* fails on first line of contenteditable
: execCommand justify* fails on first line of contenteditable
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: All All
: -- normal with 8 votes (vote)
: mozilla8
Assigned To: Fabien Cazenave [:kaze]
:
Mentors:
http://www.comdispatch.com/editor.html
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-06-26 21:48 PDT by Ron Hook
Modified: 2011-08-23 13:41 PDT (History)
17 users (show)
mounir: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (2.32 KB, text/html)
2011-07-27 11:01 PDT, Fabien Cazenave [:kaze]
no flags Details
patch proposal (6.36 KB, patch)
2011-07-28 15:45 PDT, Fabien Cazenave [:kaze]
no flags Details | Diff | Splinter Review
patch proposal (25.52 KB, patch)
2011-07-29 09:52 PDT, Fabien Cazenave [:kaze]
no flags Details | Diff | Splinter Review
alternative approach: modifying `IsNodeInActiveEditor' (5.31 KB, patch)
2011-08-08 07:15 PDT, Fabien Cazenave [:kaze]
no flags Details | Diff | Splinter Review
patch proposal (2.88 KB, patch)
2011-08-08 10:28 PDT, Fabien Cazenave [:kaze]
no flags Details | Diff | Splinter Review
testcase (2.66 KB, text/html)
2011-08-08 11:01 PDT, Fabien Cazenave [:kaze]
no flags Details
patch proposal (WillAlign) (9.17 KB, patch)
2011-08-09 11:25 PDT, Fabien Cazenave [:kaze]
no flags Details | Diff | Splinter Review
testcase (2.21 KB, text/html)
2011-08-10 06:21 PDT, Fabien Cazenave [:kaze]
no flags Details
patch proposal -- affects WillAlign and RemoveAlignment (9.72 KB, patch)
2011-08-10 08:17 PDT, Fabien Cazenave [:kaze]
ehsan: review+
Details | Diff | Splinter Review

Description Ron Hook 2008-06-26 21:48:36 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.9) Gecko/2008052906 Firefox/3.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.9) Gecko/2008052906 Firefox/3.0

The first element (<p>, <div>, or no tag) of an area where contenteditable is true will cause the folloeing error when eg: execCommand("justifyright", false, false) is applied;

uncaught exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIDOMNSHTMLDocument.execCommand]" nsresult: "0x80004005 (NS_ERROR_FAILURE)" location: "JS frame :: http://www.comdispatch.com/editor.html :: set_style :: line 22" data: no]

If applied to any following tags it works fine without error.

Reproducible: Always

Steps to Reproduce:
1.place cursor in first paragraph
2.click right button
Actual Results:  
no visual results and following error

uncaught exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIDOMNSHTMLDocument.execCommand]" nsresult: "0x80004005 (NS_ERROR_FAILURE)" location: "JS frame :: http://www.comdispatch.com/editor.html :: set_style :: line 22" data: no]

Expected Results:  
text justifies to the right

This was found to happen in Vista as well as XP.

When you drag select and the selection extends out of the area and then try a command, nothing happens.

Sometimes when you try to indent the first tag it splits the first tag and only indents the text after the split. Hard to simulate as it is intermittent.

When you set a style like bold, underline, etc... in an area without a selection focused and start typing, it applies the style for a few letters, then not, then applies it again. the fix I have found is to focus the area after the execCommand.
Comment 1 Ryan 2008-10-23 13:47:32 PDT
I have hit this exact same problem with the justify* on the first line of a contentEditable div.

Something else, am I not sure if it is related, is sending the 'indent' command actually indents the contentEditable div itself and not just its contents.
Comment 2 Fredrik Josefsson 2009-01-20 02:37:03 PST
Confirming it still exists with Firefox 3.0.5.

In my case, it happens after execCommand("insertOrderedList", null,
null) and execCommand("insertUnorderedList", null, null). And the error only occurs once per command until a reload. 

It doesn't happen in all contenteditable divs. Only if the div is empty or only contains one line.
Comment 3 Jeremy Smith 2009-01-27 16:40:35 PST
An ugly workaround example:

try
{
	document.execCommand('justifyright', false, null);
}
catch (e)
{
	//special case for Mozilla Bug #442186
	if(e && e.result == 2147500037)
	{
		//probably firefox bug 442186 - workaround
		var range = window.getSelection().getRangeAt(0);
		var dummy = document.createElement('p');

		//find node with contentEditable
		var ceNode = range.startContainer.parentNode;
		while(ceNode && ceNode.contentEditable != 'true')
			ceNode = ceNode.parentNode;
		
		if(!ceNode)
			throw 'Selected node is not editable!';
		
		ceNode.insertBefore(dummy, ceNode.childNodes[0]);
		document.execCommand('justifyright', false, null);
		dummy.parentNode.removeChild(dummy);
	}
	else if(console && console.log)
		console.log(e);
}
Comment 4 Jeremy Smith 2009-01-27 16:43:54 PST
Edit: 'br' seems to make a slightly better dummy element than 'p'.
Comment 5 Ron Hook 2009-01-27 17:40:14 PST
It seems that when the selection the command is being applied to has a parent that is the node marked contentEditable, the browser tries to implement the command against the contentEditable node itself (or even its parent). Which obviously fails.

I implemented a workaround similar to above, but I make sure that only the parentless text is inserted into a div element. Otherwise all the text inserted into one div which then creates other problems when implementing styles, indents etc... Unfortunately it also means a lot more (unnecessary) coding to be done.
Comment 6 Ngan 2009-11-06 13:16:09 PST
This happens with Firefox 3.5.x on mac as well.  I also noticed that this problem only happens when the <div contenteditable="true"> is wrapped by another div:

<body>
  <div>
    <div contenteditable="true">
      This is the first line, it should fail to justifyright
    </div>
  </div>
</body>

However this works:

<body>
  <div contenteditable="true">
    This is the first line, however, it should be OK since the div is on the body level
  </div>
</body>
Comment 7 Dale Larsen 2010-03-04 15:18:16 PST
I have tested in firefox 3.5 and 3.6 and the justify commands don't work whether or not content is selected.

https://developer.mozilla.org/en/Rich-Text_Editing_in_Mozilla
Explains that this should work. But none of the following work:

justifyCenter
justifyLeft
justifyRight
justifyFull
justify
Comment 8 Dale Larsen 2010-03-04 15:49:34 PST
Jeremy Smith's work-around work. I made it dynamic for those who pass arguments to a function. Here is an example for all methods:

function editContent(cmd, args){
   if ((cmd == 'justifyright') || (cmd == 'justifyleft') || (cmd == 'justifycenter') || (cmd == 'justifyfull')) {
      try {
         document.execCommand(cmd, false, null);
      } 
      catch (e) {
         //special case for Mozilla Bug #442186
         if (e && e.result == 2147500037) {
            //probably firefox bug 442186 - workaround
            var range = window.getSelection().getRangeAt(0);
            var dummy = document.createElement('span');
            
            //find node with contentEditable
            var ceNode = range.startContainer.parentNode;
            while (ceNode && ceNode.contentEditable != 'true') 
               ceNode = ceNode.parentNode;
            
            if (!ceNode) throw 'Selected node is not editable!';
            
            ceNode.insertBefore(dummy, ceNode.childNodes[0]);
            document.execCommand(cmd, false, null);
            dummy.parentNode.removeChild(dummy);
         } else if (console && console.log) console.log(e);
      }
   } else {
      document.execCommand(cmd, false, args);
   }
}
Comment 9 Dale Larsen 2010-03-04 15:56:58 PST
The above still doesn't fix aligning the whole document or anything after the first line. Only lines after the first.
Comment 10 Dale Larsen 2010-03-05 06:46:32 PST
Fix for my code above. Silly me, instead of a span node use br and it works.
Comment 11 Joey Novak 2010-07-22 10:59:32 PDT
Hi Dale,  I modified your code for our Application (http://www.infusionsoft.com), and added a div, with a height of 0px, this prevents the browser from flashing when it inserts the br and removes it.

Thanks a TON for the work around, the bug has been around in our queue for over a month waiting for a fix.

function editContent(cmd, args){
if ((cmd == 'justifyright') || (cmd == 'justifyleft') || (cmd ==
        'justifycenter') || (cmd == 'justifyfull')) {
              try {
                 document.execCommand(cmd, false, null);
              }
              catch (e) {
                 //special case for Mozilla Bug #442186
                 if (e && e.result == 2147500037) {
                    //probably firefox bug 442186 - workaround
                    var range = window.getSelection().getRangeAt(0);
                    var dummy = document.createElement('div');
                    dummy.style.height="1px;";

                    //find node with contentEditable
                    var ceNode = range.startContainer.parentNode;
                    while (ceNode && ceNode.contentEditable != 'true')
                       ceNode = ceNode.parentNode;

                    if (!ceNode) throw 'Selected node is not editable!';

                    ceNode.insertBefore(dummy, ceNode.childNodes[0]);                    
                    document.execCommand(cmd, false, null);
                    dummy.parentNode.removeChild(dummy);
                 } else if (console && console.log) console.log(e);
              }
           } else {
              document.execCommand(cmd, false, args);
           }
}
Comment 12 M. Klein 2011-01-24 22:02:46 PST
Can confirm this on Firefox 3.6.13 on Win 7 32 bit using "insertunorderedlist"
Comment 13 metadings 2011-03-07 21:44:34 PST
Confirming bug for Firefox 4b12, Win7 32bit.

uncaught exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIDOMNSHTMLDocument.execCommand]" nsresult: "0x80004005 (NS_ERROR_FAILURE)" location: "JS frame :: http://localhost/simpleweb/Seite(1) :: <TOP_LEVEL> :: line 580" data: no]

The exception is thrown for the first line in a contenteditable (that's not wrapped by a div):

<div id="nicEdit" contenteditable="true">
  first line doesn't justify, but throws the exception.<br />
  <div>second line that works</div>
</div>
Comment 14 Ron Hook 2011-07-21 16:54:14 PDT
Just tested this on FireFox 5.0 on Windows XP and I can confirm that it still fails.
Comment 15 Fabien Cazenave [:kaze] 2011-07-27 10:23:20 PDT
Looks like this bug has been partially fixed lately. It occurs with Aurora (Fx 7.0a2, Linux) but “almost not” with Nightly (Fx 0.8a2, Linux) where pressing the “Right” button *twice* kinda works.

If this is the editable element:
    <div contenteditable>
      <div> first paragraph </div>
      <div> second paragraph </div>
    </div>

We get the following:

1. caret placed in the first paragraph;

2. first time the “Right” button is pressed we get:
    <div contenteditable>
      <div _moz_dirty style="text-align: right;"></div>
      first paragraph
      <div> second paragraph </div>
    </div>

3. second time the “Right” button is pressed we get:
    <div contenteditable>
      <div _moz_dirty style="text-align: right;"> </div>
      <div _moz_dirty style="text-align: right;"> first paragraph </div>
      <div> second paragraph </div>
    </div>

Note that I get the same result with Aurora by removing the main <div> wrapper in the HTML page.

----

Now if this is the editable element:
    <div contenteditable>
      <p> first paragraph </p>
      <p> second paragraph </p>
    </div>

On both Aurora and Nightly, putting the caret in the first paragraph and clicking on the “Right” button *once* seems to work but we get an unwanted <div> block:
    <div contenteditable>
      <div _moz_dirty style="text-align: right;"> </div>
      <p style="text-align: right;"> first paragraph </p>
      <p> second paragraph </p>
    </div>

----

Now if this is the editable element:
    <div contenteditable>
      first paragraph
      <p> second paragraph </p>
    </div>

On both Aurora and Nightly, everything works as expected:
    <div contenteditable>
      <div _moz_dirty style="text-align: right;"> first paragraph </div>
      <p> second paragraph </p>
    </div>

----

Now debugging nsHTMLEditRules::WillAlign to see what’s going wrong…
Comment 16 Fabien Cazenave [:kaze] 2011-07-27 11:01:00 PDT
Created attachment 548832 [details]
testcase
Comment 17 Fabien Cazenave [:kaze] 2011-07-28 08:52:29 PDT
On the first child of the editable <div>, the selection is extended outside of the active editing host (StartContainer == <body>), hence this behavior.

This should probably be fixed in nsHTMLEditRules::GetPromotedPoint and/or in nsHTMLEditor::IsNodeInActiveEditor.
Comment 18 :Ehsan Akhgari (away Aug 1-5) 2011-07-28 08:57:03 PDT
(In reply to comment #17)
> On the first child of the editable <div>, the selection is extended outside of
> the active editing host (StartContainer == <body>), hence this behavior.

Heh, another bug in this series!  :-)

> This should probably be fixed in nsHTMLEditRules::GetPromotedPoint and/or in
> nsHTMLEditor::IsNodeInActiveEditor.

IIRC I fixed nsHTMLEditRules::GetPromotedPoint recently to take the editing host into account...  I'm not sure if anything needs to be fixed in IsNodeInActiveEditor.  I think you should probably just add a IsNodeInActiveEditor somewhere...
Comment 19 Fabien Cazenave [:kaze] 2011-07-28 15:45:40 PDT
Created attachment 549250 [details] [diff] [review]
patch proposal

When the caret is in the #text node in the first paragraph, GetPromotedPoint returns the <body> node for the beginning of the selection (which is outside of the editing host). This is handled properly by WillOutdent but not by WillAlign…

I guess it means I should fix WillAlign but in this patch I’ve added a special case for kOpAlign instead. Is that acceptable?
Comment 20 Fabien Cazenave [:kaze] 2011-07-29 07:21:17 PDT
Here’s the tryServer result for this patch proposal: http://tbpl.mozilla.org/?tree=Try&rev=d90fa055f5ae

As you can see, we have a few Browserscope tests that “fail” now but when using http://www.browserscope.org/richtext2/test directly we can see that the 4 involved tests are actually passed with this patch (they’re green with the patch, yellow without). I believe we should update our own browserscope unit tests accordingly (the currentStatus.js thing?)… but again, I might be missing something here. :-/

Do we need specific unit tests or should we consider that the browserscope tests are enough for this patch?

----

As a side note, before this patch I’ve tried a more radical approach by limiting GetPromotingPoint to the active editing host itself (rather than its parent) for all operations — not just for kOpAlign. Here’s the result: http://tbpl.mozilla.org/?tree=Try&rev=a5614f917c89

With this approach we get a few more tests that pass concerning <blockquote> (which are test failures for tbpl) but it breaks test_htmleditor_keyevent_handling.html.
Comment 21 Fabien Cazenave [:kaze] 2011-07-29 08:03:56 PDT
PS: the 4 browserscope tests that pass with this patch are at the bottom of the A and AC sections.
 • section A (#RTE2-A_JL_TEXT-1_SC), score raised from 21/31 to 25/31
 • section AC (#RTE2-AC_JC_TEXT-1_SC), score raised from 7/18 to 11/18
Comment 22 Fabien Cazenave [:kaze] 2011-07-29 09:52:51 PDT
Created attachment 549407 [details] [diff] [review]
patch proposal

same patch, with updated browserscope tests.
Comment 23 :Ehsan Akhgari (away Aug 1-5) 2011-07-29 16:08:13 PDT
Comment on attachment 549407 [details] [diff] [review]
patch proposal

>-//      nsAutoString attr(NS_LITERAL_STRING("align"));
>-//      res = mHTMLEditor->SetAttribute(divElem, attr, *alignType);
>-//      NS_ENSURE_SUCCESS(res, res);
>+      //nsAutoString attr(NS_LITERAL_STRING("align"));
>+      //res = mHTMLEditor->SetAttribute(divElem, attr, *alignType);
>+      //NS_ENSURE_SUCCESS(res, res);

Please remove this change?

>+      // bug 442186: when aligning the content, the parent node itself has to
>+      // remain in the editable area.
>+      // Another (better?) solution would be to fix the code in WillAlign().
>+      if ((actionID == nsHTMLEditor::kOpAlign) &&
>+          !mHTMLEditor->IsNodeInActiveEditor(parent)) {
>+        break;
>+      }

This code really doesn't belong in GetPromotedPoint.  You should move it to WillAlign.

Also, I couldn't see where in WillOutdent this case is already handled...

>diff --git a/editor/libeditor/html/tests/browserscope/lib/richtext2/currentStatus.js b/editor/libeditor/html/tests/browserscope/lib/richtext2/currentStatus.js
>--- a/editor/libeditor/html/tests/browserscope/lib/richtext2/currentStatus.js
>+++ b/editor/libeditor/html/tests/browserscope/lib/richtext2/currentStatus.js

It's great that this patch fixes some richtext2 tests!  :-)  I don't know how you edited currentStatus.js, but for future reference you can regenerate it by reading the comment here: <http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/html/tests/browserscope/test_richtext2.html?force=1#54>

But we still need to have tests for the real fix...
Comment 24 Fabien Cazenave [:kaze] 2011-08-08 07:15:29 PDT
Created attachment 551453 [details] [diff] [review]
alternative approach: modifying `IsNodeInActiveEditor'

> This code really doesn't belong in GetPromotedPoint.
> You should move it to WillAlign.

I agree it should not belong to GetPromotedPoint. As you know, I’m even tempted to think that it should be fixed in IsNodeInActiveEditor: it would solve a lot of bugs (+15 points in the Browserscope tests, including several uncaught exceptions)… but that would also raise two regressions — see the detailed explanation in this attachment.

Now, if we don’t want to touch GetPromotedPoint / IsNodeInActiveEditor…

With the way these two methods are designed, when the selection is in the first child element of the active editing host, GetPromotedPoint returns an empty text node which is outside of the editing host, i.e. the selection is extended too far imho. As a result, GetNodesFromSelection (which is called at the beginning of WillAlign) returns two nodes:
 • an empty text node;
 • the first child element (= <div> or <p> in our test case).
Note: when the selection is in the second child element of the active editing host, GetNodesFromSelection only returns this child element.

To cope with this:
 • WillAlign should ignore the empty text node (e.g. like in WillCSSIndent, by checking if it’s editable);
 • WillAlign should not replace the first child element by its textContent (wtf? Still need to work on this.)
Comment 25 Fabien Cazenave [:kaze] 2011-08-08 07:56:05 PDT
(Auto-reply to Fabien Cazenave (:kazé) from comment #24)
> WillAlign should not replace the first child element by its textContent

This happens in nsHTMLEditRules::RemoveAlignment
  // if we are in CSS mode and if the element is a DIV, let's remove it
  // if it does not carry any style hint (style attr, class or ID)
https://mxr.mozilla.org/mozilla-central/source/editor/libeditor/html/nsHTMLEditRules.cpp#8727
Comment 26 :Ehsan Akhgari (away Aug 1-5) 2011-08-08 08:03:34 PDT
Comment on attachment 551453 [details] [diff] [review]
alternative approach: modifying `IsNodeInActiveEditor'

>browserscope/test_richtext2.html -- progress (+15 points)!

This is fortunate, but it worries me, since this patch is changing something that is called quite often for different operations, and while it may randomly fix some stuff, it may randomly break some too.

>  WARNING: Someone passed native anonymous content directly into frame construction.  Stop doing that!:
>                                                     file mozilla/layout/base/nsCSSFrameConstructor.cpp,  line 5988
>  ###!!! ASSERTION: anonymous nodes should not be in child lists: '!aOldChild->IsRootOfAnonymousSubtree()',
>                                                     file mozilla/layout/base/nsCSSFrameConstructor.cpp,  line 11438

This is bug 439258, and doesn't have anything to do with your patch.

>----
>
>This patch is not appliable yet because of the two following regressions.
>As we don't have much test coverage for the editor, there might be other
>regressions that I'm not aware of.
>
>test_bug414526.html
>(note: bug 414526 is where `IsNodeInActiveEditor' was added to the codebase)
>  * 4987 ERROR TEST-UNEXPECTED-FAIL
>      | Pressing delete key at end of editor3 changes the content
>      - got "<p id=\"editor1\" contenteditable=\"true\">editor1</p><p id=\"editor2\" contenteditable=\"true\">editor2</p><div id=\"editor3\" contenteditable=\"true\">ditor3</div><p id=\"editor4\" contenteditable=\"true\">editor4</p>non-editable text<p id=\"editor5\" contenteditable=\"true\">editor5</p>",
>      expected "<p id=\"editor1\" contenteditable=\"true\">editor1</p><p id=\"editor2\" contenteditable=\"true\">editor2</p><div id=\"editor3\" contenteditable=\"true\"><div>ditor3</div></div><p id=\"editor4\" contenteditable=\"true\">editor4</p>non-editable text<p id=\"editor5\" contenteditable=\"true\">editor5</p>"

I think I remember debugging something about this failure, in this subtree: <div id=\"editor3\" contenteditable=\"true\"><div>ditor3</div></div>.  IIRC, the selection was put after the firstChild of the outer div (which is itself a div, and the promotion would move it to the offset 1 of the inner div, which would be the "e" character, which results in this character to be deleted.

>test_htmleditor_keyevent_handling.html
>  * 7372 ERROR TEST-UNEXPECTED-FAIL
>      | non-tabbable HTML editor: Shift+Tab after Tab on UL
>      - got "<ul><li id=\"target\">ul list item</li></ul>",
>      expected "<ul><ul><li id=\"target\">ul list item</li></ul></ul>"
>  * 7379 ERROR TEST-UNEXPECTED-FAIL
>      | non-tabbable HTML editor: Shift+Tab on UL
>      - got "ul list item",
>      expected "<ul><li id=\"target\">ul list item</li></ul>"
>  * 7415 ERROR TEST-UNEXPECTED-FAIL
>      | non-tabbable HTML editor: Shift+Tab after Tab on OL -
>      got "<ol><li id=\"target\">ol list item</li></ol>",
>      expected "<ol><ol><li id=\"target\">ol list item</li></ol></ol>"
>  * 7422 ERROR TEST-UNEXPECTED-FAIL
>      | non-tabbable HTML editor: Shfit+Tab on OL
>      - got "ol list item",
>      expected "<ol><li id=\"target\">ol list item</li></ol>"

I don't know what's going on here without debugging.


With all this being said, I still don't think that we should change the behavior of IsNodeInActiveEditor (or range promotion) in order to fix this bug.  While in an ideal world, we could make them work more logically, I think that a better and safer fix for this specific bug is to change WillAlign.  We don't need to fix other range promotion related bugs here.  :-)
Comment 27 :Ehsan Akhgari (away Aug 1-5) 2011-08-08 08:06:53 PDT
(In reply to Fabien Cazenave (:kazé) from comment #24)
> Now, if we don’t want to touch GetPromotedPoint / IsNodeInActiveEditor…
> 
> With the way these two methods are designed, when the selection is in the
> first child element of the active editing host, GetPromotedPoint returns an
> empty text node which is outside of the editing host, i.e. the selection is
> extended too far imho. As a result, GetNodesFromSelection (which is called
> at the beginning of WillAlign) returns two nodes:
>  • an empty text node;
>  • the first child element (= <div> or <p> in our test case).
> Note: when the selection is in the second child element of the active
> editing host, GetNodesFromSelection only returns this child element.
> 
> To cope with this:
>  • WillAlign should ignore the empty text node (e.g. like in WillCSSIndent,
> by checking if it’s editable);

Yes, this makes sense.

>  • WillAlign should not replace the first child element by its textContent
> (wtf? Still need to work on this.)

Hmm, I don't immediately see how this is related to the fix for this bug (nor do I see why this code is needed in the first place).  But removing it might break web compatibility... :(
Comment 28 :Ehsan Akhgari (away Aug 1-5) 2011-08-08 08:36:56 PDT
(In reply to Fabien Cazenave (:kazé) from comment #25)
> (Auto-reply to Fabien Cazenave (:kazé) from comment #24)
> > WillAlign should not replace the first child element by its textContent
> 
> This happens in nsHTMLEditRules::RemoveAlignment
>   // if we are in CSS mode and if the element is a DIV, let's remove it
>   // if it does not carry any style hint (style attr, class or ID)
> https://mxr.mozilla.org/mozilla-central/source/editor/libeditor/html/
> nsHTMLEditRules.cpp#8727

FWIW, this code seems to be added in bug 102135...
Comment 29 Fabien Cazenave [:kaze] 2011-08-08 10:28:55 PDT
Created attachment 551496 [details] [diff] [review]
patch proposal

This patch should fix this bug without regressions.

It still fails on the 8 related Browserscope tests (J?_TEXT-1_SC in the A and AC sections) that were fixed by the first patch proposal (attachment 549407 [details] [diff] [review]). :-(
Comment 30 Fabien Cazenave [:kaze] 2011-08-08 11:01:38 PDT
Created attachment 551503 [details]
testcase

Updated testcase.

I’ve added three new tests where the editing host has only one child. These three new tests fail: a bogus <div> node is created outside of the active editing host.
Comment 31 Fabien Cazenave [:kaze] 2011-08-09 11:25:47 PDT
Created attachment 551832 [details] [diff] [review]
patch proposal (WillAlign)

My latest patch proposal (attachment 551496 [details] [diff] [review]) introduces a regression: we don’t call `RemoveAlignment' if the related node is a <div> because RemoveAlignment is likely to remove the <div> container. But if the <div> has an `align' attribute, the CSS alignment won’t apply…

This new version should solve that issue. However, I think it means we (almost?) always preserve all <div> blocks…
Comment 32 :Ehsan Akhgari (away Aug 1-5) 2011-08-09 14:13:51 PDT
(In reply to Fabien Cazenave (:kazé) from comment #31)
> Created attachment 551832 [details] [diff] [review] [diff] [details] [review]
> patch proposal (WillAlign)
> 
> My latest patch proposal (attachment 551496 [details] [diff] [review] [diff] [details] [review])
> introduces a regression: we don’t call `RemoveAlignment' if the related node
> is a <div> because RemoveAlignment is likely to remove the <div> container.
> But if the <div> has an `align' attribute, the CSS alignment won’t apply…
> 
> This new version should solve that issue. However, I think it means we
> (almost?) always preserve all <div> blocks…

Can't we just do that all the time?
Comment 33 Fabien Cazenave [:kaze] 2011-08-09 14:58:33 PDT
I’ve been doing a few tests. I think there’s at least one case where we should remove these <div> containers.

On this page: http://kazhack.org/contentEditable.xhtml
0. put the caret on a list item
1. uncheck “style with CSS”
2. click on the “align: right” icon: a <div align="right"> is inserted inside <li> (because <li align="right"> would not work)
3. check “style with CSS”
4. click on the “align: left” icon: the <div> is removed and <li> gets a style attribute.

This is a very specific case (nit? ^^) but I think removing the <div> makes sense here and that’s what the `aPreserveDiv' argument is about in my latest patch. However, leaving the <div> in <li> would not harm much either: tell me if this kind of specific cases is worth the extra complexity in the patch.
Comment 34 :Ehsan Akhgari (away Aug 1-5) 2011-08-09 15:48:31 PDT
No, I don't think so.  If the authors mess with styleWithCss in the middle of the way, they're going to get all sorts of weird behaviors.  Let's not try to eliminate one small edge case among those here!
Comment 35 Fabien Cazenave [:kaze] 2011-08-10 06:21:06 PDT
Created attachment 552061 [details]
testcase

Updated testcase. The three tests that I’ve added in comment #30 are now addressed by bug 677752.

(In reply to Ehsan Akhgari [:ehsan] from comment #34)
> Let's not try to eliminate one small edge case among those here!

If we ignore these edge cases, then the <div> aren't a specific case in `RemoveAlignment' any more, and we can make `RemoveAlignment' significantly done.
Comment 36 Fabien Cazenave [:kaze] 2011-08-10 06:22:04 PDT
s/done/simpler/
Comment 37 Fabien Cazenave [:kaze] 2011-08-10 08:17:55 PDT
Created attachment 552083 [details] [diff] [review]
patch proposal -- affects WillAlign and RemoveAlignment

RemoveAlignment works as before except that <div> elements are handled like any other blocks now.

Note that RemoveAlignment does not get rid of <center> elements properly. That’s not a regression.
Comment 38 Fabien Cazenave [:kaze] 2011-08-10 15:28:49 PDT
Try server looks OK:
http://tbpl.mozilla.org/?tree=Try&rev=4d1734070004
Comment 39 :Ehsan Akhgari (away Aug 1-5) 2011-08-10 16:02:34 PDT
Comment on attachment 552083 [details] [diff] [review]
patch proposal -- affects WillAlign and RemoveAlignment

Looks really good, thanks!
Comment 40 :Ehsan Akhgari (away Aug 1-5) 2011-08-10 16:03:46 PDT
Landed on inbound.
Comment 41 Fabien Cazenave [:kaze] 2011-08-10 16:12:54 PDT
FTR: concerning the removal of <div> blocks, there is a similar code section in `nsHTMLEditRules::RelativeChangeIndentationOfNode' that we probably should remove, too.
https://mxr.mozilla.org/mozilla-central/source/editor/libeditor/html/nsHTMLEditRules.cpp#8941

As this code in involved in bug 677752, it can be removed in that bug instead of this one.
Comment 42 Mounir Lamouri (:mounir) 2011-08-11 04:24:36 PDT
Pushed:
http://hg.mozilla.org/mozilla-central/rev/238609d8e455
Comment 43 :Ehsan Akhgari (away Aug 1-5) 2011-08-11 07:53:52 PDT
(In reply to comment #41)
> FTR: concerning the removal of <div> blocks, there is a similar code section in
> `nsHTMLEditRules::RelativeChangeIndentationOfNode' that we probably should
> remove, too.
> https://mxr.mozilla.org/mozilla-central/source/editor/libeditor/html/nsHTMLEditRules.cpp#8941
> 
> As this code in involved in bug 677752, it can be removed in that bug instead
> of this one.

Agreed.
Comment 44 Eric Shepherd [:sheppy] 2011-08-23 13:41:54 PDT
Listed on Firefox 8 for developers.

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