Closed Bug 317093 Opened 19 years ago Closed 12 years ago

Midas: strong and em aren't recognized while formatting the text

Categories

(Core :: DOM: Editor, defect)

Product:
Core
Component:
DOM: Editor
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla15

People

(Reporter: amla70, Assigned: ayg)

References

(Blocks 2 open bugs)

Details

(Keywords: testcase)

Attachments

(4 files)

Patch part 1, v1 -- Avoid spurious UNEXPECTED-FAILs in test_richtext2 when fixing expected fails
3.26 KB, patch
ehsan.akhgari
: 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
ehsan.akhgari
: review+
Details | Diff | Splinter Review
Patch part 3, v1 -- Clean up nsHTMLEditor::GetInlinePropertyBase
11.64 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
Patch part 4, v1 -- Use computed style for command state even if styleWithCSS is false
163.31 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review 
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>
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.
OS: Windows XP → All
Hardware: PC → All
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.
Blocks: 249909
QA Contact: editor
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.
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.
Assignee: brade → ayg
Status: NEW → ASSIGNED
Attachment #612150 - Flags: review?(ehsan)
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.
Attachment #612151 - Flags: review?(ehsan)
Flags: in-testsuite+
Whiteboard: [autoland-try:-u all]
Whiteboard: [autoland-try:-u all] → [autoland-in-queue]
Attachment #612150 - Flags: review?(ehsan) → review+
Attachment #612151 - Flags: review?(ehsan) → review+
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
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
Whiteboard: [autoland-in-queue]
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.
Blocks: richtext2
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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
Attachment #615677 - Flags: review?(ehsan)
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 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.
Attachment #615675 - Flags: review?(ehsan) → review+
Attachment #615677 - Flags: review?(ehsan) → review+
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
Target Milestone: mozilla14 → mozilla15
Depends on: 748313
https://hg.mozilla.org/mozilla-central/rev/665395f5734f
https://hg.mozilla.org/mozilla-central/rev/2e99b3fea365
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: