179055 - inline styles not always coalesced in CSS mode

Status: VERIFIED FIXED

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

Description

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

1. launch composer in CSS mode
2. type aaabbbccc
3. bolden bbc
4. bolden aab

Expected result :
  a<span style="font-weight: bold;">aabbbc</span>cc

Actual result :
  a<span style="font-weight: bold;">aab</span><span style="font-weight:
bold;">bbc</span>cc

Comment 1

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

Attachment: patch #1, Kathy's dream

Comment 2

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

Comment on attachment 105578 [details] [diff] [review]
patch #1, Kathy's dream

Joe, can you review this bug please ?

Comment 3

[Kevin McCluskey (gone)]

nsbeta1+

Comment 4

[Joe Francis]

This is a good start but there is one major issue: the merging code in the css
case is driven entirely off of |IsSpanEquivalentToHTMLInlineStyle()|.  This
works only if the spans that we are checking have only one property.  

Example that illustrates the problem:
type "aaabbb"
select "aaa"
bold
italic
select "bbb"
bold
italic

Now look at the source.  No merge, even though spans have exact same properties.

In HTML only mode, we do a get a merge on the outer <i> elements, but not on the
inner <b> elements.  If you can get this working, the css-enabled editor has a
chance to do this better than the html editor.

Other changes I would like to see:
Use the existing curly brace style for the nsHTMLEditorStyle.cpp part of the
patch.  I hate it when curly brace style changes from place to place in the same
file.

Also, I would suggest changing the code there slightly to just set |mergeNodes|
boolean inside the if clauses, and not call |MoveNode()|.  Then outside both if
clauses have an: |if (mergeNodes) MoveNodes(...)|  That way we don't have two
identical MoveNode calls to keep in sync durng future changes to the source.

Hope this helps.  Do you want to tackle the big issue in this patch, or wait on
that until later?

Comment 5

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

Attachment: patch #2, mucho mucho simpler

Comment 6

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

Comment on attachment 106903 [details] [diff] [review]
patch #2, mucho mucho simpler

Joe, can you re-review please ?

Comment 7

[Joe Francis]

I dont understand this patch.

i applied it, and tried the following:
type "aaabbb"
select "aaa"
make bold
select "bbb"
make bold

I get two identical spans, unmerged.  Looking at the code I think they would
merge, but they don't.

Then, from examining the code, I see that you look for adjacent spans, but don't
check to see if they have matching attributes.  I'm concerned that if this code
did cause a merge, it might merge spans that had different styles.

Comment 8

[Joe Francis]

I tried this patch again and it works.  Meanwhile, the part of this patch that
is comparing style attributes on the spans is hidden inside NodesSameType(),
which eluded me because that function in overloaded.  The nsEditor version does
not check styles.  The nsHTMLEditor version checks css style, but only on spans.
 I find that very confusing.

Comment 9

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

Comment on attachment 106903 [details] [diff] [review]
patch #2, mucho mucho simpler

Peter, can you sr please ?

Comment 10

[Peter Van der Beken [:peterv]]

Comment on attachment 106903 [details] [diff] [review]
patch #2, mucho mucho simpler

>+      if (nextSibling || previousSibling)
>+      {
>+        nsCOMPtr<nsIDOMNode> mergeParent;
>+        res = tmp->GetParentNode(getter_AddRefs(mergeParent));
>+        if (NS_FAILED(res)) return res;

Move the return on its own line please.

>+        if (previousSibling &&
>+            nsTextEditUtils::NodeIsType(previousSibling, NS_LITERAL_STRING("span")) &&
>+            NodesSameType(tmp, previousSibling))
>+        {
>+          res = JoinNodes(previousSibling, tmp, mergeParent);
>+          if (NS_FAILED(res)) return res;

Same here.

Comment 11

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

checked in (trunk)

Comment 12

[Kathleen :Brade]

RogerMW--I'm not sure you need this patch since it's when people are editing
css/inline styles.  If you want to try it out, focus on the last two blocks of
the patch.

Daniel--is this the other bug I just asked about in a different bug?

Comment 13

[sairuh (rarely reading bugmail)]

vrfy'd fixed using 2003.02.19 (linux rh8.0). tested comment 0 and comment 4, as
well as an "overlapping" test for sanity below, and the inline styles are merged
nicely as expected.

1. enter "aaabbb"
2. select and bold "aab"
3. select and bold "abb"
4. check source --should have only one inline style spanning "aabb".