Last Comment Bug 738366 - Default styleWithCSS to false
: Default styleWithCSS to false
Status: RESOLVED FIXED
: dev-doc-needed
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: mozilla14
Assigned To: :Aryeh Gregor (away until August 15)
:
Mentors:
Depends on: 671153 794201 829506 279330 745543
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-22 11:30 PDT by :Aryeh Gregor (away until August 15)
Modified: 2013-01-11 02:53 PST (History)
3 users (show)
ayg: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed


Attachments
Patch v1 (6.22 KB, patch)
2012-03-22 11:43 PDT, :Aryeh Gregor (away until August 15)
ehsan: review+
Details | Diff | Splinter Review
Patch v2 (6.58 KB, patch)
2012-03-22 13:33 PDT, :Aryeh Gregor (away until August 15)
ayg: review+
Details | Diff | Splinter Review
Patch v3, unexpected crashtest pass maybe fixed (8.02 KB, patch)
2012-03-23 09:40 PDT, :Aryeh Gregor (away until August 15)
ayg: review+
Details | Diff | Splinter Review

Description :Aryeh Gregor (away until August 15) 2012-03-22 11:30:07 PDT
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.
Comment 1 :Aryeh Gregor (away until August 15) 2012-03-22 11:43:46 PDT
Created attachment 608410 [details] [diff] [review]
Patch v1
Comment 2 :Ehsan Akhgari (away Aug 1-5) 2012-03-22 13:11:10 PDT
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 3 :Ehsan Akhgari (away Aug 1-5) 2012-03-22 13:12:38 PDT
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.
Comment 4 :Aryeh Gregor (away until August 15) 2012-03-22 13:33:04 PDT
Created attachment 608450 [details] [diff] [review]
Patch v2

Okay.
Comment 5 Mozilla RelEng Bot 2012-03-22 13:38:18 PDT
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
Comment 6 Mozilla RelEng Bot 2012-03-22 22:46:59 PDT
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
Comment 7 :Aryeh Gregor (away until August 15) 2012-03-23 09:40:38 PDT
Created attachment 608737 [details] [diff] [review]
Patch v3, unexpected crashtest pass maybe fixed

Not sure if this will actually fix the unexpected passes, because I couldn't reproduce them locally.  Let's see.
Comment 8 Mozilla RelEng Bot 2012-03-23 09:44:37 PDT
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
Comment 9 Mozilla RelEng Bot 2012-03-23 14:47:27 PDT
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
Comment 10 :Aryeh Gregor (away until August 15) 2012-04-01 05:44:37 PDT
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?
Comment 11 :Ehsan Akhgari (away Aug 1-5) 2012-04-02 11:06:19 PDT
Hmm, this is weird.  nsHTMLEditRules::WillDeleteSelection should not call GetBlockNodeParent, right?  So how does this happen?
Comment 12 :Ehsan Akhgari (away Aug 1-5) 2012-04-02 11:13:24 PDT
If the reason for the assertion is not obvious to you, I'd be fine with you filing a follow-up bug for that.
Comment 13 :Aryeh Gregor (away until August 15) 2012-04-03 07:16:51 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/aaba08c21296

Bug 671153 covers the same exception, so I didn't file a separate followup.
Comment 14 Marco Bonardo [::mak] (Away 6-20 Aug) 2012-04-04 04:45:04 PDT
https://hg.mozilla.org/mozilla-central/rev/aaba08c21296
Comment 15 Nickolay_Ponomarev 2012-04-07 04:02:02 PDT
Should probably get mentioned in the docs, since this is considered a compatibility risk.

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