Closed Bug 140357 Opened 23 years ago Closed 21 years ago

Backspace deletes text formatting,TypeInState should be set during the deletion of the last item in an inline style

Categories

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

Product:
Core
Component:
DOM: Editor
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.4final

People

(Reporter: kinmoz, Assigned: mozeditor)

References

Details

(Keywords: topembed+, Whiteboard: EDITORBASE+ edt_x3, fixinhand [a=sspitzer,asa for 1.4 final, but wait until after rc2])

Attachments

(3 files, 1 obsolete file)

patch to editor
(deleted), patch
Brade
: review+
kinmoz
: superreview+
asa
: approval1.4+
Details | Diff | Splinter Review
improvements
(deleted), patch
Brade
: review+
kinmoz
: superreview+
asa
: approval1.4+
Details | Diff | Splinter Review
patch to nsHTMLEditRules.cpp
(deleted), patch
Brade
: review+
kinmoz
: superreview+
sspitzer
: approval1.4+
Details | Diff | Splinter Review
While playing around and comparing our Editor in Netscape InstantMessenger (NIM) with the one in AIM, for bug 125345, I was noticing some differences in editor behavior related to inline styles. In particular, it seems that AIM, as well as other editors (MS Word, Wordpad), all seem to leave an inline style button active when deleting the last char that has that style. For example if I had: <body>1<b>2</b>3</body> If I placed my caret after the '2', or selected the '2', and hit the backspace key, the bold button on the toolbar of those apps would still be pressed, such that if I typed 'a', it would be bold. Doing this experiment in Composer/IM/MailCompose using our editor, the button would end up not pressed after the deletion, so if I typed an 'a' it would show up as a normal non-bold 'a'. This doesn't sound like such a big deal, but in NIM it makes trying to use "default text properties" a big chore. In both NIM and AIM, the user is allowed to set prefs up that say I want my text to look a certain way by default ... included in these prefs is the choice to have your text styled with color, bold, italics, and underline. Currently the only way for NIM to setup any of these styles as defaults, is to call SetTextProperty() when they first bring up the window. This works fine until the user types a character and hits backspace. Once they do that all the "default styles" are lost because TypeInState was cleared when they typed the character, and backspace leaves nothing in the document, so the UI resets all the style buttons to be un-pressed since there are no inline styles above the caret. I'm thinking that the only way for us to act more like the other editors, is if at the time we delete the last character and the empty inline style container, we also set the TypeInState and notify the UI so that it can update accordingly. It looks like when deleting a mixed selection, the styles at the beginning of the selection are what stick. Comments?
This is a bit tricky. frst off, note that if you click, even in the same place, typeinstate is lost. Example: bring up new composer, make bold, type a letter (it's bold). bring up new composer, make bold, click in window, type a letter (it's *not* bold). I'm guessing you'd want to fix that too if we do this. Then there is the issue of timing. Setting up type in state for the net editor action from inside the current editor action cannot work as things stand now. This is because the selection listeners dont fire until later (the editor turned tem off during the edit action) which then clears out our type in state before the next action ever gets a chance. But this is all doable, of course. I just need to make the type in state code a bit more resilient. It should not clear itself out unless selection has changed from a specific selection that is stored with the type in state data. If I make that changes, then I could 1) cache the inline styles at the start of a delete operation 2) set up typing state after delete operation done 3) notice that the selection listener is telling me that selection is where type in state wants it, and not clear it out. Do we want this for 1.0?
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.1alpha
I posted a patch to bug 125345 that works around the problem joe mentioned above where clicking in the same place causes the type in state to reset. Selection is firing the selection changed notification when it really hasn't, and it calls it more then once for a single change (bug 140303).
I would consider this to be a key indicator of correctness, or more appropriately, EDITORBASE. We shouldn't be clearing the style after backspacing past the last character. Nominating. I'm bringing this up now due to kin's comment in bug 125345 that describes this bug being a spinoff. I consider this a part of the overall user experience when using the styles feature in IM, and didn't realize it wasn't getting covered until I *really* read comment 12 (http://bugzilla.mozilla.org/show_bug.cgi?id=125345#c12) in that bug. Thanks, -Kevin
Whiteboard: EDITORBASE
EDITORBASE+
Priority: -- → P1
Whiteboard: EDITORBASE → EDITORBASE+
Target Milestone: mozilla1.1alpha → mozilla1.1beta
Keywords: nsbeta1+
Whiteboard: EDITORBASE+ → EDITORBASE+ [adt2]
nsbeta1-. The fix is too risky for the 1.0 branch.
Keywords: nsbeta1+ → nsbeta1-
The days of having a half dozen milestones out in front of us to divide bugs between seem to be gone, though I dont know why. Lumping everything together as far out as I can. I'll pull back things that I am working on as I go.
Target Milestone: mozilla1.1beta → mozilla1.2beta
Target Milestone: mozilla1.2beta → M1
*** Bug 156722 has been marked as a duplicate of this bug. ***
changing summary
Summary: TypeInState should be set during the deletion of the last item in an inline style → Backspace deletes text formatting,TypeInState should be set during the deletion of the last item in an inline style
My favorite bug. I can't believe this isn't topembed at the very least.
Keywords: nsbeta1- → mozilla1.2, nsbeta1
Target Milestone: M1 → mozilla1.2alpha
for personal sanity, i'm marking bugs that i am looking at asap with "m1". the corallary is that moving the bug to some other milestone causes me to forget about it.
Target Milestone: mozilla1.2alpha → M1
nsbeta1+
Keywords: nsbeta1 → nsbeta1+
is this bug planned to fix in the near future? I see 'mozilla1.2' in keywords, but it is passed that milestone. thx!
QA Contact: sujay → beppe
Blocks: PhtN5
Whiteboard: EDITORBASE+ [adt2] → EDITORBASE+ [adt2] edt_x3
Keywords: topembed+
I need to port the fix from a different branch.
Whiteboard: EDITORBASE+ [adt2] edt_x3 → EDITORBASE+ [adt2] edt_x3, fixinhand
reproduced problem with 2003041609 build on winXP
Target Milestone: M1 → mozilla1.4final
Whiteboard: EDITORBASE+ [adt2] edt_x3, fixinhand → EDITORBASE+ edt_x3, fixinhand
Attached patch patch to editor (deleted) — — Splinter Review
port of original ANGELON patch. Some further de-hackifying would be nice, but it should work as is.
IMHO this bug should be fixed in 1.4final builds. Nominating for 1.4final.
Flags: blocking1.4?
Comment on attachment 124463 [details] [diff] [review] patch to editor NS folks are asking me to land this. This was previously reviewed and landed on the ANGELON branch and has been in service there with good results for some time.
Attachment #124463 - Flags: approval1.4?
I've tested my tip build of this patch and it is working well. Good to go here.
Attachment #124463 - Flags: review+
Comment on attachment 124463 [details] [diff] [review] patch to editor sr=kin@netscape.com ==== I just noticed this ... since RemoveAllDefaultProperties() is public, shouldn't it also be calling |mDefaultStyles.RemoveElementAt(j)| or somehow clearing the entire array when it's done? +NS_IMETHODIMP nsHTMLEditor::RemoveAllDefaultProperties() +{ + PRInt32 j, defcon = mDefaultStyles.Count(); + for (j=0; j<defcon; j++) + { + PropItem *item = (PropItem*)mDefaultStyles[j]; + if (item) delete item; + } + return NS_OK; +}
Attachment #124463 - Flags: superreview+
Answering Kin's question: yes it should. It is not actually used in this bug fix. This fix includes the groundwork for 195812 as well, since I landed those two simultaneously on the ANGELON branch, and since the underlying support for both issues is related. But I'll fix that anyway prior to landing.
a=adt to land this on the 1.4 branch.
Blocks: 206305
this is a good data point (for approval) "This was previously reviewed and landed on the ANGELON branch and has been in service there with good results for some time. but let's wait for another driver to second this. jfrancis, is the fix already on the trunk?
Whiteboard: EDITORBASE+ edt_x3, fixinhand → EDITORBASE+ edt_x3, fixinhand [seeking approval]
jfrancis, does this bug bite us in HTML mozilla mail compose, or just NIM / ANGELON?
It shows up in composer, html mail compose, and in AIM. There is an evil hack on the AIM side to work around it in AIM, I think. The fix is not on the trunk yet. I will land it on trunk today if possible.
There is NO evil hack in AIM to workaround this problem. This issue has been around in AIM for years now. See bugscape 18910 for details.
Suresh (comment 25)--I believe Joe is thinking of a different "hack" that IM does to remember what the last selected font/size/style/etc. are.
Joe--if you don't check this in today, can you post an updated patch in the bug?
Suresh, there is a hack in nim, which is what i meant by aim. nim resets styles on every sned precisely becasue after they empty out the im compose pane, their style settings are lost by the editor.
"nim resets styles on every send...". ok, I know that hack.
fix landed on tip. leaving bug open for 1.4 approval and landing.
Now that this is in on the trunk, can we get a QA verification. This is a fairly large change and drivers are hesitant to take this on the branch without some confidence that it's not regressing anything on the trunk. Can we get some verification from petersen and/or sairuh? Maybe also get nboca and esther to take a look for mail.
If I backspace multiple times in an empty editor, I still see this problem. See http://bugscape.mcom.com/show_bug.cgi?id=18910#c22 for more details.
check-in increased linux code size by 7k+, windows by 3k+ Linux: ================================================================= Overall Change in Size Total: +7264 (+12770/-5506) Code: +7168 (+12220/-5052) Data: +96 (+550/-454) libeditor.so Total: +7264 (+12770/-5506) Code: +7168 (+12220/-5052) Data: +96 (+550/-454) +7168 (+12220/-5052) T (CODE) +7168 (+12220/-5052) UNDEF:libeditor.so:T +3580 nsHTMLEditRules::nsHTMLEditRules(void) +3552 nsHTMLEditor::GetInlinePropertyBase(nsIAtom *, nsAString const *, nsAString const *, int *, int *, int *, nsAString *, int) +1196 nsHTMLEditor::IsTextPropertySetByContent(nsIDOMNode *, nsIAtom *, nsAString const *, nsAString const *, int &, nsIDOMNode **, nsAString *) +576 nsHTMLEditRules::CreateStyleForInsertText(nsISelection *, nsIDOMDocument *) +508 nsHTMLEditRules::BeforeEdit(int, short) +332 nsHTMLEditor::AddDefaultProperty(nsIAtom *, nsAString const &, nsAString const &) +328 nsHTMLEditRules::CacheInlineStyles(nsIDOMNode *) +312 nsHTMLEditRules::ReapplyCachedStyles(void) +308 TypeInState::FindPropInList(nsIAtom *, nsAString const &, nsAString *, nsVoidArray &, int &) +256 nsHTMLEditor::RemoveAllInlineProperties(void) +244 nsHTMLEditRules::AfterEditInner(int, short) +240 nsHTMLEditor::RemoveDefaultProperty(nsIAtom *, nsAString const &, nsAString const &) +204 nsHTMLEditor::ApplyDefaultProperties(void) +192 nsHTMLEditor::RemoveAllDefaultProperties(void) +84 nsHTMLEditRules::ClearCachedStyles(void) +72 PropItem::PropItem(nsIAtom *, nsAString const &, nsAString const &) +44 nsHTMLEditRules::~nsHTMLEditRules(void) +32 nsHTMLEditor::~nsHTMLEditor(void) +32 virtual function thunk (delta:-228) for nsHTMLEditor::AddDefaultProperty(nsIAtom *, nsAString const &, nsAString const &) +32 virtual function thunk (delta:-228) for nsHTMLEditor::RemoveAllDefaultProperties(void) +32 virtual function thunk (delta:-228) for nsHTMLEditor::RemoveDefaultProperty(nsIAtom *, nsAString const &, nsAString const &) +16 nsHTMLEditor::nsHTMLEditor(void) +8 TypeInState::NotifySelectionChanged(nsIDOMDocument *, nsISelection *, short) +8 nsHTMLEditor::GetFontFaceState(int *, nsAString &) +8 nsHTMLEditor::GetInlinePropertyWithAttrValue(nsIAtom *, nsAString const &, nsAString const &, int *, int *, int *, nsAString &) +4 NS_NewHTMLEditRules(nsIEditRules **) +4 TypeInState::TypeInState(void) +4 nsHTMLEditRules::WillDeleteSelection(nsISelection *, short, int *, int *) +4 nsHTMLEditor::GetFontColorState(int *, nsAString &) +4 nsHTMLEditor::GetHighlightColor(int *, unsigned short **) +4 nsHTMLEditor::GetInlineProperty(nsIAtom *, nsAString const &, nsAString const &, int *, int *, int *) -4 TypeInState::RemovePropFromSetList(nsIAtom *, nsString const &) -4 TypeInState::IsPropCleared(nsIAtom *, nsString const &, int &) -8 TransactionFactory::GetNewTransaction(nsID const &, EditTxn **) -72 PropItem::PropItem(nsIAtom *, nsString const &, nsString const &) -308 TypeInState::FindPropInList(nsIAtom *, nsString const &, nsString *, nsVoidArray &, int &) -1196 nsHTMLEditor::IsTextPropertySetByContent(nsIDOMNode *, nsIAtom *, nsAString const *, nsAString const *, int &, nsIDOMNode **, nsAString *) const -3460 nsHTMLEditor::GetInlinePropertyBase(nsIAtom *, nsAString const *, nsAString const *, int *, int *, int *, nsAString *) +64 (+132/-68) D (DATA) +64 (+132/-68) UNDEF:libeditor.so:D +32 nsHTMLEditor::nsIHTMLEditor virtual table +32 nsIHTMLEditor virtual table +11 dd.5424 +8 address.5420 +8 captionTag.5409 +5 bodyTag.5406 +4 pre.5421 +3 dt.5423 +3 h1.5414 +3 h2.5415 +3 h3.5416 +3 h4.5417 +3 h5.5418 +3 h6.5419 +3 li.5422 +3 tdTag.5407 +3 thTag.5408 +2 p.5413 -2 p.5401 -3 thTag.5396 -3 tdTag.5395 -3 li.5410 -3 h6.5407 -3 h5.5406 -3 h4.5405 -3 h3.5404 -3 h2.5403 -3 h1.5402 -3 dt.5411 -4 pre.5409 -5 bodyTag.5394 -8 captionTag.5397 -8 address.5408 -11 dd.5412 +32 (+418/-386) R (DATA) +32 (+418/-386) UNDEF:libeditor.so:R +386 cite.4494 +32 kSubtreeIteratorCID -386 cite.4482 Windows: ===================================================================== Overall Change in Size Total: +3580 (+5531/-1951) Code: +3600 (+5507/-1907) Data: -20 (+24/-44) editor Total: +3580 (+5531/-1951) Code: +3600 (+5507/-1907) Data: -20 (+24/-44) +3600 (+5507/-1907) text (CODE) +2761 (+2761/+0) htmleditor_s:nsHTMLEditRules.obj +1605 public: __thiscall nsHTMLEditRules::nsHTMLEditRules(void) +274 protected: unsigned int __thiscall nsHTMLEditRules::CreateStyleForInsertText(class nsISelection *,class nsIDOMDocument *) +234 public: virtual unsigned int __stdcall nsHTMLEditRules::BeforeEdit(int,short) +181 protected: unsigned int __thiscall nsHTMLEditRules::ReapplyCachedStyles(void) +171 protected: unsigned int __thiscall nsHTMLEditRules::CacheInlineStyles(class nsIDOMNode *) +125 protected: unsigned int __thiscall nsHTMLEditRules::AfterEditInner(int,short) +63 public: __thiscall StyleCache::StyleCache(void) +43 public: struct PropItem & __thiscall PropItem::operator=(struct PropItem const &) +35 protected: unsigned int __thiscall nsHTMLEditRules::ClearCachedStyles(void) +24 public: virtual __thiscall nsHTMLEditRules::~nsHTMLEditRules(void) +3 public: __thiscall nsCOMPtr<class nsIDOMRange>::operator class nsDerivedSafe<class nsIDOMRange> *(void)const +3 unsigned int __cdecl NS_NewHTMLEditRules(class nsIEditRules * *) +782 (+2572/-1790) htmleditor_s:nsHTMLEditorStyle.obj +1927 protected: unsigned int __thiscall nsHTMLEditor::GetInlinePropertyBase(class nsIAtom *,class nsAString const *,class nsAString const *,int *,int *,int *,class nsAString *,int) +191 public: virtual unsigned int __stdcall nsHTMLEditor::AddDefaultProperty(class nsIAtom *,class nsAString const &,class nsAString const &) +134 public: virtual unsigned int __stdcall nsHTMLEditor::RemoveDefaultProperty(class nsIAtom *,class nsAString const &,class nsAString const &) +122 public: virtual unsigned int __stdcall nsHTMLEditor::RemoveAllInlineProperties(void) +98 protected: unsigned int __thiscall nsHTMLEditor::ApplyDefaultProperties(void) +84 public: virtual unsigned int __stdcall nsHTMLEditor::RemoveAllDefaultProperties(void) +12 public: virtual unsigned int __stdcall nsHTMLEditor::GetFontFaceState(int *,class nsAString &) +2 public: virtual unsigned int __stdcall nsHTMLEditor::GetFontColorState(int *,class nsAString &) +2 public: virtual unsigned int __stdcall nsHTMLEditor::GetInlinePropertyWithAttrValue(class nsIAtom *,class nsAString const &,class nsAString const &,int *,int *,int *,class nsAString &) -1790 protected: unsigned int __thiscall nsHTMLEditor::GetInlinePropertyBase(class nsIAtom *,class nsAString const *,class nsAString const *,int *,int *,int *,class nsAString *) +39 (+39/+0) htmleditor_s:nsHTMLEditor.obj +28 public: virtual __thiscall nsHTMLEditor::~nsHTMLEditor(void) +8 public: __thiscall nsHTMLEditor::nsHTMLEditor(void) +3 public: virtual unsigned int __stdcall nsHTMLEditor::GetHighlightColor(int *,unsigned short * *) +18 (+135/-117) htmleditor_s:TypeInState.obj +115 public: static int __cdecl TypeInState::FindPropInList(class nsIAtom *,class nsAString const &,class nsAString *,class nsVoidArray &,int &) +14 public: virtual unsigned int __stdcall TypeInState::NotifySelectionChanged(class nsIDOMDocument *,class nsISelection *,short) +3 protected: unsigned int __thiscall TypeInState::RemovePropFromClearedList(class nsIAtom *,class nsString const &) +3 protected: unsigned int __thiscall TypeInState::RemovePropFromSetList(class nsIAtom *,class nsString const &) -117 protected: int __thiscall TypeInState::FindPropInList(class nsIAtom *,class nsString const &,class nsString *,class nsVoidArray &,int &) +16 (+24/-8) rdata (DATA) +24 (+24/+0) htmleditor_s:nsHTMLEditor.obj +12 const nsHTMLEditor::`vftable'{for `nsIHTMLEditor'} +12 const nsIHTMLEditor::`vftable' -8 (+0/-8) UNDEF:editor:rdata -8 UNDEF:editor:rdata -4 (+0/-4) idata$5 (DATA) -4 (+0/-4) xpcom:xpcom.dll -4 __declspec(dllimport) public: __thiscall nsString::nsString(class nsString const &) -4 (+0/-4) idata$4 (DATA) -4 (+0/-4) UNDEF:editor:idata$4 -4 UNDEF:editor:idata$4 -28 (+0/-28) idata$6 (DATA) -28 (+0/-28) UNDEF:editor:idata$6 -28 UNDEF:editor:idata$6
The added size is half from filling out at array of structs in nsHTMLEditRules contructor. The structs have some strings in them - if anyone has advice on how to reduce the code size of that initalization please comment here.
cc'ing simon
Depends on: 208317
The work this patch is based on was originally done for a client that defaulted to non-css editing. When testing now on the tip in mozilla, which defaults to css editing, I see that there is an unrelated bug which blocks testing of this bug. I have filed this as 208317, assigned to me.
No longer depends on: 208317
Depends on: 208317
Attached patch improvements (deleted) — — Splinter Review
This patch is on top of earlier patch (wich has already landed on tip). This patch reworks nsHTMLEditRules constructor with a trivial change that reduces some of the bloat casued by original patch, and it fixes the problem earlier reported where additional backspaces caused style state to be lost. All of this can only be seen to work on non-css mode though, due to seperate bug 208317.
patch attached to 208317. Together with these patches, all known issues should be fixed.
Attachment #125199 - Flags: superreview?(kin)
Attachment #125199 - Flags: review?(brade)
Comment on attachment 125199 [details] [diff] [review] improvements Fix UpdateSelState to always return an nsresult. You can probably just remove the last NS_FAILED check and return result at the end. Fix the whitespace (tabs?) in this block: @@ -2123,16 +2125,19 @@
Attachment #125199 - Flags: review?(brade) → review+
Comment on attachment 125199 [details] [diff] [review] improvements In |UpdateSelState()| these aren't used: + nsCOMPtr<nsIDOMNode> selNode; + PRInt32 selOffset = 0; Address the things brade pointed out above and you have sr=kin@netscape.com
Attachment #125199 - Flags: superreview?(kin) → superreview+
Attachment #125199 - Flags: approval1.4?
Comment on attachment 125199 [details] [diff] [review] improvements a=asa (on behalf of drivers) for checkin to the 1.4 branch.
Attachment #125199 - Flags: approval1.4? → approval1.4+
Comment on attachment 124463 [details] [diff] [review] patch to editor a=asa (on behalf of drivers) for checkin to the 1.4 branch.
Attachment #124463 - Flags: approval1.4? → approval1.4+
a=adt. Please land on branch and add fixed1.4 keyword.
fix landed on tip. revisions: 1.287 & 1.286 mozilla/editor/libeditor/html/nsHTMLEditRules.cpp 1.28 & 1.27 mozilla/editor/libeditor/html/TypeInState.h 1.26 & 1.25 mozilla/editor/libeditor/html/TypeInState.cpp 1.50 mozilla/editor/libeditor/html/nsHTMLEditorStyle.cpp 1.27 mozilla/editor/libeditor/html/TypeInState.h 1.25 mozilla/editor/libeditor/html/TypeInState.cpp 1.15 mozilla/editor/idl/nsIHTMLEditor.idl 1.98 mozilla/editor/libeditor/html/nsHTMLEditRules.h 1.286 mozilla/editor/libeditor/html/nsHTMLEditRules.cpp 1.208 mozilla/editor/libeditor/html/nsHTMLEditor.h 1.473 mozilla/editor/libeditor/html/nsHTMLEditor.cpp
fix landed on 1.4 branch. revisions: 1.26.4.1 mozilla/editor/libeditor/html/TypeInState.h 1.24.4.1 mozilla/editor/libeditor/html/TypeInState.cpp 1.97.4.1 mozilla/editor/libeditor/html/nsHTMLEditRules.h 1.285.2.1 mozilla/editor/libeditor/html/nsHTMLEditRules.cpp 1.46.4.1 mozilla/editor/libeditor/html/nsHTMLEditorStyle.cpp 1.207.4.1 mozilla/editor/libeditor/html/nsHTMLEditor.h 1.470.4.1 mozilla/editor/libeditor/html/nsHTMLEditor.cpp
marking fixed since this has landed everywhere.
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Keywords: fixed1.4
Suresh, FYI: This patch won't obviate the need for the nim check after doing the selectAll/Delete. This patch only caches styles when the selection in *inside* the style in question. When doing a Select All, the selection will be in the <body>, and around (above) the style nodes. In that case, the style won't be cached, so the nim reset is still needed. What nim will benefit from is the case where a user sets a style in composing a message, types, then backspaces to delete what they typed, then types some more. In that case the style they set will still be present when they type. Trying to cache styles in the other case (where selection contains the style, rather than the style containing the selection) is much harder. We can talk about that offline - there are issues there.
Using branch build on winxp (haven't tested linux or mac yet) and the original scenario: This is fixed for Editor Not Fixed for HTML Mail Compose Not fixed for NIM which will need an additional fix, I think, as mentioned in comment 47. However, the portion of comment 47 regarding this fix correcting the NIM problem when backspace overrides the set style for NIM is fixed. Scenario in comment 1 Fixed for Composer Fixed for HTML Mail compose Fixed for NIM Reopening for HTML Mail Compose, if this should be a new bug due to it being a Mail bug now, let me know.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Also note: kathy did not see it working in Editor, after working with her we found out this is still broken in editor if you have unchecked "Use CSS styles instead of HTML elements and attributes (which is ON by default).
Update, MacOSX branch build 20030612 this fix has flakey behavior. Using the original scenario 123 (with 2 being bold) then backspacing over the 2 to see that the Bold attribute is still active: This fix works for a new profile with default setting in prefs 1st time you test (123) scenario. If I exit and relaunch app to same profile, it breaks, but I can get it back into a fixed state by: Sometimes just exiting and relaunch app can cause it to work again. Sometimes just toggling the preference "Use CSS styles instead of HTML elements and attributes" can cause this to work again. This fakey behavior does not happen on winxp or linux.
I want everyone involved to try this: only do your style expirements with backspace or forward delete, and only do them with multiple characters in the style run. This fix is not designed to work with creating a ranged selection with the mouse, and it will sometimes not work with single character style runs if you have more text after them. The reason for this is that the editor is only caching style if the selection is INSIDE the style when you did the deletion. So when you deleted the final character of a style run, the editor remembers that style if the selection was inside that style *at the beginning of the operation*. So the testcase for this bug doesn't really exactly match what the fix is for, since in this testcase you selection may or may not be inside the bold when you are about to delete the last bold character. But that doesn't mean the fix isn't valuable. It will work in the more usual cases: 1) someone makes bold, types 1 or more characters, backspaces thru them, then types again. Fix works here. 2) someone has a bunch of plaintext, selects some (more than one character), makes bold. Later they click or arrow to the end of this text, nd backspace through it. Then they type. Fix works here. So, given that, are there other problems? I'm in a bit of build hell here, so I won't have a build until later this evening. I'm concerned about the html vs. css mode comments. Do the cases I mention above work in html mode? If not then there is a problem with the patch.
Using branch build 20030612 on winxp and Your scenario 1 works only if you add a space to the end of the string. (in HTML mode and CSS mode for Editor and Mail Compose) Click Bold button, type 10 characters, then backspace(without a space after last character) to left margin, type again = Bold attribute not active Click Bold button, type 10 characters, then hit space bar to add a space, then backspace to left margin, type again = Bold attribute active Your scenario 2 does not work in HTML mode for editor or Mail Compose Does work in Editor in CSS mode.
Attached patch initializing variables, not just a good idea... (obsolete) (deleted) — — Splinter Review
give this a try.
Attachment #125547 - Flags: superreview?(sspitzer)
Attachment #125547 - Flags: review?
Attachment #125547 - Flags: approval1.4?
First off, a big thank you to Seth Spitzer who was my remote testing and debugging guinea pig while I was stuck in build hell. Here is the status with the latest patch from 140357, which corrects an unitialized variable in the editor: In all html editors (be they css-mode composer, non-css-mode composer, html mail compose, or nim) you can choose style, type a word, backspace through it exactly, and then type some more, and style will be preserved. There are the following remaining issues: 1) Extra backspaces cause cached style info to be lost. I have no explanation for this yet. The whole point of the second patch in 140357 was to fix this, and I did test it. Working on debugging this currently. 2) <big>/<small> is not preserved. Unlike other styles, you can't simply check for the presence of <big> or <small>. Instead what you have to know is how many nested levels of each got deleted. I can try creating a patch for this for the tip and we can evaluate from there. This problem affects composer and mail compose if you use the larger/smaller buttons on the toolbar, instead of the Format->Size menu. The latter sets font size, the former do <big>/<small>
Attached patch patch to nsHTMLEditRules.cpp (deleted) — — Splinter Review
This patch incorporates the initialization fix, obsoleted above. It also has changes that allow redundant backspacing to not perturb cached style state. translation: (bold) "Z" (backspace)(backspace)(backspace) "W"; the W will be bold. There is still the issue I discussed in comment 51 which can cause the original testcase to still not work. I've investigated if there was a simple low risk fix for that and concluded there isn't. That should be tackled post-1.4. The <big>/<small> issue discussed in comment 54 is still present as well.
Attachment #125547 - Attachment is obsolete: true
Attachment #125576 - Flags: superreview?(kin)
Attachment #125576 - Flags: review?(brade)
Attachment #125576 - Flags: approval1.4?
Comment on attachment 125576 [details] [diff] [review] patch to nsHTMLEditRules.cpp fix typo (cahced) in this comment: + // When we apply cahced styles to TypeInState, we always want The last block I'd rewrite like this (lines with significant changes start with '*': * PRBool bAny; nsAutoString curValue; if (useCSS) // check computed style first in css case { mHTMLEditor->mHTMLCSSUtils->IsCSSEquivalentToHTMLInlineStyleSet( selNode, mCachedStyles[j].tag, &(mCachedStyles[j].attr), bAny, curValue, COMPUTED_STYLE_TYPE); } * else * bAny = PR_FALSE; if (!bAny) // then check typeinstate and html style { * PRBool bFirst, bAll; res = mHTMLEditor->GetInlinePropertyBase(mCachedStyles[j].tag, &(mCachedStyles[j].attr), &(mCachedStyles[j].value), &bFirst, &bAny, &bAll, &curValue, PR_FALSE); if (NS_FAILED(res)) return res; }
Attachment #125576 - Flags: review?(brade) → review+
Comment on attachment 125576 [details] [diff] [review] patch to nsHTMLEditRules.cpp sr=kin@netscape.com
Attachment #125576 - Flags: superreview?(kin) → superreview+
Attachment #125547 - Flags: superreview?(sspitzer)
Attachment #125547 - Flags: review?
Attachment #125547 - Flags: approval1.4?
I'll ping drivers about this for 1.4 final. I think we want it because of what it will do for mail compose and editor (in non-css mode). note, we may have missed 1.4 rc 2. from 1.4 branch tinderbox: "The 1.4 branch is closed to all checkins while we try to get RC2 compiled and released. Unless you have explicit permission from drivers to land on a closed branch, please do not check in here. Thanks."
Status: REOPENED → ASSIGNED
Flags: blocking1.4? → blocking1.4+
Whiteboard: EDITORBASE+ edt_x3, fixinhand [seeking approval] → EDITORBASE+ edt_x3, fixinhand [a=sspitzer,asa for 1.4 final, but wait until after rc2]
Comment on attachment 125576 [details] [diff] [review] patch to nsHTMLEditRules.cpp a=sspitzer,asa for 1.4 final, but wait until after rc2 is out the door. (which should be any minute now)
Attachment #125576 - Flags: approval1.4? → approval1.4+
Blocks: 209395
Ok, third patch is now on 1.4 branch and on tip. Lets open new bugs for any edge cases to avoid infinite bug mutation. marking fixed...
Status: ASSIGNED → RESOLVED
Closed: 21 years ago → 21 years ago
Resolution: --- → FIXED
Keywords: fixed1.4
Using branch builds 20030619 on winxp, macosx and linux and the scenarios (1&2) provided in comment 51, this is fixed with CSS editing on and off. I tested Bold, Italic, Underlined, and a color in scenario 1. Also, in scenrario 1 I hit backspace multiple times at the left margin (#1 of comment 54), the bold attribute is still active, so that is fixed too. The simple scenario involving 1 character (as originally reported) is not fixed but that is not part of this fix per comment 51. replacing fixed1.4 with verified1.4
Keywords: fixed1.4 → verified1.4
Note, the testing in comment 61 was done in mailnews compose and composer.
I spoke with beppe, she wanted these tested too: Can you please try one thing in composer. Initiate an aim session with someone, have several comments back and forth. Save the conversation as an html file. Open that html file, set focus on a line, outside of the screenname, backspace till you hit the screen name and see if you can backspace into and out of the screen name. If you can, then this bug would be considered fixed in my opinion. Result: I could backspace over the screenname however there was a hesitation when the backspace hit the : portion of the screen name which looked like it was stopping. Note: I saw this same behavior in the build before the fix so I'm not sure if it was actually stopping or just hesitating in previous builds. 2nd test: also, I think this may be the bug where you have [bold]aaaa[/bold]bbbb and you can't backspace into the bold text. Result this works in both previous and current builds.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: