Closed Bug 738366 Opened 12 years ago Closed 12 years ago

Default styleWithCSS to false

Categories

(Core :: DOM: Editor, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla14
Tracking Status
firefox14 + fixed

People

(Reporter: ayg, Assigned: ayg)

References

(Depends on 1 open bug)

Details

(Keywords: dev-doc-needed)

Attachments

(1 file, 2 obsolete files)

All other browsers output stuff like b/strong by default, and that's what the spec says.  Gecko should too.  Authors currently have to work around this by running document.execCommand("useCSS", false, true) all the time, and they shouldn't have to.
Attached patch Patch v1 (obsolete) — Splinter Review
Assignee: nobody → ayg
Status: NEW → ASSIGNED
Attachment #608410 - Flags: review?(ehsan)
I'm fine with taking this as long as we track this bug for Firefox 14.  This is going to break web compat at some place, so we should track this and make sure that any regressions reported from this are going to be tracked properly.
Comment on attachment 608410 [details] [diff] [review]
Patch v1

Can you also add code to test_bug738366.html to make sure that issuing the stylewithcss command toggles the behavior?  r=me with that.
Attachment #608410 - Flags: review?(ehsan) → review+
Attached patch Patch v2 (obsolete) — Splinter Review
Okay.
Attachment #608410 - Attachment is obsolete: true
Attachment #608450 - Flags: review+
Whiteboard: [autoland-try:-u all]
Whiteboard: [autoland-try:-u all] → [autoland-in-queue]
Autoland Patchset:
	Patches: 608450
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=9b8810d40bea
Try run started, revision 9b8810d40bea. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=9b8810d40bea
Try run for 9b8810d40bea is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=9b8810d40bea
Results (out of 219 total builds):
    success: 181
    warnings: 24
    failure: 14
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-9b8810d40bea
Whiteboard: [autoland-in-queue]
Not sure if this will actually fix the unexpected passes, because I couldn't reproduce them locally.  Let's see.
Attachment #608450 - Attachment is obsolete: true
Attachment #608737 - Flags: review+
Whiteboard: [autoland-try:-u crashtest -b d]
Whiteboard: [autoland-try:-u crashtest -b d] → [autoland-in-queue]
Autoland Patchset:
	Patches: 608737
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=595b6c572e72
Try run started, revision 595b6c572e72. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=595b6c572e72
Try run for 595b6c572e72 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=595b6c572e72
Results (out of 13 total builds):
    success: 6
    warnings: 7
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-595b6c572e72
Whiteboard: [autoland-in-queue]
Ehsan, I looked at the new assertion but didn't understand it.  The interesting part of the stack is:

^G###!!! ASSERTION: null node passed to GetBlockNodeParent(): 'Not Reached', file /mnt/extra/checkouts/mozilla-central/editor/libeditor/html/nsHTMLEditor.cpp, line 931
nsHTMLEditor::GetBlockNodeParent(nsIDOMNode*) (/mnt/extra/checkouts/mozilla-central/editor/libeditor/html/nsHTMLEditor.cpp:932)
nsHTMLEditRules::WillDeleteSelection(nsISelection*, short, bool*, bool*) (/mnt/extra/checkouts/mozilla-central/editor/libeditor/html/nsHTMLEditRules.cpp:2187)
nsHTMLEditRules::WillDoAction(nsISelection*, nsRulesInfo*, bool*, bool*) (/mnt/extra/checkouts/mozilla-central/editor/libeditor/html/nsHTMLEditRules.cpp:617)
nsPlaintextEditor::DeleteSelection(short) (/mnt/extra/checkouts/mozilla-central/editor/libeditor/text/nsPlaintextEditor.cpp:789)
nsDeleteCommand::DoCommand(char const*, nsISupports*) (/mnt/extra/checkouts/mozilla-central/editor/libeditor/base/nsEditorCommands.cpp:630)
nsControllerCommandTable::DoCommand(char const*, nsISupports*) (/mnt/extra/checkouts/mozilla-central/embedding/components/commandhandler/src/nsControllerCommandTable.cpp:191)
nsBaseCommandController::DoCommand(char const*) (/mnt/extra/checkouts/mozilla-central/embedding/components/commandhandler/src/nsBaseCommandController.cpp:169)
nsCommandManager::DoCommand(char const*, nsICommandParams*, nsIDOMWindow*) (/mnt/extra/checkouts/mozilla-central/embedding/components/commandhandler/src/nsCommandManager.cpp:273)
nsHTMLDocument::ExecCommand(nsAString_internal const&, bool, nsAString_internal const&, bool*) (/mnt/extra/checkouts/mozilla-central/content/html/document/src/nsHTMLDocument.cpp:3100)

This is probably the same as the following line:

WARNING: NS_ENSURE_TRUE(leftParent && rightParent) failed: file /mnt/extra/checkouts/mozilla-central/editor/libeditor/html/nsHTMLEditRules.cpp, line 2194

The chance that this was actually caused by my change seems to be approximately zero, so is it okay if I just file a followup bug and increase the expected number of assertions to two for this test?
Hmm, this is weird.  nsHTMLEditRules::WillDeleteSelection should not call GetBlockNodeParent, right?  So how does this happen?
If the reason for the assertion is not obvious to you, I'd be fine with you filing a follow-up bug for that.
https://hg.mozilla.org/integration/mozilla-inbound/rev/aaba08c21296

Bug 671153 covers the same exception, so I didn't file a separate followup.
Depends on: 671153
Flags: in-testsuite+
Target Milestone: --- → mozilla14
https://hg.mozilla.org/mozilla-central/rev/aaba08c21296
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Should probably get mentioned in the docs, since this is considered a compatibility risk.
Keywords: dev-doc-needed
Depends on: 745543
Depends on: 279330
Depends on: 794201
Depends on: 829506
Depends on: 889940
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: