Closed
Bug 100855
Opened 23 years ago
Closed 22 years ago
[PATCH]Paragraph style does not update after deleting text that was aligned with a div
Categories
(Core :: DOM: Editor, defect)
Core
DOM: Editor
Tracking
()
VERIFIED
FIXED
mozilla1.0
People
(Reporter: TucsonTester2, Assigned: mozeditor)
References
Details
(Whiteboard: EDITORBASE+; fixed on trunk; [need a=] [adt2])
Attachments
(3 files, 5 obsolete files)
2.69 KB,
patch
|
Brade
:
review+
kinmoz
:
superreview+
|
Details | Diff | Splinter Review |
2.87 KB,
patch
|
Brade
:
review+
kinmoz
:
superreview+
|
Details | Diff | Splinter Review |
7.62 KB,
patch
|
asa
:
approval+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•23 years ago
|
||
Confirmed on Win 2k using Build 2001091905.
EDITORBASE
Assignee: syd → cmanske
Whiteboard: EDITORBASE
Target Milestone: --- → mozilla0.9.6
Comment 3•23 years ago
|
||
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee | ||
Comment 4•23 years ago
|
||
I've seen the display value and mState get out of sync before, but not in a while.
Comment 5•23 years ago
|
||
load balancing. This is probably an editor:core issue, but I'll try to help.
Status: NEW → ASSIGNED
Target Milestone: mozilla0.9.6 → mozilla0.9.7
Comment 6•23 years ago
|
||
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.
Assignee: cmanske → jfrancis
Status: ASSIGNED → NEW
Component: Editor: Composer → Editor: Core
Assignee | ||
Comment 7•23 years ago
|
||
fixed
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Comment 8•23 years ago
|
||
I am still seeing the original problem on Win 2k using build 2001112803. I am reopening this bug.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 10•23 years ago
|
||
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.
Assignee | ||
Comment 11•23 years ago
|
||
pushing off 098 to 099
Target Milestone: mozilla0.9.8 → mozilla0.9.9
Comment 13•23 years ago
|
||
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.
Assignee | ||
Comment 14•23 years ago
|
||
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.
Assignee | ||
Comment 15•23 years ago
|
||
marking helpwanted and spamming ed team
Whiteboard: EDITORBASE+ → EDITORBASE+; helpwanted
Comment 16•23 years ago
|
||
Over to Charley
Assignee: jfrancis → cmanske
Status: REOPENED → NEW
Component: Editor: Core → Editor: Composer
Target Milestone: mozilla0.9.9 → mozilla1.0
Comment 17•23 years ago
|
||
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.
Status: NEW → ASSIGNED
Comment 18•23 years ago
|
||
This is in nsHTMLEditRules::GetNodesForOperation()
Assignee | ||
Comment 19•23 years ago
|
||
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?
Assignee | ||
Comment 20•23 years ago
|
||
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•23 years ago
|
||
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().
Assignee: cmanske → jfrancis
Status: ASSIGNED → NEW
Assignee | ||
Comment 22•23 years ago
|
||
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•23 years ago
|
||
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.
Assignee | ||
Comment 24•23 years ago
|
||
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.
Assignee | ||
Comment 25•23 years ago
|
||
*** Bug 112704 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 26•23 years ago
|
||
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•23 years ago
|
||
jfrancis: would you mind filing another bug report for the UI bug? And please mention the bug # here too
Assignee | ||
Comment 28•23 years ago
|
||
patches editor/libeditor/html/nsHTMLEditRules.{h,cpp} and also editor/ui/composer/content/editorOverlay.xul Fixes this bug and also bugs 42738 and 79578.
Assignee | ||
Updated•23 years ago
|
Status: NEW → ASSIGNED
Whiteboard: EDITORBASE+; helpwanted → EDITORBASE+; fixinhand; need r=,sr=,a=
Comment 29•23 years ago
|
||
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.
Assignee | ||
Comment 30•22 years ago
|
||
This one should apply. Time for reviews
Attachment #72234 -
Attachment is obsolete: true
Attachment #72567 -
Attachment is obsolete: true
Comment 31•22 years ago
|
||
* 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);
Whiteboard: EDITORBASE+; fixinhand; need r=,sr=,a= → EDITORBASE+; fixinhand; need r=,sr=,a= [adt2]
Updated•22 years ago
|
Summary: Paragraph style does not update after deleting text that was aligned with a div → [PATCH]Paragraph style does not update after deleting text that was aligned with a div
Comment 32•22 years ago
|
||
Joe and I discussed some possible performance improvements for this fix. I think he's going to be attatching a new patch.
Assignee | ||
Comment 33•22 years ago
|
||
(improved) fix landed on trunk
Whiteboard: EDITORBASE+; fixinhand; need r=,sr=,a= [adt2] → EDITORBASE+; fixed on trunk; need r=,sr=,a= [adt2]
Comment 34•22 years ago
|
||
Resolving as FIXED. Please add fixed1.0.0 keyword after the fix has been checked into the 1.0.0 branch.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago → 22 years ago
Resolution: --- → FIXED
Comment 35•22 years ago
|
||
Verified on Mac OSX and Win 2k using the 04-15 trunk builds.
Status: RESOLVED → VERIFIED
Assignee | ||
Comment 36•22 years ago
|
||
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•22 years ago
|
||
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.
Assignee | ||
Comment 38•22 years ago
|
||
what landed on trunk in nsHTMLEditRules.cpp
Attachment #74658 -
Attachment is obsolete: true
Assignee | ||
Comment 39•22 years ago
|
||
what landed on trunk in nsHTMLEditRules.h
Assignee | ||
Comment 40•22 years ago
|
||
what landed on trunk to editorOverlay.xul
Comment 41•22 years ago
|
||
Comment on attachment 80695 [details] [diff] [review] diffs to editorOverlay.xul r=brade
Attachment #80695 -
Flags: review+
Comment 42•22 years ago
|
||
Comment on attachment 80694 [details] [diff] [review] diffs to nsHTMLEditRules.h r=brade
Attachment #80694 -
Flags: review+
Assignee | ||
Comment 43•22 years ago
|
||
some tweaks in response to comments from brade.
Assignee | ||
Updated•22 years ago
|
Attachment #80693 -
Attachment is obsolete: true
Comment 44•22 years ago
|
||
Comment on attachment 80736 [details] [diff] [review] diffs for nsHTMLEditRules.cpp r=brade
Attachment #80736 -
Flags: review+
Comment 45•22 years ago
|
||
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?
Attachment #80736 -
Flags: superreview+
Comment 46•22 years ago
|
||
Comment on attachment 80695 [details] [diff] [review] diffs to editorOverlay.xul sr=kin@netscape.com
Attachment #80695 -
Flags: superreview+
Comment 47•22 years ago
|
||
Comment on attachment 80694 [details] [diff] [review] diffs to nsHTMLEditRules.h sr=kin@netscape.com
Attachment #80694 -
Flags: superreview+
Comment 48•22 years ago
|
||
It's unclear to drivers if this is ready for re-assessment for approval. If so, please email drivers
Assignee | ||
Comment 49•22 years ago
|
||
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.
Attachment #80736 -
Attachment is obsolete: true
Comment 50•22 years ago
|
||
Comment on attachment 80999 [details] [diff] [review] diffs to nsHTMLEditRules.cpp a=asa (on behalf of drivers) for checkin to the 1.0 branch
Attachment #80999 -
Flags: approval+
Comment 52•22 years ago
|
||
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.
Keywords: verified1.0.0
You need to log in
before you can comment on or make changes to this bug.
Description
•