Last Comment Bug 317093 - Midas: strong and em aren't recognized while formatting the text
: Midas: strong and em aren't recognized while formatting the text
Status: RESOLVED FIXED
: testcase
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: mozilla15
Assigned To: Aryeh Gregor (:ayg) (next working March 28-April 26)
:
: Makoto Kato [:m_kato]
Mentors:
Depends on: 748313
Blocks: 249909 richtext2
  Show dependency treegraph
 
Reported: 2005-11-19 01:57 PST by Alfonso Martinez
Modified: 2012-04-24 17:57 PDT (History)
6 users (show)
ayg: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch part 1, v1 -- Avoid spurious UNEXPECTED-FAILs in test_richtext2 when fixing expected fails (3.26 KB, patch)
2012-04-04 04:49 PDT, Aryeh Gregor (:ayg) (next working March 28-April 26)
ehsan: review+
Details | Diff | Splinter Review
Patch part 2, v1 -- execCommand() should remove <strong>, <em>, and <s> as well as <b>, <i>, <strike> (81.07 KB, patch)
2012-04-04 04:51 PDT, Aryeh Gregor (:ayg) (next working March 28-April 26)
ehsan: review+
Details | Diff | Splinter Review
Patch part 3, v1 -- Clean up nsHTMLEditor::GetInlinePropertyBase (11.64 KB, patch)
2012-04-17 05:26 PDT, Aryeh Gregor (:ayg) (next working March 28-April 26)
ehsan: review+
Details | Diff | Splinter Review
Patch part 4, v1 -- Use computed style for command state even if styleWithCSS is false (163.31 KB, patch)
2012-04-17 05:33 PDT, Aryeh Gregor (:ayg) (next working March 28-April 26)
ehsan: review+
Details | Diff | Splinter Review

Description Alfonso Martinez 2005-11-19 01:57:49 PST
Filing this bug according to bug 195050 comment 14

If a page tries to use Midas to be able to edit some HTML but the HTML has <strong> or <em> then the bold and italic commands doesnt work fine. It's not that the output is using <b> and <i> vs <strong> and <em>, it's the fact that if those tags are present they are a big problem for the users.

testcase: 
go to http://www.mozilla.org/editor/midasdemo/ switch to HTML source and paste this code:
<p>This is <b>bold</b> and this is <strong>strong</strong><br />
This one is <i>italic</i> and this one is <em>emphasis</em><br />
</p>

Switch back to WYSISYG and try to remove the effects.

Results:
if using CSS, after selecting each word and trying to set and remove the effect, switching back to HTML Source shows this:

<p>This is bold and this is <strong style="font-weight: normal;">strong</strong><br>This one is italic and this one is <em>emphasis</em><br></p>

the strong has been "removed" but the strong tag remains now with a style, and it hasn't been possible to remove the em tag.

Starting again, and now in WYSIWYG mode disable the use CSS checkbox, trying again to remove the format gives this output:
<p>This is bold and this is <strong>strong</strong><br>This one is italic and this one is <em>emphasis</em><br></p>

neither the strong or em has been removed.

setting the formats again on the strong and em gives this output:
<p>This is bold and this is <b><strong>strong</strong></b><br>This one is italic and this one is <i><em>emphasis</em></i><br></p>


expected results:
<p>This is bold and this is strong<br>This one is italic and this one is emphasis<br></p>
Comment 1 Kathleen Brade 2005-11-19 03:33:36 PST
This probably isn't what you are looking for but there is a command "removeformat" which removes all the styles from the selection.

If you just want Midas to support commands for "strong" and "em" I can create a patch for that but the new commands probably won't play well if <b> and <i> are already present.
Comment 2 Alfonso Martinez 2005-11-19 03:52:28 PST
I'm not asking for new commands to generate strong or em, or change the default output of the "bold" and "italic" commands by default to match IE (despite the command names, IE outputs strong and em instead of b and i) as maybe it would break current implementations. Maybe a new bug could be filed about that options and it would be interesting to see if it's worth or not, but this one should be fixed anyway.

This bug is filed just to remark the fact that if the html has strong or em tags then Midas doesn't deal properly with them, and due to bug 194957 the removeformat isn't a solution if the editor is using CSS. Besides that it would remove all the format inside of the selected text instead of just removing the tags that are causing the problems.

Comment 3 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-04-04 03:37:54 PDT
Confirmed in mozilla-central.  The styles aren't being correctly stripped here.  This is a particular problem because <strong> and <em> are generated by IE and Opera.  Self-contained test-case:

data:text/html,<!doctype html>
<div contenteditable><strong>strong</strong><em>em</em></div>
<script>
getSelection().selectAllChildren(document.body.firstChild.firstChild);
document.execCommand("bold");
getSelection().selectAllChildren(document.body.firstChild.lastChild);
document.execCommand("italic");
document.body.textContent =
document.body.firstChild.innerHTML;
</script>

Expected result:

  strongem

Actual result:

  <strong style="font-weight: normal;">strong</strong><em>em</em>

IE 10 Developer Preview, Chrome 19 dev, and Opera Next 12.00 alpha all give the expected result.
Comment 4 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-04-04 04:49:28 PDT
Created attachment 612150 [details] [diff] [review]
Patch part 1, v1 -- Avoid spurious UNEXPECTED-FAILs in test_richtext2 when fixing expected fails

This first part of the patch is actually unrelated to the bug; I just figured it wasn't worth filing its own bug.  It uses info() to output debugging info in test_richtext2.html, instead of is().  Without this patch, when there are unexpected passes, the is()es throw in spurious unexpected fails.  With this patch, it's easier to confirm that all unexpected test results are passes rather than fails.
Comment 5 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-04-04 04:51:09 PDT
Created attachment 612151 [details] [diff] [review]
Patch part 2, v1 -- execCommand() should remove <strong>, <em>, and <s> as well as <b>, <i>, <strike>

This fixes the actual bug.  The strikethrough command is affected too, as it turns out, so I fixed that as well.  Obviously, the richtext2 changes were auto-generated.  All test changes fix unexpected passes.
Comment 6 Mozilla RelEng Bot 2012-04-05 07:28:13 PDT
Autoland Patchset:
	Patches: 612150, 612151
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=fac878c9e955
Try run started, revision fac878c9e955. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=fac878c9e955
Comment 7 Mozilla RelEng Bot 2012-04-05 12:06:18 PDT
Try run for fac878c9e955 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=fac878c9e955
Results (out of 219 total builds):
    exception: 1
    success: 182
    warnings: 36
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-fac878c9e955
Comment 10 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-04-16 06:55:09 PDT
This was actually only fixed when styleWithCSS is true.  In particular, the test in comment 3 now fails again (differently), due to bug 738366.  Reopening to fix more fully.
Comment 11 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-04-17 05:26:34 PDT
Created attachment 615675 [details] [diff] [review]
Patch part 3, v1 -- Clean up nsHTMLEditor::GetInlinePropertyBase
Comment 12 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-04-17 05:33:40 PDT
Created attachment 615677 [details] [diff] [review]
Patch part 4, v1 -- Use computed style for command state even if styleWithCSS is false

Causes 273 new UNEXPECTED-PASSes in richtext2.  This just always uses computed styles for states for any command with a CSS equivalent.  This is per spec, and gives much more correct results in many cases.  State/value/indeterm no longer depend on styleWithCSS for non-collapsed ranges.  (This doesn't currently fix collapsed ranges, but it's a big improvement anyway.)

This re-fixes the test case from comment 3.  I'm pretty sure I've seen other bugs filed that would be fixed by this, too.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=d768b96aab54
Comment 13 Mozilla RelEng Bot 2012-04-17 10:47:18 PDT
Try run for d768b96aab54 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=d768b96aab54
Results (out of 219 total builds):
    success: 184
    warnings: 35
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/ayg@aryeh.name-d768b96aab54
Comment 14 :Ehsan Akhgari 2012-04-17 12:39:49 PDT
Comment on attachment 615675 [details] [diff] [review]
Patch part 3, v1 -- Clean up nsHTMLEditor::GetInlinePropertyBase

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

::: editor/libeditor/html/nsHTMLEditorStyle.cpp
@@ +1056,5 @@
>      NS_ENSURE_SUCCESS(result, result);
>      result = range->GetEndOffset(&endOffset);
>      NS_ENSURE_SUCCESS(result, result);
> +
> +    for (iter->Init(range); !iter->IsDone(); iter->Next()) {

You're not removing the existing Init() call and adding another one.  Please remove the other call.
Comment 15 Aryeh Gregor (:ayg) (next working March 28-April 26) 2012-04-18 07:27:46 PDT
Not sure if I'm supposed to change the milestone here, since different patches landed in different versions.  Maybe I should have filed a followup.

http://hg.mozilla.org/projects/birch/rev/665395f5734f
http://hg.mozilla.org/projects/birch/rev/2e99b3fea365

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