85 bytes, text/html
86 bytes, text/html
13.16 KB, patch
|Details | Diff | Splinter Review|
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
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.
> 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 :-)
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" ?
Confirming bug on Mac OS X with 1.4 branch, setting platform to ALL.
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
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.
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
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.
Created attachment 124740 [details] [diff] [review] Patch v2 - diff -w Fixed copy&paste error in patch.
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.
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.
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 :-)
Changing to better summary.
> 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.
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.
Created attachment 124885 [details] [diff] [review] Patch v3 implemented jfrancis' suggestion
Comment on attachment 124885 [details] [diff] [review] Patch v3 looks good!
Comment on attachment 124885 [details] [diff] [review] Patch v3 email@example.com ==== I'd rename |ar_outsideSelectAll| to |ar_outsideUserSelectAll| or |ar_outsideUserSelectAllSubtree|.
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.
Created attachment 125319 [details] [diff] [review] Patch v3b New patch, with the only change: - Renamed ar_outsideSelectAll to eOutsideUserSelectAll - Renamed ar_anywhere to eAnywhere
Comment on attachment 125319 [details] [diff] [review] Patch v3b carrying forward r=jfrancis and sr=kin
fixed on trunk
Comment on attachment 125319 [details] [diff] [review] Patch v3b requesting 1.4 branch approval for this editor correctness fix
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.
This problem was also reported in bug 211082.
Comment on attachment 125319 [details] [diff] [review] Patch v3b moving approval request forward.
Comment on attachment 125319 [details] [diff] [review] Patch v3b a=mkaply
Please add the fixed1.4.1 keyword when this is checked in.
Checked in to 1.4 branch for 1.4.1