Open Bug 746166 Opened 8 years ago Updated 5 years ago

Remove use of cmd_backgroundColor from comm-central

Categories

(SeaMonkey :: Composer, enhancement)

enhancement
Not set

Tracking

(Not tracked)

ASSIGNED

People

(Reporter: ayg, Assigned: neil)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Bug 205485 made the backcolor command work the same as hilitecolor.  This means that cmd_backgroundColor is no longer touched by anything web-facing, but comm-central still uses it, at least here:

http://mxr.mozilla.org/comm-central/source/editor/ui/composer/content/editor.js#1156

Can this be removed?  If so, we could remove cmd_backgroundColor completely (bug 746165).
CC some people who may know the answers.
My historical understanding is this:
cmd_backgroundColor: gets/sets the background of the page, table or cell.
cmd_highlight: gets/sets the background of the selection. Requires CSS enabled.

What is your proposal?
cmd_backgroundColor is no longer exposed to web content -- document.execCommand("backcolor") now behaves identically to document.execCommand("hilitecolor").  Both of them are cmd_highlight.  So there are no users of cmd_backgroundColor in mozilla-central anymore, but there are in comm-central.  If comm-central could replace those users with something else, like maybe directly setting the body's bgcolor attribute, we could remove a bunch of code from mozilla-central.  That's all.

(Incidentally, cmd_highlight now works with CSS disabled too -- bug 279330.)
(In reply to Aryeh Gregor from comment #3)
> cmd_backgroundColor is no longer exposed to web content --
> document.execCommand("backcolor") now behaves identically to
> document.execCommand("hilitecolor").  Both of them are cmd_highlight.  So
> there are no users of cmd_backgroundColor in mozilla-central anymore, but
> there are in comm-central.  If comm-central could replace those users with
> something else, like maybe directly setting the body's bgcolor attribute, we
> could remove a bunch of code from mozilla-central.  That's all.
I looked at the code and it looks as if we only use it to query the background colour, not to set it, so maybe we could convert that code from C++ to JS.

> (Incidentally, cmd_highlight now works with CSS disabled too -- bug 279330.)
As in, "inserts the CSS that it would have done in CSS enabled mode". I guess CSS enabled mode is a bit of a misnomer, since it only affects some styles.
"That code" turns out to be nsBackgroundColorStateCommand, which looks like it would be fairly easy to convert to JavaScript.
Attached patch Proposed patch (obsolete) — Splinter Review
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #621659 - Flags: review?(ayg)
Version: unspecified → Trunk
Comment on attachment 621659 [details] [diff] [review]
Proposed patch

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

::: editor/ui/composer/content/ComposerCommands.js
@@ +3989,5 @@
> +  },
> +
> +  getCommandStateParams: function(aCommand, aParams, aRefCon) {
> +    var mixed = {};
> +    var color = GetCurrentEditor().getBackgroundColorState(mixed);

This still relies on nsHTMLEditor::GetBackgroundColorState, which has no reachable callers in mozilla-central.  Ideally I'd like to remove that too, and GetHTMLBackgroundColorState, and the aBlockLevel = true variant of GetCSSBackgroundColorState.  Would it be possible for you to write an implementation that doesn't rely on those?  If not, this patch is still an improvement -- thanks!  We could always take it and file a followup bug to remove callers of the additional methods.
Attachment #621659 - Flags: review?(ayg) → feedback+
Attached patch Possible patchSplinter Review
OK, so this version uses the same code that the action on the button uses to determine the colour to preselect in the colour picker, which prefers a table cell or table to any other sort of background colour.
Attachment #621659 - Attachment is obsolete: true
Attachment #623497 - Flags: review?(iann_bugzilla)
Attachment #623497 - Flags: feedback?(ayg)
Comment on attachment 623497 [details] [diff] [review]
Possible patch

LGTM.  Thanks!
Attachment #623497 - Flags: feedback?(ayg) → feedback+
Comment on attachment 623497 [details] [diff] [review]
Possible patch

r=me
Seems to do the correct thing but not 100% sure without the code this replaces being removed.
Attachment #623497 - Flags: review?(iann_bugzilla) → review+
Bug 746165 now has a patch that removes the dead code from m-c, so you can test out whether this patch does the right thing.  Obviously, I won't land that until this is landed.
No longer blocks: 746165
Blocks: 746165
Aargh! Now composer can't change the background color of a table or table cell if it has a background-color property set. It adds/removes the bgcolor attribute, but the style overrides that. Seamonkey 11 (Beta) on Windows.
(In reply to Shanti Rao from comment #12)
> Aargh! Now composer can't change the background color of a table or table
> cell if it has a background-color property set. It adds/removes the bgcolor
> attribute, but the style overrides that. Seamonkey 11 (Beta) on Windows.
Depending on your CSS setting (in Preferences) Composer will alter either the attribute or the style and ignore the other. This is not new behaviour.
Ping for this.
(In reply to Yonggang Luo from comment #14)
> Ping for this.

I found a bug in the patch. I haven't got around to seeing whether it can be fixed.
You need to log in before you can comment on or make changes to this bug.