202037 - switching useCSS and executing commands issue; mixing html tags and css is broken

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Doron Rosenberg (IBM)]

Hard to summarize:

Set useCSS to false, then select some text in midas and set it to bold.  This
creates <b>foo</b>.  Switch back to css mode, then italisize it. This creates 

<b style="font-style: italic;">foo</b>

Now bold it again, this removes the <b> tags.  It also removes the italic css
inline declaration.

Not a huge issue, since switching css/markup is probably not going to happen
other than during initialization.

Comment 1

[Kathleen :Brade]

If users edit existing documents and have their composer prefs set to css
(default for new profiles), then they'll run into this problem.

-->glazman

Comment 2

[Samir Gehani]

adt: nsbeta1-

Comment 3

[Kevin McCluskey (gone)]

Daniel, will this be an issue for embeddors that want to want to set a
background color for text while in HTML editing mode. (Do they temporarily set
the editor to CSS mode just to add the CSS property to set the background color
then flip back to HTML mode?)

Comment 4

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Wow. I must admit I never thought of such an edge case...

CSS editing has been in Composer for almost a year and a half now and nobody
ever hit that problem. So I don't think it's a very serious issue.

I have a fairly simple fix in mind though, so I will try it.
Accepting bug, but I don't think this is should be a blocker or a high priority
bug.

Comment 5

[Doron Rosenberg (IBM)]

What about the point Kevin brought up? someone editing a html formated document
(<b>foo</b>) and edits it in css mode?  Its probably the only case that could
actually happen as editor becomes more and more popular :)

Comment 6

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Doron: Working on it

Comment 7

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Attachment: fix #1

Comment 8

[Kai Engert [:KaiE:]]

Comment on attachment 125028 [details] [diff] [review]
fix #1

I can't comment whether this code is inserted at the right place in the
sources...

But the added is straightforward to understand and seems reasonable.

r=kaie

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 125028 [details] [diff] [review]
fix #1

+	 if (NS_FAILED(res)) return res;

Lines of the form shown above have the problem that it's impossible to set a
breakpoint on the then clause.	In general, I'm all for preserving style in a
file, but this style causes an actual loss of functionality for the developer.

Change that, and you've got sr=dmose.

Comment 10

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Attachment: fix #2

thanks dmose, but I did a slighly better patch dealing with the case of a class

attribute too. To answer a question that my peers may ask : no I did not want
to
deal with an ID on purpose because (a) IDs are very rarely set on b/i/u
elements
(b) associated to stylesheets, they are too specific (ie they designate too
precisely one element and one _only_) to be preserved when doing such a
transformation.

Comment 11

[Kai Engert [:KaiE:]]

Comment on attachment 125764 [details] [diff] [review]
fix #2

looks good.
r=kaie

But I personally would understand the IDL comment better if you changed it from
  "and delete those not in the source."
to
  "or deletes it from the destination node, if the source node does not contain
it"

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 125764 [details] [diff] [review]
fix #2

sr=[e-mail] with the minor changes we discussed on irc

Comment 13

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

checked in, trunk.