Default styleWithCSS to false

RESOLVED FIXED in Firefox 14

Status

()

Core
Editor
--
enhancement
RESOLVED FIXED
6 years ago
8 months ago

People

(Reporter: ayg, Assigned: ayg)

Tracking

(Depends on: 3 bugs, {dev-doc-needed})

Trunk
mozilla14
dev-doc-needed
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox14+ fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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.
Created attachment 608410 [details] [diff] [review]
Patch v1
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.
tracking-firefox14: --- → +
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+
Created attachment 608450 [details] [diff] [review]
Patch v2

Okay.
Attachment #608410 - Attachment is obsolete: true
Attachment #608450 - Flags: review+
Whiteboard: [autoland-try:-u all]

Updated

6 years ago
Whiteboard: [autoland-try:-u all] → [autoland-in-queue]

Comment 5

6 years ago
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

6 years ago
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

Updated

6 years ago
Whiteboard: [autoland-in-queue]
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.
Attachment #608450 - Attachment is obsolete: true
Attachment #608737 - Flags: review+
Whiteboard: [autoland-try:-u crashtest -b d]

Updated

6 years ago
Whiteboard: [autoland-try:-u crashtest -b d] → [autoland-in-queue]

Comment 8

6 years ago
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

6 years ago
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

Updated

6 years ago
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
Last Resolved: 5 years ago
Resolution: --- → FIXED

Comment 15

5 years ago
Should probably get mentioned in the docs, since this is considered a compatibility risk.
Keywords: dev-doc-needed

Updated

5 years ago
Depends on: 745543
Depends on: 279330

Updated

5 years ago
status-firefox14: --- → fixed

Updated

5 years ago
Depends on: 794201

Updated

5 years ago
Depends on: 829506

Updated

8 months ago
Depends on: 889940
You need to log in before you can comment on or make changes to this bug.