100855 - [PATCH]Paragraph style does not update after deleting text that was aligned with a div

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect)

Description

[TucsonTester2]

Build ID: 2001091904

After deleting text that was set to heading 1 and center aligned, the drop down
for the paragraph style still showed Heading 1.  If you begin typing it will be
body text.

Reproducibility: everytime

Steps to Reproduce:
1.Open Composer
2.Click on the center align button on the toolbar
3.Choose heading 1 from the paragraph drop-down menu on the toolbar
4.Type some text
5.Click Edit -> select all
6.Delete the text
7.type some text

Actual Results:
The text will come out as body text but the Paragraph drop-down shows Heading 1
still.

Expected Results:
I would expect that the paragraph style would display properly.

Comment 1

[TucsonTester2]

Confirmed on Win 2k using Build 2001091905.

Comment 2

[Syd Logan]

EDITORBASE

Comment 3

[Charles Manske]

Debugging this, after deleting the text, the paragraph updating code is called:
nsMultiStateCommand::UpdateCommandState() for "cmd_paragraphStyle" command,
but mState appears to be "" instead of "H1", so it thinks it doesn't need to
tell the UI (menulist) to change to 'body text'.
Joe: Are you familiar with what can cause this to happen?
I'm not very expert in the command code.

Comment 4

[Joe Francis]

I've seen the display value and mState get out of sync before, but not in a 
while.  

Comment 5

[Charles Manske]

load balancing. This is probably an editor:core issue, but I'll try to help.

Comment 6

[Charles Manske]

Following the original "Steps to Reproduce", I now find that after using Center,
then selecting "Heading 1", that style is not set. Typing looks like normal text
even though the paragraph style on the toolbar says "Heading 1". If you select
any heading style after text has been typed, it will change into the proper
heading style. Then you can test the rest of the steps:
select all, delete the text, and type. You will see 'body text' style but the 
paragraph style in the toolbar menulist still shows the heading style.
Handing off to Joe for at least the first problem.

Comment 7

[Joe Francis]

fixed

Comment 8

[michael wendell]

I am still seeing the original problem on Win 2k using build 2001112803.

I am reopening this bug.

Comment 9

[Joe Francis]

098

Comment 10

[TucsonTester1]

Original issue described is still occuring on Mac OS X using build 2001121004.
regarding Comment 6, I am not seeing that issue now. Only the issue reported in
the begining. 

Comment 11

[Joe Francis]

pushing off 098 to 099

Comment 12

[Syd Logan]

plussing

Comment 13

[TucsonTester1]

I can still comfirm the original problem.  This also happens with the right
alignment button and all of the heading options in the paragraph format box. 
After step 4, if you hit enter (once or several times), the text turns back to
paragraph style but still the paragraph drop-down box displays the previous heading.

Comment 14

[Joe Francis]

Assuming that editor isn't lying to the ui code about the para format, the way
to fix this bug and others like it once and for all is for the ui to never cache
what it *thinks* the value of the popup is.  Instead, it should always fetch the
value from whatever drives the disaplyed value.  I think the ui code is getting
out of sync with it's own popup.

Comment 15

[Joe Francis]

marking helpwanted and spamming ed team

Comment 16

[Syd Logan]

Over to Charley

Comment 17

[Charles Manske]

I have thoroughly debugged this. The problem is not in any state cache problem.
The root of the problem is in nsHTMLEditRules::GetParagraphState() which calls
nsHTMLEditRules::GetParagraphFormatNodes() to get the current paragraph node.
After these steps:
2.Click on the center align button on the toolbar
3.Choose heading 1 from the paragraph drop-down menu on the toolbar
The current html source is:
<h1><div align="center">[caret location]</div></h1>
In this state, the DIV blocks GetParagraphFormatNodes from finding the H1, and it
returns "" instead. 
After this:
5.Click Edit -> select all
6.Delete the text
The true paragraph state is "body text" (reported as "").
Since that is the state the UI thought it was in before, it doesn't change
the paragraph menulist state.
Similar problems occur if you do <blockquote> before applying other paragraph
styles -- I bet Joe already has some bugs on that problem!
I noticed some special testing in GetNodesForOperation():
  // outdent should look inside of divs.
  if (inOperationType == kOutdent && !useCSS) 
that does look inside of <div>s 
In the case of setting paragraph style, inOperationType = kMakeBasicBlock,
so I tested including that case as well. This seems to work perfectly, but I 
know how subtle the rules code is, so this would definitely need testing for
unforseen side effects. (Then again, if we're lucky, maybe it would actually 
cure some other bugs!)
Joe will evaluate.

Comment 18

[Charles Manske]

Attachment: Suggested fix

This is in nsHTMLEditRules::GetNodesForOperation()

Comment 19

[Joe Francis]

Charlie I'm confused.  You state that "" is returned as teh state after the 
deletion.  Isn't "" suposed to be interpreted by the ui as Body Text?  In the 
original bug report, it says we get Heading instead (in the ui).  

That would be a UI bug wouldn't it?  I'm not trying to deflect the issue away 
from any bugs in the edit rules (I think you are right that divs and blockquotes 
confuse the state reporting by the edit rules).  It just sounds to me like we  
have a seperate ui bug that also needs fixing.  

Am I right or just misunderestanding this?

Comment 20

[Joe Francis]

Here's what I think should happen.  I need to tweak GetParagraphState().  When
it gets the list of nodes bach from GetParagraphFormatNodes() it should examine
them  and their parents until it finds one of the types listed in the paragraph
format menu (note this will always succeed, since we will always find the body
eventually if nothing else).  Blockquote should be removed as one of the types.

This way even if GetParagraphFormatNodes() returns a div or a blockquote, we
will still look "up" until we find the real paragraph format.

I'm afraid to make the change Charlie suggests because GetParagraphFormatNodes()
is also used by the routines that actually do block operations.  Thus, making
headers etc would work differently if we applied Charlie's patch.

I also worry that there is a real ui bug here (see my earlier comment) that
needs to be investigated.  

I can make the changes I propose above without too much trouble.  About 1/2 day
of work counting testing and review process.  I'm hoping Charlie can look into
the issues I raise about a possible ui bug.

Comment 21

[Charles Manske]

To comment #19:
Yes, returning "" *after* the deletion is correct; the bug is that "" is 
reported by nsHTMLEditRules::GetParagraphState() after changing the centerred 
text to H1 because the <div> hides detecting it.
To comment #20:
There are no UI bugs -- I did investigate thoroughly. It all boils down to 
reporting the real paragraph style from nsHTMLEditRules::GetParagraphState().
You suggestion of looking up the parent chain for paragraph styles we know about
is excellent. I figured that the change I suggested would be risky (though why 
do we do similar special case testing when inOperationType = kOutdent?)
Handing back to Joe to fix nsHTMLEditRules::GetParagraphState().

Comment 22

[Joe Francis]

Charlie, I disagree.  There is a ui bug too.  The rules are reporting body text 
(incorrectly at first, correctly after the delete).  The ui says heading.  The ui 
is not listening to the editor.

There are two bugs here.

Comment 23

[Charles Manske]

I can't explain it any clearer. The UI is reporting "Heading 1" after the 
deletion ONLY because the nsHTMLEditRules::GetParagraphState() did not report
the paragraph style correctly when it was set. So yes, it does cache that setting
and is fooled into thinking it hasn't changed after the deletion, but you can't
blame the UI code for that.

Comment 24

[Joe Francis]

I can and I do blame the ui code for that.  If the editor code is unable for
some reason to execute a format operation, we need to not have the ui lie about
it.  It should always report what the editor tells it.

Comment 25

[Joe Francis]

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

Comment 26

[Joe Francis]

Assigning to me for the GetParagraphState() work.  This will, unfortunately,
hide the ui bug, which I would love to see fixed because it can cause the ui to
get out of sync with reality, and potentially stay out of sync for the rest of
the editing session.

Comment 27

[basic]

jfrancis: would you mind filing another bug report for the UI bug? And please
mention the bug # here too

Comment 28

[Joe Francis]

Attachment: patcc for editor

patches editor/libeditor/html/nsHTMLEditRules.{h,cpp} and also
editor/ui/composer/content/editorOverlay.xul

Fixes this bug and also bugs 42738 and 79578.

Comment 29

[Charles Manske]

Joe: I just can't apply the patch to nsHTMLEditRules.cpp. It keeps giving me
"malformed patch" error messages. I've corrected all the LF/CR format problems.
I've applied the rest manually.
I'd love to try this soon!
BTW, I totally support removing Blockquote from the menulist.

Comment 30

[Joe Francis]

Attachment: cleaned up patch to editor

This one should apply.	Time for reviews

Comment 31

[Kathleen :Brade]

* in nsHTMLEditRules.cpp, fix this typo:  heirarchy  --> hierarchy

* should initialize listCount in this block if there is any change Count would
fail to initialize it for you (error of some sorts?):
+  PRUint32 listCount;
+  arrayOfNodes->Count(&listCount);

and then shouldn't we reset (initialize) it here:
-  PRUint32 listCount;

* you need to fix this return value (don't you see a warning for it?) ;-)
+PRBool 
+nsHTMLEditRules::IsFormatNode(nsIDOMNode *aNode)
+{
+  if (!aNode) return NS_ERROR_NULL_POINTER;

* I'd like to see initialization of of "len" unless you are positive the
GetLength call won't fail and will do that for us:
+  PRUint32 len, j=0;
+  childList->GetLength(&len);

* This line doesn't check its return value (and it probably should?):
+      AppendInnerFormatNodes(aArray, child);

Comment 32

[kinmoz]

Joe and I discussed some possible performance improvements for this fix. I think
he's going to be attatching a new patch.

Comment 33

[Joe Francis]

(improved) fix landed on trunk

Comment 34

[Kevin McCluskey (gone)]

Resolving as FIXED. Please add fixed1.0.0 keyword after the fix has been checked
into the 1.0.0 branch.

Comment 35

[michael wendell]

Verified on Mac OSX and Win 2k using the 04-15 trunk builds.

Comment 36

[Joe Francis]

need adt and driver approval for branch landing.  I'm really hoping we can land
this one, as it fixes a longstanding bug in the editor that caused incorrect ui
feedback.  And given the implementation of the ui, the incorrect state it
reports can prevent you from setting the correct state.  

On top of that, we have also removed an item from the paragraph format popup
which should have never been there ("blockquote").

Fix should only affect what the ui reports as the paragraph format.  Fix does
NOT affect actual editor actions.  It has baked on the trunk for a week now with
no problems.

Comment 37

[Jaime Rodriguez, Jr.]

adt1.0.0+ (on ADT's behalf) for checkin to the 1.0 branch. Pls check this in to
the 1.0 branch today, then add the fixed1.0.0 keyword.

Comment 38

[Joe Francis]

Attachment: diffs to nsHTMLEditRles.cpp

what landed on trunk in nsHTMLEditRules.cpp

Comment 39

[Joe Francis]

Attachment: diffs to nsHTMLEditRules.h

what landed on trunk in nsHTMLEditRules.h

Comment 40

[Joe Francis]

Attachment: diffs to editorOverlay.xul

what landed on trunk to editorOverlay.xul

Comment 41

[Kathleen :Brade]

Comment on attachment 80695 [details] [diff] [review]
diffs to editorOverlay.xul

r=brade

Comment 42

[Kathleen :Brade]

Comment on attachment 80694 [details] [diff] [review]
diffs to nsHTMLEditRules.h

r=brade

Comment 43

[Joe Francis]

Attachment: diffs for nsHTMLEditRules.cpp

some tweaks in response to comments from brade.

Comment 44

[Kathleen :Brade]

Comment on attachment 80736 [details] [diff] [review]
diffs for nsHTMLEditRules.cpp

r=brade

Comment 45

[kinmoz]

Comment on attachment 80736 [details] [diff] [review]
diffs for nsHTMLEditRules.cpp

I sent these comments to joe in email, just adding it here for completeness.

In AppendInnerFormatNodes() I think there's one more optimization we can do,
though I don't want to make it a condition for checkin.

I think the |firstInline| info needs to be passed down into recursive calls so
that it could be used to suppress further appending of |firstInline| blocks
should one be found any where during the recursion. Recursive calls should also
be able to modify this flag so that it can pass back info to the caller, to
prevent |firstInline| blocks. The cases I'm thinking about look something like
this, though I'm not sure if you agree it's a common occurrence:

    Case 1:  <div>inline1<div>inline2<p>p1</p></div><p>p2</p></div>

    Case 2:  <div><div><p>p1</p>inline1</div><p>p2</p>inline2</div>

In Case 1, there shouldn't be any need to append both "inline1" and "inline2"
to the array since they will both lead to the same exact answer. So when we
recurse, looking for |firstInline| should be disabled.

In Case 2, we find "inline1" during a recursive call, so when we unwind, we
shouldn't need to append "inline2".

I realize most of this was code that was just moved to utility methods ... In
GetFormatString():


+  else if (nsHTMLEditUtils::IsHeader(aNode))
+  {
+    nsAutoString tag;
+    nsEditor::GetTagString(aNode,tag);
+    ToLowerCase(tag);
+    format = tag;
+  }



We can avoid an unnecessary copy by just using |format| directly right?


+  else
+  {
+    format.Truncate(0);
+  }


If we get to this |else|, |format| will not have been touched, so I don't think
there's a need for a Truncate() call?

Comment 46

[kinmoz]

Comment on attachment 80695 [details] [diff] [review]
diffs to editorOverlay.xul

sr=[e-mail]

Comment 47

[kinmoz]

Comment on attachment 80694 [details] [diff] [review]
diffs to nsHTMLEditRules.h

sr=[e-mail]

Comment 48

[Randell Jesup [:jesup] (needinfo me)]

It's unclear to drivers if this is ready for re-assessment for approval.  If so,
please email drivers

Comment 49

[Joe Francis]

Attachment: diffs to nsHTMLEditRules.cpp

Ok here's the deal.  This diff contains changes made in response to kin's sr. 
I removed two unneeded local variables from GetFormatString().	This addressed
kin's final two comments by removing the unneeded format var, and by making the
truncate() call needed after all (thus mooting that comment).

The other thing Kin and I talked about was a performance enhancement.  However,
real world documents were unlikely to benefit significantly from the
enhancement, so we agreed to postpone that tweak in order to minimize changes
to the patch at this point.

So, in summary, the patches as they now stand have reviews, and have only
changed from the trunk landing in trivial ways.  Seeking approval from drivers.

Comment 50

[Asa Dotzler [:asa]]

Comment on attachment 80999 [details] [diff] [review]
diffs to nsHTMLEditRules.cpp

a=asa (on behalf of drivers) for checkin to the 1.0 branch

Comment 51

[Joe Francis]

FIXED ON BRANCH

Comment 52

[michael wendell]

Verified on the 04-30 branch, and adding verified1.0.0 keyword.

I am still seeing a minor issue with this, that has been filed in bug 141272.