Closed Bug 140357 Opened 22 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
33.83 KB, patch
Brade
: review+
kinmoz
: superreview+
asa
: approval1.4+
Details | Diff | Splinter Review
improvements
9.35 KB, patch
Brade
: review+
kinmoz
: superreview+
asa
: approval1.4+
Details | Diff | Splinter Review
patch to nsHTMLEditRules.cpp
2.93 KB, 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 — — 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 — — 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.


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