Last Comment Bug 77705 - add CSS support to Composer
: add CSS support to Composer
Status: VERIFIED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Composer (show other bugs)
: Trunk
: All All
: P3 enhancement (vote)
: mozilla0.9.8
Assigned To: Daniel Glazman (:glazou)
: sujay
:
Mentors:
: 59991 59993 97001 99775 (view as bug list)
Depends on: 62682 18894 34849 77882 78202 78709 79722 94078 94080 95062 97771 98159 98765 101925 102147 427340
Blocks: 97840 111531
  Show dependency treegraph
 
Reported: 2001-04-26 07:29 PDT by Daniel Glazman (:glazou)
Modified: 2008-04-05 22:27 PDT (History)
25 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Test case for HTML styles, CSS styles, mixed HTML+CSS styles (3.15 KB, text/html)
2001-05-02 04:27 PDT, Daniel Glazman (:glazou)
no flags Details
work-in-progress code v1 (12.92 KB, patch)
2001-05-02 06:44 PDT, Daniel Glazman (:glazou)
no flags Details | Diff | Splinter Review
test #2 for HTML and CSS text alignment (1.98 KB, text/html)
2001-05-04 07:34 PDT, Daniel Glazman (:glazou)
no flags Details
first shot for ChangeCSSInlineStyleTxn (11.91 KB, patch)
2001-05-10 06:39 PDT, Daniel Glazman (:glazou)
no flags Details | Diff | Splinter Review
patch adding "CSS editing" pref (2.22 KB, patch)
2001-05-21 02:29 PDT, Daniel Glazman (:glazou)
no flags Details | Diff | Splinter Review
work-in-progress code v2 (51.53 KB, patch)
2001-08-08 03:25 PDT, Daniel Glazman (:glazou)
no flags Details | Diff | Splinter Review
work-in-progress code v3 (58.23 KB, patch)
2001-08-09 07:16 PDT, Daniel Glazman (:glazou)
no flags Details | Diff | Splinter Review
button for Text Highlight (should be in /themes/modern/editor/btn2/highlight.gif) (903 bytes, image/gif)
2001-08-31 02:08 PDT, Daniel Glazman (:glazou)
no flags Details
work-in-progress v4 (this is getting bigger'n'bigger...) (69.91 KB, patch)
2001-08-31 02:10 PDT, Daniel Glazman (:glazou)
no flags Details | Diff | Splinter Review
PATH V 1.0 MANIFEST (2.35 KB, text/plain)
2001-10-18 06:13 PDT, Daniel Glazman (:glazou)
no flags Details
binary files (button icons) (4.55 KB, application/octet-stream)
2001-10-18 06:50 PDT, Daniel Glazman (:glazou)
no flags Details
PATCH v 1.0, ready for review !!! YAY !!!! (5929 lines) (230.21 KB, patch)
2001-10-18 08:00 PDT, Daniel Glazman (:glazou)
mozeditor: review+
Details | Diff | Splinter Review
Kin's review of new .cpp/.h files in PATCH v 1.0 (4.96 KB, text/plain)
2001-11-21 13:48 PST, kinmoz
no flags Details
review of new and changed .cpp and .h files in patch (9.56 KB, text/plain)
2001-12-06 04:35 PST, Joe Francis
no flags Details
Review for files starting at nsComposerCommands.cpp in patch (all UI) (12.67 KB, text/html)
2001-12-06 10:30 PST, Charles Manske
no flags Details
Review for files starting at nsComposerCommands.cpp in patch (all UI) (12.67 KB, text/html)
2001-12-06 10:33 PST, Charles Manske
no flags Details
Review for files starting at nsComposerCommands.cpp in pat (12.67 KB, text/html)
2001-12-06 10:41 PST, Charles Manske
no flags Details
Kin's review of changes to existing .cpp/.h files in PATCH v 1.0 (13.78 KB, text/plain)
2001-12-07 14:18 PST, kinmoz
no flags Details
new button icons for modern theme (15.32 KB, image/gif)
2002-01-07 05:58 PST, Daniel Glazman (:glazou)
no flags Details
new button icons for classic theme (3.07 KB, image/gif)
2002-01-07 05:59 PST, Daniel Glazman (:glazou)
no flags Details
MANIFEST FOR PATCH #2.0 (1.90 KB, text/plain)
2002-01-07 06:03 PST, Daniel Glazman (:glazou)
no flags Details
PATCH v2.0 (228.33 KB, patch)
2002-01-07 07:50 PST, Daniel Glazman (:glazou)
no flags Details | Diff | Splinter Review
PATCH v2.1 (228.33 KB, patch)
2002-01-07 08:45 PST, Daniel Glazman (:glazou)
no flags Details | Diff | Splinter Review

Description Daniel Glazman (:glazou) 2001-04-26 07:29:57 PDT
This is a tracking bug for the CSSization of Composer.

See also http://www.mozilla.org/editor/adding-css-to-editor.html for more
information about that.
Comment 1 Daniel Glazman (:glazou) 2001-04-26 07:31:42 PDT
B, I and U buttons now reflecting the real state of the selection even if
the corresponding styles come from CSS rules or CSS inline styles.

Now working on color and "Text Style" menu items.
Comment 2 Daniel Glazman (:glazou) 2001-04-26 07:35:27 PDT
adding mozilla1.0 keyword
Comment 3 Joe Francis 2001-04-26 10:51:22 PDT
I'm recalling a discussion we had a while ago where we talked about forking the 
css-editor off of the implementation class heirarchy.  Right now we have:
  nsEditor-->nsPlaintextEditor-->nsHTMLEditor

One simple possibility is just:
  nsEditor-->nsPlaintextEditor-->nsHTMLEditor-->nsCSSHTMLEditor

But if enough things are different then perhaps:
  nsEditor-->nsPlaintextEditor-->nsHTMLEditor
                              \->nsCSSHTMLEditor

is better.  Daniel, what is your plan there?  I don't think we should get rid of 
the existing non-CSS HTML editor, although I guess that is an option.
Comment 4 Daniel Glazman (:glazou) 2001-04-27 02:59:19 PDT
We definitely need to have another discussion about that now that I understand
much more all the handling of the "properties" in the html editor. I am open to
all options since I have no marketing input on that point. The only things I am
personnally sure about :

  - we have to keep non-css editing for email
  - html+css editor will reuse a lot of code from the pure html editor in
    particular when it has to deal with a page with pure html styles...

Comment 5 Daniel Glazman (:glazou) 2001-04-30 02:50:31 PDT
Color button now reflecting the real state of the selection even if
the color come from CSS rules or CSS inline styles.

Code optimization so nsIDOMViewCSS is queried only once per call to
|GetInlinePropertyBase| .
Comment 6 Daniel Glazman (:glazou) 2001-04-30 08:42:21 PDT
Menu items of Format > Font 

      Variable Width
      Fixed Width
      -----
      Helvetica, Arial
      Times
      Courier

now reflecting the real state of the selection even if
the style come from CSS rules or CSS inline styles.

Comment 7 Daniel Glazman (:glazou) 2001-04-30 09:40:49 PDT
state of U button buggy because of bug 78202; harishd working on it
Comment 8 Daniel Glazman (:glazou) 2001-05-02 04:09:03 PDT
Added very simple code to nsHTMLEditor so mail composition remains exactly as it
is while HTML document editor now takes advantage of css.
Comment 9 Daniel Glazman (:glazou) 2001-05-02 04:27:22 PDT
Created attachment 32869 [details]
Test case for HTML styles, CSS styles, mixed HTML+CSS styles
Comment 10 Daniel Glazman (:glazou) 2001-05-02 06:44:07 PDT
Created attachment 32873 [details] [diff] [review]
work-in-progress code v1
Comment 11 Daniel Glazman (:glazou) 2001-05-02 08:09:15 PDT
Preference for CSS in Composer added.
See http://www.mozilla.org/editor/adding-css-to-editor.html
Comment 12 Daniel Glazman (:glazou) 2001-05-03 06:45:38 PDT
The 4 alignment buttons and corresponding menu items now reflecting the real state
of the selection even if the alignment comes from CSS rules or CSS inline styles.

me discovered the -moz-center -moz-right (and so on) values for text-align CSS
property :-/

See also bug 78703.

+ Code optimization.

Comment 13 rubydoo123 2001-05-03 11:02:45 PDT
moving this to future to get this off the un-milestoned list, move to the 
correct milestone when you are ready.
Comment 14 Daniel Glazman (:glazou) 2001-05-04 07:34:57 PDT
Created attachment 33183 [details]
test #2 for HTML and CSS text alignment
Comment 15 R.K.Aa. 2001-05-06 15:53:17 PDT
There is a problem in Composer when it contains text formatted with CSS justify

 <style type="text/css">
 .justify {text-align: justify;

It becomes very hard to navigate inside the text with the mouse: Except for on
the first line after a .justify tag, the cursor is placed at beginning of line
instead of where i click. Perhaps this is known - mentioning just in case.
Comment 16 Daniel Glazman (:glazou) 2001-05-09 07:57:15 PDT
I totally forgot the STRIKE element ! Its corresponding menu item now reflecting
the real CSS state of the selection.

Comment 17 Daniel Glazman (:glazou) 2001-05-09 08:55:55 PDT
Currently finishing :

a) code for the retrieval of the CSS declarations contained in a STYLE attribute

   for that purpose, had to do implementation of nsDOMCSSDeclaration::GetCssText

b) a transaction for setting, removing, changing the value of a CSS declaration
   contained in a STYLE attribute

Comment 18 Daniel Glazman (:glazou) 2001-05-10 06:39:21 PDT
Created attachment 33869 [details] [diff] [review]
first shot for ChangeCSSInlineStyleTxn
Comment 19 Daniel Glazman (:glazou) 2001-05-10 06:46:57 PDT
First shot for ChangeCSSInlineStyleTxn attached above.

I chose, after short discussion with Joe, to make it as simple as possible even
if the footprint is for the moment too big :

    for Undo and Redo, I store the two corresponding values of the STYLE
    attribute (if it is set) and if it is set or not ; then undo and redo are
    just a matter of re-assigning the attribute's value or removing the
    attribute.

    In the future, Undo and Redo should be more cost-effective and use only
    the property, property value and if the property was set or not. It implies
    the recreation of the contents of the style attribute as in Do now, but
    the footprint will be much slower.

For the moment, let's do something that works and improve it afterwards.

Now working on nsHTMLEditor::SetInlineProperty.


Comment 20 Daniel Glazman (:glazou) 2001-05-14 14:14:37 PDT
Background-color button and "Page properties" menu item now manipulate CSS inline
style on the BODY or on tables and table sub-elements :-) First demo made today !
Comment 21 Daniel Glazman (:glazou) 2001-05-21 02:29:39 PDT
Created attachment 35381 [details] [diff] [review]
patch adding "CSS editing" pref
Comment 22 Daniel Glazman (:glazou) 2001-05-21 02:30:19 PDT
Just adding a very small patch about new CSS pref because I always forget where
it is defined.
Comment 23 Daniel Glazman (:glazou) 2001-05-21 07:23:33 PDT
First inline CSS styles generated on text node today :

    [this should be bold and [ this should be bold underlined [and italic]]]

1) select all text, click on B
2) select "[ this should be bold underlined [and italic]]", click on U
3) select "[and italic]", click on I
4) click on color, choose red

result :

   <span style="font-weight : bold ; ">[ this should be bold and
      <span style="text-decoration : underline ; ">[ this should be bold underlined
        <span style="font-style : italic ; color : rgb(255,0,0)">[ and
italic]</span>]</span>]</span>

:-)
Comment 24 Joe Francis 2001-05-21 10:04:04 PDT
sweet!
Comment 25 Hixie (not reading bugmail) 2001-05-22 01:05:12 PDT
Lovely structural markup! ;-)
Comment 26 Daniel Glazman (:glazou) 2001-05-22 03:57:01 PDT
RemoveStyleInside must be CSSized in order to get a working setter for CSS
properties. Investigating now. In the case of CSS editing, RemoveStyleInside
should remove both the HTML presentation hints (w/o CLASS or ID) and the
corresponding CSS declarations.

Problem to solve 1 : node agregator in case of equal styles should check both
HTML presentation hints and corresponding CSS styles.
Example :

   <p>bar <b>foo</b></p>
   <p><span style="font-weight : bold ; ">foo</span> bar</p>

if the caret is at the beginning of the second P and user hits backspace in CSS
mode, the result should be

   <p>bar <span style="font-weight : bold ; ">foofoo</span> bar</p>


Problem to solve 2 : the styles for B element (and others) are contained in the
UA stylesheet and I should not re-hardcode them. I will try to get the
corresponding style declarations directly from the UA stylesheet through the CSS
OM when initializing the editor's instance.

Comment 27 Daniel Glazman (:glazou) 2001-08-06 08:07:21 PDT
Now working on problem 1 (see above).
Comment 28 Daniel Glazman (:glazou) 2001-08-07 08:33:21 PDT
First removal of CSS inline styles :

  <span style="font-weight : bold; font-style : italic>foo bar foo</span>

select "bar" and click on B button; result in composer :

  <span style="font-style : italic"><span style="font-weight : bold;">foo</span>
    bar<span style="font-weight : bold;">foo</span></span>

:-)
Comment 29 Daniel Glazman (:glazou) 2001-08-08 03:25:13 PDT
Created attachment 45061 [details] [diff] [review]
work-in-progress code v2
Comment 30 Daniel Glazman (:glazou) 2001-08-08 03:28:52 PDT
The patch "work-in-progress code v2" attached above allows you set AND remove
CSS inline styles for B, U and I buttons. Be sure to have the new pref "CSS
editing" checked. Of course, if you are using CSS editing, it also deals with
B U and I elements.
This patch does not use the new ChangeCSSInlineStyleTxn transaction. Working on
it now.

Feedback welcome ;-)
Comment 31 Hixie (not reading bugmail) 2001-08-08 14:42:45 PDT
I don't want to be nasty or anything, but why is this:

  <span style="font-style : italic"><span style="font-weight : bold;">foo</span>
    bar<span style="font-weight : bold;">foo</span></span>

...better than this:

  <i><b>foo</b> bar<b>foo</b></i>

...?
Comment 32 timeless 2001-08-08 15:10:10 PDT
bigger is better!! </sarcasm>
Comment 33 rubydoo123 2001-08-08 16:36:52 PDT
well, the deprecated elements in 4.01 strict are being replaced with the
appropriate CSS. The bold and italic elements should not be, at least not at
this point. But, center, strike, underline and font will definetly be replaced,
and hopefully margins instead of blockquote for indent, and the appropriate
color for selection and page colors.

Before this gets checked in though, we need to ensure that this does not affect
mail compose, or at least not until they tell us tehy are ready for CSS
Comment 34 Hixie (not reading bugmail) 2001-08-08 19:46:52 PDT
O..k... I still don't see the point of using style attributes instead of
deprecated elements though. The elements are deprecated because they don't
convey structural information. Neither do "span" tags and "style" attributes.
If we're going to Do The Right Thing per the spec then we should be putting
stylistic information in an (external) stylesheet, and using structural markup
(<cite>, <ul>, <var>, <dfn>, <ins>, etc) to mark up the document.
Comment 35 Christopher Aillon (sabbatical, not receiving bugmail) 2001-08-08 20:27:04 PDT
Do we want to declare style="font-style: italic" for every text that needs to be
italicized?

Would it be better to define "common" classes such as bold, italic, boldItalic,
etc in an external stylesheet?
Comment 36 Daniel Glazman (:glazou) 2001-08-09 03:10:17 PDT
> Do we want to declare style="font-style: italic" for every text that needs to be
> italicized?
>
> Would it be better to define "common" classes such as bold, italic, boldItalic,
> etc in an external stylesheet?

That implies much more code because you have to handle an external object (the
stylesheet) and it restricts the classes namespace (you could start these
class names with a moz something).
Even if you make the stylesheet embedded into the document, it implies much more
code and complexity. I have to say that it implies stylesheet manipulation
through the DOM that is for the moment not fully reliable.
Comment 37 Daniel Glazman (:glazou) 2001-08-09 03:15:30 PDT
> O..k... I still don't see the point of using style attributes instead of
> deprecated elements though. The elements are deprecated because they don't
> convey structural information. Neither do "span" tags and "style" attributes.
> If we're going to Do The Right Thing per the spec then we should be putting
> stylistic information in an (external) stylesheet, and using structural markup
> (<cite>, <ul>, <var>, <dfn>, <ins>, etc) to mark up the document.

No, deprecated elements are deprecated because they convey presentational
information and not only structural information. spans and divs do carry
structural information only. The style attribute is a matter of conflict between
you Ian and us. We still think that the style attribute does not alter the
structure and that the web as we actually know it cannot live without it.

Handling external stylesheets is a much more difficult task than handling a
style attribute ; it is even harder with our DOM, which is not 100% reliable
on stylesheets. I am not sure that we will support the modification in Composer
of an external stylesheet in the two or three coming years.

Comment 38 Daniel Glazman (:glazou) 2001-08-09 03:17:52 PDT
> Before this gets checked in though, we need to ensure that this does not affect
> mail compose, or at least not until they tell us tehy are ready for CSS

Since all the code I wrote depends on an new boolean attribute of the editor
saying if CSS support is enabled or not, and since email never sets this boolean
to true...
Comment 39 Daniel Glazman (:glazou) 2001-08-09 03:27:01 PDT
> O..k... I still don't see the point of using style attributes instead of
> deprecated elements though.


I forgot this answer : being able to validate the document we create against
the strict dtd is a goal.
Comment 40 Daniel Glazman (:glazou) 2001-08-09 06:42:43 PDT
Added some magic so two adjacent SPANs are not merged if they have
different styles, or different class names, or if one of them carries an ID.

Example:

 <p>normal text<span style="font-weight: bold; ">bold text</span></p>
 <p><span style="font-style: italic; ">italic text</span>normal text</p>

put the caret at the beginning of 2nd paragraph and hit backspace.
The result should be:
  <p>normal text<span style="font-weight: bold; ">bold text</span>
  <span style="font-style: italic; ">italic text</span>normal text</p>
and not

  <p>normal text<span style="font-weight: bold; ">bold text
  italic text</span>normal text</p>
Comment 41 Daniel Glazman (:glazou) 2001-08-09 07:05:50 PDT
Code now only uses ChangeCSSInlineStyleTxn transaction.
Comment 42 Daniel Glazman (:glazou) 2001-08-09 07:16:56 PDT
Created attachment 45226 [details] [diff] [review]
work-in-progress code v3
Comment 43 Daniel Glazman (:glazou) 2001-08-09 08:41:33 PDT
Block alignment now uses 'text-align' CSS property instead of ALIGN attribute !
Comment 44 Hixie (not reading bugmail) 2001-08-09 15:03:28 PDT
> No, deprecated elements are deprecated because they convey presentational
> information and not only structural information. spans and divs do carry
> structural information only.

   <div style="font: 900 20px Tahoma; display: block; margin:1em">Response</div>
   <div>I disagree, I think <span style="font:1em monospace">div</span> and 
   <span style="font:1em monospace">span</span> elements do not carry any <span 
   style="font-style: italic">structural</span> information at all.</div>

vs:

   <h1>Response</h1>
   <p>I disagree, I think <code>div</code> and <code>span</code> elements do 
   not carry any <em>structural</em>information at all.</p>

Ask yourself this: which looks more like XSL:FOs, which we both agree are bad 
since they have no structural semantics?
Comment 45 Hixie (not reading bugmail) 2001-08-09 15:05:10 PDT
Christopher: Having presentational classes is almost as bad as having inline
presentational style. (The only advantage being that you would more easily be
able to mass-convert them to the more correct class names like "important",
"authorName" or whatever.)
Comment 46 Daniel Glazman (:glazou) 2001-08-10 03:28:23 PDT
Ian : h1, p, code and em are not deprecated ; why do you want us to turn them
into divs and spans plus CSS ? I know that you play devil's advocate but choose
better arguments, please ;-)
Comment 47 Daniel Glazman (:glazou) 2001-08-10 03:29:17 PDT
List properties dialog can now generate 'list-style-type' CSS property instead
of deprecated TYPE attribute when CSS pref is checked.
Comment 48 Daniel Glazman (:glazou) 2001-08-10 05:24:47 PDT
added code so list items can be aligned through 'text-align' instead of
creating a div inside. See also bug 66731.
Comment 49 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2001-08-10 08:58:32 PDT
> Block alignment now uses 'text-align' CSS property instead of ALIGN
> attribute !

*Block* alignment?  Shouldn't that use 'auto' margins?  Note that the ALIGN 
attribute in HTML maps to a very confusing combination of 'text-align' and 
'auto' margins that can't be simulated using CSS without significant use of 
inefficient descendant combinators.


My two cents -- I agree with Ian that <span style="font-weight: bold">...</span> 
is worse than <b>...</b>.  Both convey presentation (the same presentation), but 
the first is bulkier and demonstrates the wrong way to use CSS (as a simple 
search-replace of presentational HTML, rather than to style semantically rich 
markup).  Unless we're going to encourage the authors using the editor to say 
*why* they want something bold, we may as well just use the B element.  The 
reason B was deprecated was because it does exactly the same thing that <span 
style="font-weight: bold">...</span> does.

> I forgot this answer : being able to validate the document we create against
> the strict dtd is a goal.

Being able to validate against the HTML 4 Strict DTD only means that the 
document produced meets the requirements of the spec that can be expressed in 
the DTD.  If it does that by simply changing the violations (or use of 
deprecated/discouraged features) of the spec from violations that can be caught 
by a validator to those that can't be caught by a validator, it isn't really 
doing any good.  (HTML 4 section 15.2 says (emphasis mine) "Although they are 
not all deprecated, their use is discouraged in favor of style **sheets**.")
Comment 50 Hixie (not reading bugmail) 2001-08-10 15:48:24 PDT
> Ian : h1, p, code and em are not deprecated ; why do you want us to turn them
> into divs and spans plus CSS ? I know that you play devil's advocate but 
> choose better arguments, please ;-)

All your examples have used <span> and <div>. An editor which supports 
structural markup wouldn't have a way of inserting an <i> element, it would have
a way of inserting an <em> element *which could then be styled using a 
stylesheet editor*.

Plus, everything that dbaron said.
Comment 51 rubydoo123 2001-08-10 16:51:41 PDT
We will actually be supporting both HTML style elements and CSS. We are and will
continue to support bold, italic, etc., as long as they are not deprecated. In
particular, bold and italic have dual meanings - style and structure (in that it
denotes something is special). We have had numerous discussions on that in the
WG. I doubt they will be deprecated any time soon.

Utilizing span and div into the document is legal and remains legal, and we will
undoubtedly utilize those elements until they are also deemed deprecated. Again,
I doubt div would be deprecated because it does denote structure, it is
unfortunate that it has been incorrectly used in the past.
Comment 52 Daniel Glazman (:glazou) 2001-08-13 00:30:24 PDT
I forgot that text-decoration accepts more than one value :-( Fixing now...
Comment 53 Daniel Glazman (:glazou) 2001-08-13 04:11:23 PDT
Bug 95062 completely blocks correct handling of U button, Underline and
Strikethrough menu items.
Comment 54 Daniel Glazman (:glazou) 2001-08-29 02:44:08 PDT
(a) Still jetlagging, I worked this night on a declarative way of setting 
the equivalences between HTML and CSS in both directions. With that in,
deprecating a new element/attribute in favor of css should be feasible
with only adding just a few macros...

(b) now working on table attributes width, bgcolor, width and vertical-align
Comment 55 Daniel Glazman (:glazou) 2001-08-29 23:50:35 PDT
Just adding a ref to MS CSS Editor (part of Visual Interdev tool).
http://msdn.microsoft.com/library/default.asp?url=/library/en-us/vidref98/html/vihoweditingstylesheets.asp
Comment 56 Daniel Glazman (:glazou) 2001-08-29 23:52:46 PDT
Text highlight (ability to add background-color using picker to text and not
only blocks) added.
Comment 57 Daniel Glazman (:glazou) 2001-08-30 09:25:25 PDT
Added magic so Text Highlight button is enabled only if CSS is enabled
Comment 58 Charles Manske 2001-08-30 10:09:48 PDT
This stuff looks great! As soon as 0.9.4 is over, I'll play with it.
Thanks for the link to MS' stylesheet editor.
Noticing that you set default pref to "true" for CSS editing, 
do you plan to support autoconverting all non-CSS-using pages to use inline
style instead? Maybe have that as a second pref? I wonder if it's a problem in a
page that is mostly using deprecated HTML attributes, then we edit it and insert
only inline CSS. Maybe that pref(s) should be exposed during installation so
users are aware of that issue?
I know they want to minimize extra steps during installation, but which way 
Composer works is a very important issue, imho. (And we now have the right
manager to address this issue!)
Comment 59 Daniel Glazman (:glazou) 2001-08-30 13:24:25 PDT
cmanske : beppe invested a lot of time trying to convince me that it is not
always possible to translate styles done with HTML deprecated elements or
attributes into CSS inline styles (including embedded rules for some attributes
on BODY), but I admit I am still very skeptical on this point. So don't push
me too far, or I add it to my todo list ;-)

Anyway, this won't be in 1st release of CSS support in composer. A question of
priority and timeframe...
Comment 60 Daniel Glazman (:glazou) 2001-08-31 02:08:56 PDT
Created attachment 47784 [details]
button for Text Highlight (should be in /themes/modern/editor/btn2/highlight.gif)
Comment 61 Daniel Glazman (:glazou) 2001-08-31 02:10:36 PDT
Created attachment 47785 [details] [diff] [review]
work-in-progress v4 (this is getting bigger'n'bigger...)
Comment 62 Daniel Glazman (:glazou) 2001-08-31 02:20:13 PDT
New work-in-progress patch ; don't forget to add the gif file for the text
highlight button in themes/modern/editor/btn2 directory.
Comment 63 Daniel Glazman (:glazou) 2001-09-04 08:44:58 PDT
1. effect of bug 98159 : background color button for blocks does not work
   well for tables ; I also have a new patch for 97771.
   If you exclude the side effects on these bugs, the background button and
   the highlight button work just fine (I tortured them)

2. added some code so Bold and Italic can be removed even if the style comes
   from a CSS **rule** ; in that case, adding the css property with 'normal'
   value.

3. started working on the UI dialogs... Not an easy task because attributes
   are often directly set from js

4. started working on indentation
Comment 64 Daniel Glazman (:glazou) 2001-09-06 02:11:17 PDT
1. Because nsDOMCSSDeclaration::GetPropertyCSSValue is not implemented, I had to
   write a small parser for the length values of margin-left

2. With that in, I now have a cssized version of the Indent toolbar button.
   Here are the increments I am using for each unit type :

      px  40
      cm  1.05
      mm  10.5
      pc  2.48
      pt  29.76
      in  0.4134
      em  3
      ex  6
      %   4

3. now working on Outdent button and enabling/disabling of indent button
Comment 65 Daniel Glazman (:glazou) 2001-09-06 03:12:52 PDT
enabling/disabling of outdent button now cssized
Comment 66 Daniel Glazman (:glazou) 2001-09-06 03:26:49 PDT
*** Bug 97001 has been marked as a duplicate of this bug. ***
Comment 67 Daniel Glazman (:glazou) 2001-09-06 05:44:46 PDT
YAY !! Outdent button now cssized !
Comment 68 Daniel Glazman (:glazou) 2001-09-10 07:28:28 PDT
YAY AGAIN : Table Properties dialog now generates CSS properties instead of HTML
attributes.... me can now resurrect the "Table height" property :-))))
Comment 69 Boris Zbarsky [:bz] (still a bit busy) 2001-09-15 09:58:45 PDT
*** Bug 99775 has been marked as a duplicate of this bug. ***
Comment 70 Daniel Glazman (:glazou) 2001-09-24 08:14:11 PDT
The table properties dialog was generating CSS styles but was unable to update
itself *from* CSS styles ; I just found the way to do that and it already works
for vertical and horizontal alignment; will be a little bit trickier for
background-color but not a strong problem.
Comment 71 Daniel Glazman (:glazou) 2001-09-26 07:51:21 PDT
Page Color and Background dialog now CSSized
Comment 72 Daniel Glazman (:glazou) 2001-10-05 08:44:05 PDT
All dialogs now CSSized (yipie !). That includes CSSization of
   height     on    TD TH
   nowrap     on    TD TH
   size       on    HR
   align      on    CAPTION and LEGEND (see bug 91491)
   height     on    TABLE (YES !!!!! and the UI appears only in CSS mode :-))
   type       on    LI OL UL
   align      on    HR
   width      on    TABLE TD TH HR

Attaching a table summary of deprecated attributes.

Remaining issues :
   (1) handle <font size=".."> ; that's the big thing...
   (2) a bug in GetAlignment()
   (3) the Text Highlight button is not disabled when I uncheck the CSS pref
   (4) code cleanup
and that's all !
Comment 73 Daniel Glazman (:glazou) 2001-10-08 08:15:01 PDT
I have delayed a little bit the issues above because I have a fix for bug 18894
and am now able to CSSize attribute BORDER on IMG and will probably be able to
do so for attributes HSPACE and VSPACE :-)
Comment 74 Daniel Glazman (:glazou) 2001-10-17 05:26:27 PDT
*** Bug 59993 has been marked as a duplicate of this bug. ***
Comment 75 Daniel Glazman (:glazou) 2001-10-18 06:13:18 PDT
Created attachment 54053 [details]
PATH V 1.0 MANIFEST
Comment 76 Daniel Glazman (:glazou) 2001-10-18 06:50:20 PDT
Created attachment 54057 [details]
binary files (button icons)
Comment 77 Daniel Glazman (:glazou) 2001-10-18 08:00:26 PDT
Created attachment 54060 [details] [diff] [review]
PATCH v 1.0, ready for review !!! YAY !!!! (5929 lines)
Comment 78 Daniel Glazman (:glazou) 2001-10-19 08:33:24 PDT
*** Bug 59991 has been marked as a duplicate of this bug. ***
Comment 79 Daniel Glazman (:glazou) 2001-10-30 00:29:37 PST
*** Bug 106480 has been marked as a duplicate of this bug. ***
Comment 80 Daniel Glazman (:glazou) 2001-11-07 01:13:06 PST
bulk milestone change
Comment 81 kinmoz 2001-11-21 13:48:54 PST
Created attachment 58761 [details]
Kin's review of new .cpp/.h files in PATCH v 1.0

Here's an attatchment of my review of the new .cpp/.h files in Patch v 1.0:

    ChangeCSSInlineStyleTxn.cpp
    ChangeCSSInlineStyleTxn.h
    nsHTMLCSSUtils.cpp
    nsHTMLCSSUtils.h
Comment 82 Charles Manske 2001-12-04 12:17:17 PST
I've reviewed all files in the patch starting at:
editor/composer/src/nsComposerCommands.cpp
and mailed comments to glazman.
Comment 83 Joe Francis 2001-12-06 04:35:55 PST
Created attachment 60619 [details]
review of new and changed .cpp and .h files in patch

I tried to concentrate on the big picture of how it all works.	I hope you find
my questions and comments helpful.  Good job Daniel!
Comment 84 Joe Francis 2001-12-06 04:56:04 PST
Comment on attachment 54060 [details] [diff] [review]
PATCH v 1.0, ready for review !!! YAY !!!! (5929 lines)

r=jfrancis
Comment 85 Daniel Glazman (:glazou) 2001-12-06 05:56:33 PST
> RemoveInlinePropertyImpl(): around line 1299, and later at 1371, there
> is code for removing css style when there is no matching html style in
> the ancestry.  But this code only works for bold, right?  (As it is
> the only one indicated by IsCSSInvertable()).  So does that mean that
> italic, etc, if set by a rule rather than a style attribute, simply
> cannot be inverted (unitalicized, etc) in the css editor?

That's correct for underline. Example : <p style="font-style: italic">a b c</p>
The only way to make "b" roman instead of italic would be
<p><span style="font-style: italic">a</span> b 
   <span style="font-style: italic">c</span></p>

I am not at all in favour of it because it's really not the same thing. It's
also much harder to code.

See first paragraph of
http://www.w3.org/TR/REC-CSS2/text.html#lining-striking-props after property's
definition.

For other inline styles : totally correct, I stupidly forgot some of them.
Where was my brain that day... Thanks for detecting that.
Comment 86 Charles Manske 2001-12-06 10:30:11 PST
Created attachment 60655 [details]
Review for files starting at nsComposerCommands.cpp in patch (all UI)
Comment 87 Charles Manske 2001-12-06 10:33:02 PST
Created attachment 60658 [details]
Review for files starting at nsComposerCommands.cpp in patch (all UI)
Comment 88 Charles Manske 2001-12-06 10:41:35 PST
Created attachment 60662 [details]
Review for files starting at nsComposerCommands.cpp in pat
Comment 89 Charles Manske 2001-12-06 10:49:23 PST
Comment on attachment 60655 [details]
Review for files starting at nsComposerCommands.cpp in patch (all UI)

sorry for duplicate attachments -- it seemed like the files weren't sent, so I
sent it again!
Comment 90 Charles Manske 2001-12-06 11:07:17 PST
Comment on attachment 54060 [details] [diff] [review]
PATCH v 1.0, ready for review !!! YAY !!!! (5929 lines)

r=cmanske for UI work, with changed described in my review above
Comment 91 kinmoz 2001-12-07 14:18:27 PST
Created attachment 60844 [details]
Kin's review of changes to existing .cpp/.h files in PATCH v 1.0
Comment 92 Daniel Glazman (:glazou) 2001-12-13 02:34:38 PST
098
Comment 93 Daniel Glazman (:glazou) 2001-12-17 04:46:05 PST
just wanting to thank Joe, Charley and especially Kin for their excellent reviews.
sorry for the spam.
Comment 94 Daniel Glazman (:glazou) 2001-12-18 05:28:45 PST
1. Finally found my way out of merging conflict nightmare
2. all issues raised by Kin solved
3. now working on Joe's and Charley's reviews
Comment 95 Daniel Glazman (:glazou) 2001-12-18 08:28:29 PST
1. all issues raised by Charley now solved
2. working now on second half of Joe's comments, first half already done
3. expect to have a sr'able patch *by the end of this week*
   (unfortunately, I don't think I can make better than that).
4. preparing a list of testcases ; they will have to be all green for check-in

ALL EDITOR FOLKS : once this code gets its sr=, we'll have to choose a date
for this carpool landing and I'll ask you that day to block all your check-ins
until my code is in ; thanks a lot :-)

Comment 96 Daniel Glazman (:glazou) 2001-12-21 09:07:07 PST
Sorry to say, but the recent check-ins in chrome and some modifs in Composer's js
code generated bugs or unexpected behaviors in my code... This patch won't be fully
ready in 2001 even if the remaining points to solve are unsignificant.
Comment 97 Daniel Glazman (:glazou) 2002-01-04 08:35:24 PST
I am almost done with this :-) I am testing the code to see if everything is ok ;
doing that I discovered that I was not removing useless divs on outdent to 0. This
is now added.
Comment 98 Daniel Glazman (:glazou) 2002-01-07 05:56:09 PST
Comment on attachment 54053 [details]
PATH V 1.0 MANIFEST

obsolete manifest
Comment 99 Daniel Glazman (:glazou) 2002-01-07 05:56:34 PST
Comment on attachment 54057 [details]
binary files (button icons)

obsolete icons
Comment 100 Daniel Glazman (:glazou) 2002-01-07 05:57:08 PST
Comment on attachment 54060 [details] [diff] [review]
PATCH v 1.0, ready for review !!! YAY !!!! (5929 lines)

obsolete patch
Comment 101 Daniel Glazman (:glazou) 2002-01-07 05:58:43 PST
Created attachment 63803 [details]
new button icons for modern theme

new version of themes\modern\editor\icons\btn2.gif needed for patch 2.0
Comment 102 Daniel Glazman (:glazou) 2002-01-07 05:59:40 PST
Created attachment 63804 [details]
new button icons for classic theme

new version of themes\classic\editor\icons\btn2.gif needed for patch 2.0
Comment 103 Daniel Glazman (:glazou) 2002-01-07 06:03:49 PST
Created attachment 63805 [details]
MANIFEST FOR PATCH #2.0

This attachment contains the list of files modified or added by patch #2.0.
The two binary files themes/classic/editor/icons/btn2.gif and
themes/modern/editor/icons/btn2.gif not listed here.
Comment 104 Daniel Glazman (:glazou) 2002-01-07 07:50:56 PST
Created attachment 63816 [details] [diff] [review]
PATCH v2.0


patch v2.0 ; needs testing on Macintosh(with help of my favorite MacBuddy,
Peter)
Comment 105 Daniel Glazman (:glazou) 2002-01-07 08:45:35 PST
Created attachment 63820 [details] [diff] [review]
PATCH v2.1


PATCH v2.1, obsoletes 2.0, thanks to ToLowerCase() ...

This one is (sniff, I am so happy), ready for sr= !
Kin, Joe Charley : all your comments are in. :-)

I respectfully suggest to deal with ALL new minor issues (i.e. non
showstoppers),
including code cleanup or reorg, in new bugs filed AFTER this code gets in.
Thanks.
Comment 106 Joe Francis 2002-01-07 15:07:22 PST
Daniel, what was the result, if any, of my review comment:
"libeditor/html/nsHTMLEditor.cpp & h:

NodeIsBlockStatic(): I realize table, thead, etc are actually
something a bit different from blocks, but I fear removing them
from the block list will break whitespace handling and other
things.  This problem is serious enough that we need to plan out
what to do here prior to landing."

I think we may need to make a "NodeIsBlockOrTable()" and replace all the 
prexisting NodeIsBlock() calls with it.
Comment 107 Daniel Glazman (:glazou) 2002-01-08 01:09:15 PST
Oh, right, I forgot to give you my answer to this point ; here it is, sorry for
that :

  I don't understand your point. Here is the diff chunk for NodeIsBlockStatic() :

<chunk>
@@ -586,19 +597,19 @@
           tagAtom==nsIEditProperty::noscript   ||
           tagAtom==nsIEditProperty::form       ||
           tagAtom==nsIEditProperty::hr         ||
-          tagAtom==nsIEditProperty::table      ||
           tagAtom==nsIEditProperty::fieldset   ||
           tagAtom==nsIEditProperty::address    ||
           tagAtom==nsIEditProperty::body       ||
+          tagAtom==nsIEditProperty::caption    ||
+          tagAtom==nsIEditProperty::table      ||
+          tagAtom==nsIEditProperty::tbody      ||
+          tagAtom==nsIEditProperty::thead      ||
+          tagAtom==nsIEditProperty::tfoot      ||
           tagAtom==nsIEditProperty::tr         ||
           tagAtom==nsIEditProperty::td         ||
           tagAtom==nsIEditProperty::th         ||
-          tagAtom==nsIEditProperty::caption    ||
           tagAtom==nsIEditProperty::col        ||
           tagAtom==nsIEditProperty::colgroup   ||
-          tagAtom==nsIEditProperty::tbody      ||
-          tagAtom==nsIEditProperty::thead      ||
-          tagAtom==nsIEditProperty::tfoot      ||
           tagAtom==nsIEditProperty::li         ||
           tagAtom==nsIEditProperty::dt         ||
           tagAtom==nsIEditProperty::dd         ||
</chunk>

  table, thead, ... are _not_ removed. I just _moved_ them and gathered
  table elements for readability reasons.
Comment 108 Daniel Glazman (:glazou) 2002-01-08 01:45:09 PST
Patch successfully tested on Mac.
Comment 109 kinmoz 2002-01-08 16:17:48 PST
Looks like you addressed most of my comments in v2.1 of your patch, I still
have some questions/comments, some are from my review of the 1.0 patch ...
that said I'm willing to give you a conditional sr=kin if you get an r=jfrancis.


===========================
==== TransactionFactory.cpp
===========================


+  else if (aTxnType.Equals(ChangeCSSInlineStyleTxn::GetCID()))
+    *aResult = new ChangeCSSInlineStyleTxn();

This code should probably be moved up under the #ifndef
MOZILLA_PLAINTEXT_EDITOR_ONLY
like SetDocTitleTxn.


===================================
==== nsEditor.cpp and nsIEditor.idl
===================================


I'd rather we didn't add CSS'isms to nsEditor/nsIEditor, but for the sake
of time/checkin and if it's ok with joe perhaps it's ok to land it anyways
with the promise that you'll make it go away or use some sort of generic
mechanism. :-) 


=====================
==== nsHTMLEditor.cpp
=====================


+    // the HTML Editor is CSS-aware only in the case of Composer
+    mCSSAware  = PRBool(0 == aFlags);


Is it a safe assumption that composer will never have flags set?


========================
==== nsHTMLEditRules.cpp
========================


Is this change in GetNodesForOperation() going to impact the !usCSS case?


@@ -4550,8 +4811,7 @@

   // certain operations should not act on li's and td's, but rather inside
   // them.  alter the list as needed
-  if ( (inOperationType == kMakeBasicBlock)  ||
-       (inOperationType == kAlign) )
+  if (inOperationType == kMakeBasicBlock)
   {
     PRUint32 listCount;
     (*outArrayOfNodes)->Count(&listCount);


====


You changed the semantics of RemoveAlignmentInside() so perhaps the comment
above the function should be updated to reflect the modifications you made.
I'm wondering if we should drop the "Inside" in it's name since it no longer
just processes the children inside the node.
Comment 110 Daniel Glazman (:glazou) 2002-01-09 00:34:28 PST
Just for the record, Joe already gave his r= in comment #84...
Comment 111 Daniel Glazman (:glazou) 2002-01-09 01:30:55 PST
Answers to last Kin's comments :

> ===========================
> ==== TransactionFactory.cpp
> ===========================
> 
> 
> +  else if (aTxnType.Equals(ChangeCSSInlineStyleTxn::GetCID()))
> +    *aResult = new ChangeCSSInlineStyleTxn();
> 
> This code should probably be moved up under the #ifndef
> MOZILLA_PLAINTEXT_EDITOR_ONLY
> like SetDocTitleTxn.

done

> ===================================
> ==== nsEditor.cpp and nsIEditor.idl
> ===================================
> 
> 
> I'd rather we didn't add CSS'isms to nsEditor/nsIEditor, but for the sake
> of time/checkin and if it's ok with joe perhaps it's ok to land it anyways
> with the promise that you'll make it go away or use some sort of generic
> mechanism. :-) 

Promised.

> =====================
> ==== nsHTMLEditor.cpp
> =====================
> 
> 
> +    // the HTML Editor is CSS-aware only in the case of Composer
> +    mCSSAware  = PRBool(0 == aFlags);
> 
> Is it a safe assumption that composer will never have flags set?

That's the only way, at this step, I can make the difference between an
initialization for Composer or for html mail composer.  See
nsEditorShell::InstantiateEditor. Let's deal with this issue later.

> ========================
> ==== nsHTMLEditRules.cpp
> ========================
> 
> 
> Is this change in GetNodesForOperation() going to impact the !usCSS case?

No.  The alignment algos have been modified for that purpose.  The modification
of GetNodesForOperation() was absolutely needed for the css case.

> You changed the semantics of RemoveAlignmentInside() so perhaps the comment
> above the function should be updated to reflect the modifications you made.
> I'm wondering if we should drop the "Inside" in it's name since it no longer
> just processes the children inside the node.

Was already done, from your previous comments.
Comment 112 Daniel Glazman (:glazou) 2002-01-09 06:43:50 PST
It's good to see a so long work end with a so short comment :

     FIXED, CHECKED-IN !!!!

doobeedoobeedoo :-)
Comment 113 Joe Francis 2002-01-09 09:37:14 PST
oops.  As Daniel states, i misread the patch at NodeIsBlockStatic().  Carry on.  
Nothing to see here.  Move along people...
Comment 114 Blake Ross 2002-01-09 10:48:20 PST
You may want to have a xul person review your xul changes in the future. Your 
patch added some <text class="label"/>, and reverted some <label/> tags (which 
are correct) to <text/> as well.  The text tag is deprecated, in fact it 
shouldn't even be working anymore. This needs to be fixed. Do you want to do 
that here, or as part of a separate bug?

Also, you don't need the prefattribute and preftype attributes on a <checkbox/>.
Comment 115 Charles Manske 2002-01-09 11:22:14 PST
I reviewed the XUL a few weeks ago, but I'll take responsibility for not telling
Daniel about using <label> instead of <text>
Comment 116 Daniel Glazman (:glazou) 2002-01-10 05:29:33 PST
blaker: thanks for spotting this out; issue now covered by bug 119164

cmanske: a second beer ? ;-)
Comment 117 André Dahlqvist 2002-01-27 09:11:18 PST
Why was this tracker bug resolved as fixed where there are still dependencies
that are not fixed? Is there another tracker bug that covers them now?
Comment 118 sujay 2002-02-12 14:51:19 PST
Daniel, please verify this one...thanks.
Comment 119 Daniel Glazman (:glazou) 2002-02-12 20:01:01 PST
verified

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