Smiley gets deleted, editor tries to manipulate whitespace in the -moz-user-select:all block

RESOLVED FIXED in mozilla1.5alpha

Status

()

Core
Editor
RESOLVED FIXED
15 years ago
14 years ago

People

(Reporter: kaie, Assigned: kaie)

Tracking

({fixed1.4.1})

Trunk
mozilla1.5alpha
x86
All
fixed1.4.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.4.1 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [adt2] editorbase)

Attachments

(3 attachments, 5 obsolete attachments)

(Assignee)

Description

15 years ago
This bug is seen *only* on the initial attempt in a Mozilla session.
If you need to test again, you need to restart Mozilla!

Seen while testing bug 118912. The patch in bug 118912 seems to have caused this
new behaviour, but that change seems necessary.


To reproduce:
- start Mozilla
- open a new mail message compose window
- type some chars, e.g. "wsfijf"
- use smiley toolbar icon to insert the topmost smiley
- type any letter, e.g. "x"

Actual behaviour:
  The smiley gets deleted

Expected behaviour:
  The smiley should not get deleted

Comment 1

15 years ago
This bug is most likely caused by the selection being inside the span when the
smiley is inserted. 

Probably the code that inserts the smiley places the selection beyond the span,
but then later one (in the processing handled by
nsHTMLEditRules:AfterEditInner()) the selection is getting pulled back into the
span (in the case where the smiley is at the end of it's block, as is often the
case when you insert it as you type).

The selection adjusting code needs to make sure that it does not cross
-moz-user-select: all boundaries when adjusting the selection.

I don't know why you only see this the first try.  It's probably something silly
like the additioan of a trailing br once you do the delete step.  This would
then give the selection somewhere happy to be other than in the span the next
time you try.
(Assignee)

Comment 2

15 years ago
> I don't know why you only see this the first try.  It's probably something silly
> like the additioan of a trailing br once you do the delete step.  This would
> then give the selection somewhere happy to be other than in the span the next
> time you try.

I think you misunderstood. It's not happening each time I open a mail compose
window, it's only happening the first time I open a mail compose window!

If it were something like an additional <br>, I think I should be able to
reproduce each time I open a new mail composer window, but I don't :-)
(Assignee)

Comment 3

15 years ago
At times I see the bug, we are constructing a nsWSRunObject which decides
  mEndReason == eThisBlock (128)
and ww get a WS run of
  eTrailingWS
which causes nsWSRunObject::InsertText
to call DeleteChars to delete the trailing space after the smiley.
However, all of the smiley gets deleted.
(GetNextWSNode in GetWSNodes returns NULL)

At times it works correctly, we get
  mEndReason == eBreak (32)
and we get a WS run of
  eNormalWS
which only causes nsWSRunObject::InsertText
to do some NBSP checking.


When I do
  "open new mail message, type x, insert smiley, send mail"
the first time after having started Mozilla, I get
  <body text="#000000" bgcolor="#ffffff">
  x<span class="moz-smiley-s1"><span> :-) </span></span>
  </body>

and at any later time I get
  <body text="#000000" bgcolor="#ffffff">
  x<span class="moz-smiley-s1"><span> :-) </span></span><br>
  </body>


One could argue, it is a bug that we don't get the trailing <br> on the initial
attempt in the application session.

But I think it's not good we rely on the trailing <br> so much.


What would the real fix be?
  "Do not try to delete the trailing space at the end of a block, 
   when the space is part of a moz-select-all block"
?
(Assignee)

Updated

15 years ago
Whiteboard: editorbase

Comment 4

15 years ago
adt: nsbeta1+/adt2
Keywords: nsbeta1+
Whiteboard: editorbase → [adt2] editorbase
(Assignee)

Comment 5

15 years ago
Confirming bug on Mac OS X with 1.4 branch, setting platform to ALL.
OS: Windows 2000 → All
(Assignee)

Comment 6

15 years ago
There is another variation of this regression, probably also because the editor
code is trying to clean up whitespace.

I'm not yet sure whether I'll file a separate bug for this other regression.,
for now I'll not.

To reproduce the other regression:
- use any document, new or existing
- place caret at location of choice
- enter char a
- enter a smiley
- enter char b
- hit backspace

Expected behaviour:
  only "b" should get deleted

Actual behaviour:
  both "b" and the smiley left to it get deleted
(Assignee)

Comment 7

15 years ago
Created attachment 124718 [details]
test case 1 - type a letter after smiley

Testcase 1.

Open in editor.
Place caret after smiley.
Type any letter.
Smiley gets deleted.
(Assignee)

Comment 8

15 years ago
Created attachment 124719 [details]
test case 2 - hit backspace at end of document

test case 2

- open in editor
- place caret at end of document
- hit backspace
- both "b" and smiley get deleted
(Assignee)

Comment 9

15 years ago
By now it's clear this bug caused by the editor's logic attempt to clean up the
whitespace surrounding the smiley, when that whitespace appears to be insignificat.
However, that surrounding whitespace is part of the smiley atom by use of the
-moz-user-select property, and deleting it deletes all of the smiley.

For testcase 1, the code is deleting a space.
For testcase 2, the code is trying to convert a space into a NBSP, but again, it
does so by trying to delete a space, which results in deleting all of the smiley.


I'm not yet sure what the ideal fix for this bug should be.

My only idea so far is: When deleting, never clean up insignificant whitespace
that is contained in select-all blocks.

I'm attaching a patch that fixes both testcases.
(Assignee)

Comment 10

15 years ago
Created attachment 124737 [details] [diff] [review]
Patch v1 - diff -w
(Assignee)

Comment 11

15 years ago
Created attachment 124738 [details] [diff] [review]
Patch v1 - full patch
(Assignee)

Updated

15 years ago
Attachment #124738 - Flags: review?(jfrancis)
(Assignee)

Updated

15 years ago
Attachment #124738 - Attachment is obsolete: true
Attachment #124738 - Flags: review?(jfrancis)
(Assignee)

Comment 12

15 years ago
Created attachment 124740 [details] [diff] [review]
Patch v2 - diff -w

Fixed copy&paste error in patch.
Attachment #124737 - Attachment is obsolete: true
(Assignee)

Comment 13

15 years ago
Created attachment 124741 [details] [diff] [review]
Patch v2 - full patch
(Assignee)

Updated

15 years ago
Attachment #124741 - Flags: review?(jfrancis)

Comment 14

15 years ago
I mentioned this to kaie on IRC, but thought I'd mention it here ... the fact
that it only happens on the initial open of a mail window, is probably due to
the fact that mail caches the MailCompose window ... and when you initially
bring it up, it is blank and the edit rules probably have the mBogusNode pointer
set ... when you close it and bring it up again, I believe the mail app just
does a select all and delete to clear the window out ... which does *not*
restore the mBogusNode.

I suggested to kaie that you may be able to reproduce the problem in new mail
compose windows by leaving the old one up, forcing mail to create a new window
instead of using the cached version.

Comment 15

15 years ago
I took a peek at the patch, and it seems to work ok. What I'm wondering is if we
should only skip the various deletes when we cross a boundary from editable to
non-editable ... for example when we are inserting text into an editable region
of the doc but the ws code crosses into a non-editable region when looking for
whitespace to delete, as is  the case in this bug ... but do we want to allow
someone to edit the contents underneath a -moz-user-select:all subtree
programatically with the editor? If so, this patch will prevent the ws run
object from doing it's magic.
(Assignee)

Comment 16

15 years ago
Kin:

> but do we want to allow
> someone to edit the contents underneath a -moz-user-select:all subtree
> programatically with the editor? 

Daniel says: Yes, we do, in templates.


> If so, this patch will prevent the ws run
> object from doing it's magic.

I'm not sure I understand correctly, so I must state what I assume you are saying.

I assume you are saying: "When we manipulate text inside a contents underneath a
-moz-user-select:all subtree, we still want the nsWSRunObject to do it's magic".

I assume you mean, when manipulating text, you want whitespace cleanup to happen.

But, whenever the nsWSRunObject tries to do "it's magic" and cleanup whitespace,
won't it automatically always delete the whole object?

From my current knowledge I would have assumed: Any attempt to manipulate the
whitespace inside such an protected block is futile. One must remove the
grouping (done with moz-select:all) before the intended action can succeeded.

Please help me to undestand better :-)
(Assignee)

Comment 17

15 years ago
Changing to better summary.
Summary: Initial attempt in session only: Typing any char deletes smiley → Smiley gets deleted, editor tries to manipulate whitespace in the -moz-user-select:all block

Comment 18

15 years ago
> I assume you are saying: "When we manipulate text inside a contents
> underneath a -moz-user-select:all subtree, we still want the nsWSRunObject
> to do it's magic".
>
> I assume you mean, when manipulating text, you want whitespace cleanup to
> happen.


Right, I was wondering if we should allow editing operations that happen
*entirely* underneath a -moz-user-select:all subtree to proceed as normal,
whitespace cleanup and all. How that could be accomplished is still up for
dicsucssion.


> But, whenever the nsWSRunObject tries to do "it's magic" and cleanup
> whitespace, won't it automatically always delete the whole object?


It may with the current implementation.


> From my current knowledge I would have assumed: Any attempt to manipulate the
> whitespace inside such an protected block is futile. One must remove the
> grouping (done with moz-select:all) before the intended action can succeeded.


A while back we had a meeting where we discussed -moz-user-select:all and it's
current use ... if I remember correctly we came up with the conclusion that
-moz-user-select:all wasn't the same as not-editable ... that is
-moz-user-select:all basically meant that user gestures (clicks, user initiated
edits, etc) would treat the subtree as a unit and it if it wasn't marked as
not-editable, you should be able to edit the subtree.

I'm curious what jfrancis has to say about all this. I was just flagging the
issue for discussion because it seems like we'll run into bugs like this more
and more, and if we keep putting checks, like we do in the patch, at the lowest
levels, we may never be able to edit these subtrees without actually removing
the -moz-user-select:all style as you mentioned above.

Comment 19

15 years ago
I think this patch is exactly the right idea.  One thing I would like to see is
for a bool to be added to DeleteChars() that indicates if we need to check for
select-all style.  That way we have the code that checks in only one place,
which is easier to maintain and less cluttered.
(Assignee)

Comment 20

15 years ago
Created attachment 124885 [details] [diff] [review]
Patch v3

implemented jfrancis' suggestion
Attachment #124740 - Attachment is obsolete: true
Attachment #124741 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #124741 - Flags: review?(jfrancis)
(Assignee)

Updated

15 years ago
Attachment #124885 - Flags: review?(jfrancis)

Comment 21

15 years ago
Comment on attachment 124885 [details] [diff] [review]
Patch v3

looks good!
Attachment #124885 - Flags: review?(jfrancis) → review+

Comment 22

15 years ago
Comment on attachment 124885 [details] [diff] [review]
Patch v3

sr=kin@netscape.com

==== I'd rename |ar_outsideSelectAll| to |ar_outsideUserSelectAll| or
|ar_outsideUserSelectAllSubtree|.
Attachment #124885 - Flags: superreview+

Comment 23

15 years ago
Comment on attachment 124885 [details] [diff] [review]
Patch v3

Also as a sidenote, I think most of the enums under the  editor directory try
to stick to  using 'k' or 'e' prefixes.
(Assignee)

Comment 24

15 years ago
Created attachment 125319 [details] [diff] [review]
Patch v3b

New patch, with the only change:
- Renamed ar_outsideSelectAll to eOutsideUserSelectAll
- Renamed ar_anywhere to eAnywhere
Attachment #124885 - Attachment is obsolete: true
(Assignee)

Comment 25

15 years ago
Comment on attachment 125319 [details] [diff] [review]
Patch v3b

carrying forward r=jfrancis and sr=kin
Attachment #125319 - Flags: superreview+
Attachment #125319 - Flags: review+
(Assignee)

Comment 26

15 years ago
fixed on trunk
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.5alpha
(Assignee)

Comment 27

15 years ago
Comment on attachment 125319 [details] [diff] [review]
Patch v3b

requesting 1.4 branch approval for this editor correctness fix
Attachment #125319 - Flags: approval1.4?

Comment 28

15 years ago
This should go on 1.4 if possible.  There has been interest from embedders over
the -moz-user-select:all style, and honoring it correctly in editor.  
(Assignee)

Comment 29

15 years ago
This problem was also reported in bug 211082.
(Assignee)

Updated

15 years ago
Blocks: 211307

Comment 30

15 years ago
Comment on attachment 125319 [details] [diff] [review]
Patch v3b

moving approval request forward.
Attachment #125319 - Flags: approval1.4? → approval1.4.x?

Comment 31

15 years ago
Comment on attachment 125319 [details] [diff] [review]
Patch v3b

a=mkaply
Attachment #125319 - Flags: approval1.4.x? → approval1.4.x+

Comment 32

15 years ago
Please add the fixed1.4.1 keyword when this is checked in.
Flags: blocking1.4.x+
(Assignee)

Comment 33

15 years ago
Checked in to 1.4 branch for 1.4.1
Keywords: fixed1.4.1

Updated

14 years ago
Blocks: 224532
You need to log in before you can comment on or make changes to this bug.