215406 - Remove all text styles menu item and key binding both no longer working

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect, P1)

Description

[Richard A. Wells]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.5a) Gecko/20030718
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.5a) Gecko/20030718

The re-implementation of style support in 1.5a seems to have broken the
Format->Remove All Text Styles menu item, at least on Windows.  Neither the menu
item nor the key binding seemt to do anything.  Toggling text styles using other
formatting commands (bold, italic, fixed width) work correctly, but clearing
them all in one swell foop no longer works.

Reproducible: Always

Steps to Reproduce:
1. Create new composer page
2. Type a line of text, applying text styles to some of the text
3. Select the line
4. Format->Remove All Text Styles


Actual Results:  
Nothing changes in the selected text.

Expected Results:  
Remove all text styles from the selected text.

Comment 1

[Kathleen :Brade]

It'd be nice to get a timeframe for when this broke.

Comment 2

[Joe Francis]

investigating...

Comment 3

[Joe Francis]

Teh culprit is:

nsHTMLEditorStyle.cpp
1.53	glazman%netscape.com	Jun 17 01:45	 	Removing text styles in CSS mode was
potentially removing too much if the styles were added in HTML mode
The fix creates a span element that will carry the inline styles and class of
the HTML element to be removed, if any.
b=202037, r=kaie, sr=dmose

Code was added to preserve additional inline style attributes on html style
nodes that were going away.  However, there is no check to see if this
additional style is in fact the style we are trying to remove.  

Short fixes: backout change, or remove the style attribute copying but keep the
class attribute checking.

More correct fix: check to see if style is one we are trying to remove, and if
so don't preserve it.  Note that (aProperty == nsnull) implies that *all* styles
are being removed.

Daniel, do you want to fix this, or should I?

Comment 4

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

Taking

Comment 5

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

excellent diagnosis from Joe, as usual... Easy fix pending.

Comment 6

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

Attachment: fix #1

Comment 7

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

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

Joe, can you review this simple fix, please ?
David, can you sr please ?
Thanks!

Comment 8

[Joe Francis]

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

I'm confused about this part:
+      if (aProperty &&
+	   (hasStyleAttr || hasClassAtrr)) {

Does this mean we will remove spans that have class attribute when we are
removing all styles?  Is that ok?

Comment 9

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

Right. I think it is fair to remove class on inline-level nodes when we
remove all styles. It _may_ remove styles that are not textual (a border for
instance) but we have no way to remove the "textual" styles attached through
a class while keeping the others...

Comment 10

[Joe Francis]

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

ok, sounds good.  r=mozeditor

Comment 11

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

checked in (trunk)