Closed Bug 77705 Opened 24 years ago Closed 23 years ago

add CSS support to Composer

Categories

(SeaMonkey :: Composer, enhancement, P3)

Product:
SeaMonkey
Component:
Composer
Type:
enhancement

Tracking

(Not tracked)

Status:
VERIFIED FIXED
Milestone:
mozilla0.9.8

People

(Reporter: glazou, Assigned: glazou)

References

(Depends on 1 open bug)

Details

Attachments

(10 files, 13 obsolete files)

Test case for HTML styles, CSS styles, mixed HTML+CSS styles
3.15 KB, text/html
Details
test #2 for HTML and CSS text alignment
1.98 KB, text/html
Details
Kin's review of new .cpp/.h files in PATCH v 1.0
4.96 KB, text/plain
Details
review of new and changed .cpp and .h files in patch
9.56 KB, text/plain
Details
Review for files starting at nsComposerCommands.cpp in pat
12.67 KB, text/html
Details
Kin's review of changes to existing .cpp/.h files in PATCH v 1.0
13.78 KB, text/plain
Details
new button icons for modern theme
15.32 KB, image/gif
Details
new button icons for classic theme
3.07 KB, image/gif
Details
MANIFEST FOR PATCH #2.0
1.90 KB, text/plain
Details
PATCH v2.1
228.33 KB, patch
Details | Diff | Splinter Review 
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.
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.
Status: NEW → ASSIGNED
adding mozilla1.0 keyword
Keywords: mozilla1.0
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.

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...


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| .
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.


state of U button buggy because of bug 78202; harishd working on it
Added very simple code to nsHTMLEditor so mail composition remains exactly as it
is while HTML document editor now takes advantage of css.


  

  



    
    

    
  

      
      
      
      

        Attached patch
          
          
        work-in-progress code v1 (obsolete)
        — 
          — Splinter Review
      

    


  

  

  



    
    

    
  
Preference for CSS in Composer added.
See http://www.mozilla.org/editor/adding-css-to-editor.html


  

  



    
    

    
  
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.



  

  



    
    

    
  
moving this to future to get this off the un-milestoned list, move to the 
correct milestone when you are ready.

  

  


Priority: -- → P3
Target Milestone: --- → Future

    
    

    
  


  

  

  



    
    

    
  
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.

  

  



    
    

    
  
I totally forgot the STRIKE element ! Its corresponding menu item now reflecting
the real CSS state of the selection.



  

  



    
    

    
  
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



  

  



    
    

    
  


  

  

  



    
    

    
  
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.




  

  



    
    

    
  
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 !

  

  



    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch adding "CSS editing" pref (obsolete)
        — 
          — Splinter Review
      

    


  

  

  



    
    

    
  
Just adding a very small patch about new CSS pref because I always forget where
it is defined.

  

  



    
    

    
  
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>

:-)

  

  



    
    

    
  
sweet!


  

  



    
    

    
  
Lovely structural markup! ;-)

  

  



    
    

    
  
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.



  

  



    
    

    
  
Now working on problem 1 (see above).

  

  



    
    

    
  
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>

:-)

  

  



    
    

    
  

      
      
      
      

        Attached patch
          
          
        work-in-progress code v2 (obsolete)
        — 
          — Splinter Review
      

    


  

  

  



    
    

    
  
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 ;-)


  

  



    
    

    
  
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>

...?

  

  



    
    

    
  
bigger is better!! </sarcasm>

  

  



    
    

    
  
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

  

  



    
    

    
  
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.

  

  



    
    

    
  
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?

  

  



    
    

    
  
> 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.

  

  



    
    

    
  
> 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.



  

  



    
    

    
  
> 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...


  

  



    
    

    
  
> 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.

  

  



    
    

    
  
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>

  

  



    
    

    
  
Code now only uses ChangeCSSInlineStyleTxn transaction.

  

  



    
    

    
  

      
      
      
      

        Attached patch
          
          
        work-in-progress code v3 (obsolete)
        — 
          — Splinter Review
      

    


  

  

  



    
    

    
  
Block alignment now uses 'text-align' CSS property instead of ALIGN attribute !

  

  



    
    

    
  
> 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?

  

  



    
    

    
  
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.)

  

  



    
    

    
  
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 ;-)


  

  



    
    

    
  
List properties dialog can now generate 'list-style-type' CSS property instead
of deprecated TYPE attribute when CSS pref is checked.


  

  



    
    

    
  
added code so list items can be aligned through 'text-align' instead of
creating a div inside. See also bug 66731.

  

  



    
    

    
  
> 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**.")

  

  



    
    

    
  
> 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.

  

  



    
    

    
  
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.

  

  



    
    

    
  
I forgot that text-decoration accepts more than one value :-( Fixing now...

  

  



    
    

    
  
Bug 95062 completely blocks correct handling of U button, Underline and
Strikethrough menu items.

  

  



    
    

    
  
(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

  

  



    
    

    
  
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

  

  



    
    

    
  
Text highlight (ability to add background-color using picker to text and not
only blocks) added.

  

  



    
    

    
  
Added magic so Text Highlight button is enabled only if CSS is enabled


  

  



    
    

    
  
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!)


  

  



    
    

    
  
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...

  

  



    
    

    
  


  

  

  



    
    

    
  
New work-in-progress patch ; don't forget to add the gif file for the text
highlight button in themes/modern/editor/btn2 directory.

  

  



        Attachment #32873 -
        Attachment is obsolete: true

    
    

    
  
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

  

  


Group: netscapeconfidential?
assignee_accessible: 1 → 0
Group: netscapeconfidential?
CC list accessible: false
qacontact_accessible: 1 → 0
Not accessible to reporter

    
    

    
  
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

  

  



    
    

    
  
enabling/disabling of outdent button now cssized

  

  



    
    

    
  
*** Bug 97001 has been marked as a duplicate of this bug. ***

  

  



    
    

    
  
YAY !! Outdent button now cssized !

  

  


Depends on: 98765

    
    

    
  
YAY AGAIN : Table Properties dialog now generates CSS properties instead of HTML
attributes.... me can now resurrect the "Table height" property :-))))


  

  



    
    

    
  
*** Bug 99775 has been marked as a duplicate of this bug. ***

  

  



    
    

    
  
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.

  

  



    
    

    
  
Page Color and Background dialog now CSSized

  

  



    
  
Blocks: 97840
Target Milestone: Future → mozilla0.9.6

    
    

    
  
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 !

  

  



    
    

    
  
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 :-)

  

  



    
    

    
  
*** Bug 59993 has been marked as a duplicate of this bug. ***

  

  



    
    

    
  

      
      
      
      

        Attached file
          
        PATH V 1.0 MANIFEST (obsolete)
        — 
      

    


  

  

  



        Attachment #33869 -
        Attachment is obsolete: true

        Attachment #35381 -
        Attachment is obsolete: true

        Attachment #45061 -
        Attachment is obsolete: true

        Attachment #45226 -
        Attachment is obsolete: true

        Attachment #47784 -
        Attachment is obsolete: true

        Attachment #47785 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached file
          
        binary files (button icons) (obsolete)
        — 
      

    


  

  

  


Component: Editor: Core → Editor: Composer

    
    

    
  


  

  

  



    
    

    
  
*** Bug 59991 has been marked as a duplicate of this bug. ***

  

  



    
    

    
  
*** Bug 106480 has been marked as a duplicate of this bug. ***

  

  



    
    

    
  
bulk milestone change

  

  


Target Milestone: mozilla0.9.6 → mozilla0.9.7

    
    

    
  


  
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

  

  



    
    

    
  
I've reviewed all files in the patch starting at:
editor/composer/src/nsComposerCommands.cpp
and mailed comments to glazman.

  

  



    
    

    
  


  
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 on attachment 54060 [details] [diff] [review]
PATCH v 1.0, ready for review !!! YAY !!!! (5929 lines)

r=jfrancis

  

  



        Attachment #54060 -
        Flags: review+

    
    

    
  
> 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 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!

  

  



        Attachment #60655 -
        Attachment is obsolete: true

    
  

        Attachment #60658 -
        Attachment is obsolete: true

    
    

    
  
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

  

  



    
    

    
  


  

  

  



    
    

    
  
098

  

  


Target Milestone: mozilla0.9.7 → mozilla0.9.8

    
    

    
  
just wanting to thank Joe, Charley and especially Kin for their excellent reviews.
sorry for the spam.

  

  



    
    

    
  
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

  

  



    
    

    
  
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 :-)



  

  



    
    

    
  
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.

  

  



    
    

    
  
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 on attachment 54053 [details]
PATH V 1.0 MANIFEST

obsolete manifest

  

  



        Attachment #54053 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 54057 [details]
binary files (button icons)

obsolete icons

  

  



        Attachment #54057 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 54060 [details] [diff] [review]
PATCH v 1.0, ready for review !!! YAY !!!! (5929 lines)

obsolete patch

  

  



        Attachment #54060 -
        Attachment is obsolete: true

    
    

    
  


  
new version of themes\modern\editor\icons\btn2.gif needed for patch 2.0

  

  



    
    

    
  


  
new version of themes\classic\editor\icons\btn2.gif needed 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.

  

  



    
    

    
  

      
      
      
      

        Attached patch
          
          
        PATCH v2.0 (obsolete)
        — 
          — Splinter Review
      

    


  
patch v2.0 ; needs testing on Macintosh(with help of my favorite MacBuddy,
Peter)

  

  



    
    

    
  

      
      
      
      

        Attached patch
          
          
        PATCH v2.1
        — 
          — Splinter Review
      

    


  
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.

  

  



        Attachment #63816 -
        Attachment is obsolete: true

    
    

    
  
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.


  

  



    
    

    
  
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.

  

  



    
    

    
  
Patch successfully tested on Mac.

  

  



    
    

    
  
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.


  

  



    
    

    
  
Just for the record, Joe already gave his r= in comment #84...

  

  



    
    

    
  
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.

  

  



    
    

    
  
It's good to see a so long work end with a so short comment :

     FIXED, CHECKED-IN !!!!

doobeedoobeedoo :-)


  

  


Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED

    
    

    
  
oops.  As Daniel states, i misread the patch at NodeIsBlockStatic().  Carry on.  
Nothing to see here.  Move along people...


  

  



    
    

    
  
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/>.

  

  



    
    

    
  
I reviewed the XUL a few weeks ago, but I'll take responsibility for not telling
Daniel about using <label> instead of <text>

  

  



    
    

    
  
blaker: thanks for spotting this out; issue now covered by bug 119164

cmanske: a second beer ? ;-)


  

  



    
    

    
  
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?

  

  



    
    

    
  
Daniel, please verify this one...thanks.

  

  



    
    

    
  
verified

  

  


Status: RESOLVED → VERIFIED
Summary: [trk] add CSS support to Composer → add CSS support to Composer
Product: Browser → Seamonkey
Depends on: 427340

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: