125345 - Font attributes disappear when you double click editor window

Status: VERIFIED FIXED

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

Description

[Joseph Elwell]

Steps to reproduce:
1) Launch IM Compose window or Composer
2) Select a font color
3) double click in the editor area

Actual Results:
The font color reverts to black.

Expected Results:
The Color shouldn't reset just because I clicked on the editor window a few times.

This causes annoying problems in Instant Messenging because the user is often
switching between applications and Instant Messenging windows.

Comment 1

[Joseph Elwell]

nominating.

Comment 2

[Kevin McCluskey (gone)]

Marking nsbeta1-

Comment 3

[michael wendell]

I am seeing this problem on the 02-14 trunk builds on both Win 2k and Mac OSX.  

I am switching the platform and OS to all/all.

Comment 4

[kmurray1115]

Kevin, any more info as to why was this minused? BTW - this occurs with single
click to regain focus into IM editor window - style lost. Pretty common if user
is IM'ing with more than one window open, or loses focus and clicks back into
compose area.

Comment 5

[Kathleen :Brade]

removing minus to get this on Kevin McCluskey's radar

Comment 6

[Kevin McCluskey (gone)]

I could only get it to reset back if I followed the original instructions in the
bug. I had to set the color and then immediately double click. This is why I
marked this bug nsbeta1-. It required a set of steps that I didn't consider that
common since it required a double click in the body of the editor or im window
which didn't have any defined action except to clear the current color
selection. Double clicking over text reset the color to match the text that was
double clicked on.

I'll nsbeta1+ since others are seeing different results so we can investigate
further.

Comment 7

[Kevin McCluskey (gone)]

[adt2]

Comment 8

[Kevin McCluskey (gone)]

nominating for EDITORBASE

Comment 9

[kmurray1115]

I highly recommend and EDITORBASE+ on this one. I don't see the steps to
reproduce as complex as the ones KevinMcC describes. My specific steps:
1. Make suree you have a default style set up via Prefs|IM|Styles
2. Launch an IM window by either sending as initiator or receive on from a buddy
3. Type an IM and hit enter (or click Send)
4. Single click into the IM editor window 
Note that default pen/style has reverted to Variable Width and some alteration
has occurred to the font size and color (different results based on what default
style you have chosen). Also note that I didn't switch windows and come back to
cause this. That's just a way of encountering the bug more frequently. This
occurs whether you switch windows or not - all that is required to reproduce (on
Win32) is a single click in to the IM editor after you've set a default style.

Comment 10

[Kevin McCluskey (gone)]

4.x inserts a font tag with a non breaking space when the font color is set. Why
aren't we doing the same?

EDITORBASE+.

Comment 11

[Syd Logan]

there might be a solution for this in IM application code.

Comment 12

[kinmoz]

Attachment: Patch Rev 1

This patch prevents us from clearing the Editor's TypeInState when we get a
"selection changed" notification, andthe caret really hasn't moved. I tried to
fix it properly in selection, but then found that I'd have to change several
things in selection, so rather than risk potential regressions to selection or
any thing listening to it at this point in the game, I decided to file a bug on
the fact that we get these bogus notifications (bug 140303), so mjudge can
address it properly later.

It should be noted that this patch only fixes the problem when clicking. You
can get this same loss of default text styles by typing a letter and then
backspacing. That is due to a separate problem, which I've filed as bug 140357.
I also noted that the way the code in AimIM.js resets the styles after message
submission could lead to inconsistent behavior from AOL IM.

An interesting side-effect of my proposed change is that now the TypeInState is
not cleared when editor operations make changes to the parent of the node
containing the caret ... So for example if I placed the caret in a list item,
and hit the bold button, then indented, outdented, or unlisted that list item,
and then typed in a character, it would appear bold since technically the caret
never moved.

jfrancis so what do you think? Can I get a review from you? Or at least some
comments on potential problems this could cause?

Comment 13

[Joe Francis]

Comment on attachment 81186 [details] [diff] [review]
Patch Rev 1

r=jfrancis.  i discussed some optional changes with kin (not using the "anchor"
routines, adding error checking).  But I think this is ok either way.

Comment 14

[kinmoz]

Attachment: Patch Rev 1.1

This patch is pretty much the same as the first patch with the following
changes:

  - Renamed anchor* to sel*.
  - Call nsEditor::GetStartNodeAndOffset() instead of selection
    GetAnchor*() methods.
  - Add error checking.

Comment 15

[Marc Attinasi]

Comment on attachment 81490 [details] [diff] [review]
Patch Rev 1.1

sr=attinasi

Comment 16

[kinmoz]

Fix (Patch Rev 1.1) checked into the TRUNK:

  mozilla/editor/libeditor/html/TypeInState.cpp  revision 1.19
  mozilla/editor/libeditor/html/TypeInState.h    revision 1.25

Comment 17

[Jaime Rodriguez, Jr.]

Let's review this one in the RTM timeframe. adt1.0.0- [adt2 RTM]

Comment 18

[scottputterman]

Let's get this verified on the trunk.  renominating.

Comment 19

[michael wendell]

Verified on the 05-08 trunk build.

Comment 20

[scottputterman]

adding adt1.0.0+.  Please get drivers approval and then check into the 1.0 branch.

Comment 21

[scottputterman]

changing to adt1.0.1+ for checkin to the 1.0 branch.  Please get drivers
approval before checking in.

Comment 22

[chris hofmann]

a=chofmann for 1.0.1

Comment 23

[chris hofmann]

Comment on attachment 81490 [details] [diff] [review]
Patch Rev 1.1

please checkin to the 1.0.1 branch. once there, remove the "mozilla1.0.1"
keyword and add the "fixed1.0.1" keyword." in the comments.

Comment 24

[kinmoz]

Fix (Patch Rev 1.1) checked into the MOZILLA_1_0_BRANCH:

    mozilla/editor/libeditor/html/TypeInState.cpp  revision 1.18.36.2
    mozilla/editor/libeditor/html/TypeInState.h    revision 1.24.36.2

Comment 25

[sujay]

verified in 6/4 branch build.