80550 - Bidi code killed by checkin to bug 65557

Status: VERIFIED FIXED

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

Description

[Simon Montagu :smontagu]

A block of IBMBIDI code in nsHtmlEditRules::WillDeleteSelection has disappeared.

In a comment in bug 65557 jfrancis says:

"I ripped out the ibmbidi stuff out of nsHTMLEditRules.cpp.  This is just
temporary - it will be back in before this lands."

If not putting it back was an oversight, please correct it :-)
If there is a problem with the ibmbidi stuff, let me know what needs fixing

Comment 1

[Joe Francis]

Hey, you beat me to filing this bug.

There is no known problem with IBMBIDI here.  I do need to make sure I 
reintegrate it in the right place.  I'm also thinking that it should go into a 
seperate routine, in a seperate source file.  nsHTMLEditRules.cpp is already too 
big, and that bidi stuff really reduces the already poor readability of the 
WillDeleteSelection code.  Also, I'm wondering if it needs to be called from more 
than one place.  My memory is telling me that before it was only called in the 
backwards deletion case, not the forwards one.  But I might have that wrong.

Comment 2

[Joe Francis]

moz 091

Comment 3

[Simon Montagu :smontagu]

I just did some debugging on this in my local tree, and both the backward and
forward deletion cases call the IBMBIDI code correctly.

Putting this in a separate file sounds like a good idea. Maybe it could be
combined with the similar logic in nsTextEditRules::WillDeleteSelection and be
called from both places. 

As for the right place to reintegrate it, it certainly has to be before the test
for whitespace. I don't know about the test for "if we are inside an empty
block". When does that condition occur? I couldn't reproduce it.

By the way, I remembered that there is one open problem with this code - see bug
74948

Comment 4

[Joe Francis]

Simon, can you explain to me the intent of the BIDI code in question?  I don't 
understand it, which prevents me from knowing exactly what situations it should 
be called.

Comment 5

[Simon Montagu :smontagu]

The object of this code is to prevent deletion of a character which is not
visually adjacent to the caret location in reordered text. For example, if I type

latinHEBREW

and move the caret back to before the H of HEBREW, it will appear as

latinWERBEH|

where | represents the caret.

Backward deletion at this point would delete the character logically before the
caret, which is the "n" of latin. Especially if the Hebrew text was long, I
might not even notice what was happening, and it would be generally very confusing.

To prevent this scenario the code here detects that the caret is between two
characters with different directionality (or embedding levels, in the terms of
the Bidi algorithm), changes the Bidi level of the caret, and cancels the
deletion. The change in the Bidi level of the caret moves the visual location of
the caret, like this:

latin|HEBREW

(see the IBMBIDI code in nsCaret::SetupDrawingFrameAndOffset), and the next
backward deletion deletes the "n".

Comment 6

[Joe Francis]

Ok, cool.  I was pretty close but that explanantion helps a lot.  I'll try to 
make a version that works for both text and html edit rules, and put it in a 
seperate file.

Comment 7

[Joe Francis]

Attachment: new file editor/base/nsTextEditRulesBIDI.cpp

Comment 8

[Joe Francis]

Attachment: diffs of sources in editor/base

Comment 9

[Joe Francis]

Attachment: diffs of Makefile.in

Comment 10

[Joe Francis]

Attachment: diffs of editor/base/makefile.win

Comment 11

[Joe Francis]

Ok, I've taken a shot at this.  I put what is basically the htmleditrules 
version of the BIDI deletion code into a seperate file, editor/base/
nsTextEditRulesBIDI.cpp.  It contains one function 
CheckBIDILevelForDeletion().  I'm assuming that the html version works 
properly for plaintext as well (it just might do some unneeded checking).

I tweaked texteditrules and htmleditrules to use this function.  I also 
cleaned up the includes a bit and made new makefiles to build it.

Mac folks who want to test this will have to manually add 
nsTextEditRulesBIDI.cpp to their editor project.

Simon, can you test this?  I'm not really qualified to evaluate the results.

Comment 12

[Simon Montagu :smontagu]

I am testing this yesterday and today. It looks mostly good so far, but the call 
from nsHTMLEditRules.cpp has to move earlier, and I want to rewrite 
nsTextEditRulesBIDI.cpp -- now that this stuff is in a separate method it can 
return immediately on failure without the huge nested if.

Comment 13

[Simon Montagu :smontagu]

Attachment: new version of editor/base/nsTextEditRulesBIDI.cpp

Comment 14

[Simon Montagu :smontagu]

Attachment: modified patch after testing

Comment 15

[kinmoz]

Comments on nsTextEditRules::CheckBIDILevelForDeletion():

  -- Just a comment ... I tend to discourage the pattern:

       if (NS_FAILED(res) || !frameAfter)
         return NS_ERROR_FAILURE;

     in favor of the more long winded:

       if (NS_FAILED(res))
         return res;
       if (!frameAfter)
         return NS_ERROR_NULL_POINTER;

     I've fixed many bugs having to do with this
     pattern, so I'm just trying to prevent people
     from getting into the habbit of using this pattern.

  -- We don't seem to check errors when calling PresShell/Frame
     Bidi methods, is this intentional?

  -- I know the following works, but can we put braces around
     the "if" clause anyways?


        if (frameBefore==frameAfter)
          // there was no frameAfter, i.e. the caret is at the end of the
          // document -- use the base paragraph level
          frameBefore->GetBidiProperty(context, baseLevel, (void**)&levelAfter);
        else
  
  -- We can return from the method without aCancel ever being initialized.

Comment 16

[kinmoz]

Comments on the 05/17/01 06:49 patch:

  -- I believe that Joe placed the CheckBIDILevelForDeletion() call,
     in his original nsHTMLEditRules.cpp patch, where he did because
     startNode and startOffset can be modified by the Plaintext whitespace
     code. Are we sure we want to call CheckBIDILevelForDeletion()
     before this whitespace code is executed? 

  -- I noticed that you moved the CheckBIDILevelForDeletion() call,
     in nsTextEditRules.cpp, up into the "collapsed and in a text node"
     case. Are you sure that is the only case that needs
     CheckBIDILevelForDeletion() called. Keep in mind that it is
     entirely possible that the caret could be next to a textnode,
     either before or after it in it's block parent, and any
     inserts/deletes could still modify that textnode.

  -- If you decide to move the CheckBIDILevelForDeletion() call
     back to where Joe had it in his patch, we would need to
     check to makes sure that startNode was not null before
     calling CheckBIDILevelForDeletion() to avoid an unintended
     error from being thrown.

Comment 17

[Simon Montagu :smontagu]

The call to CheckBIDILevelForDeletion definitely has to be before the whitespace
code. Although whitespace is initially "neutral" in terms of directionality,
after performing the Bidi algorithm it acquires a direction from the surrounding
text, so the testing in CheckBIDILevel... is just as important for whitespace as
for non-whitespace.

Point taken about "in a text node", and the comments on the new file. Will
attach new diffs today.

Comment 18

[Simon Montagu :smontagu]

Attachment: new version of nsTextEditRulesBIDI.cpp addressing kin's comments

Comment 19

[Simon Montagu :smontagu]

Attachment: new diffs of editor/base addressing kin's comments

Comment 20

[Joe Francis]

ok, i will approve this patch.  But I do think more work may be needed in future. 

Issues:

1)
As the patch stands, we check to see if BIDI code wants to cancel the deletion 
and move the caret BEFORE we adjust selection to skip past non-significant 
whitespace.  Is the BIDI code smart enough here?  Will it think that the 
intervening whitespace is about to be deleted and thus not cancel the deletion?  
If so, what will happeni is that I will skip over that ws and delete the next 
"real" text, which might have different directionality and cause confusing 
behavior.

2)
A similar but additional issue exists in nsTextEditRules.cpp.  There I also 
delete any intervening empty text nodes which may have accumulated.  Will BIDI 
code think that an ampty text node of same directionality is going to be deleted 
and not cacel, when in fact I will continue on until I find some "real" text to 
delete, again possibly of different directionality?

Having said all that, if SImon thinks the behavior will be better with his 
changes to the patch than without them, I say go ahead and land this.  I would 
like someone with BIDI knowledge to think about the issues I have raised, though, 
and open bugs on them if appropriate.

r=jfrancis

assigning to mkaply per ftang request.  updating status whiteboard.

Comment 21

[Simon Fraser [no longer active]]

Comments on the last patch:

+#ifdef IBMBIDI // Test for distance between caret and text that will be deleted

Move the comment under the #ifdef line. The comment is saying what the code does, 
not what the #ifdef means (this is in 2 places).

Comments on nsTextEditRulesBIDI.cpp:

  long levelAfter;
  long levelBefore;
  long levelOfDeletion;

Should use PRInt32, although I note that nsIFrame::GetBidiProperty uses *longs. 
Why?

  nsCOMPtr<nsIAtom> embeddingLevel = NS_NewAtom("EmbeddingLevel"); 
  nsCOMPtr<nsIAtom> baseLevel = NS_NewAtom("BaseLevel");

These leak! See bug 59212.

Fix these, and sr=sfraser

Comment 22

[Mike Kaply [:mkaply]]

i think we need to open a separate bug for GetBidiProperty using longs. that is 
outside the scope of this bug.

Comment 23

[Mike Kaply [:mkaply]]

Attachment: nsTextEditRulesBidi.cpp - addref added and longs changed to PRuint8

Comment 24

[Mike Kaply [:mkaply]]

Attachment: Makefile changes that preserve IBMBIDI - removed #ifdef from nsTextEditRulesBidi.cpp

Comment 25

[Daniel (Leaf) Nunes]

r=leaf for build system changes in 35500

Comment 26

[Mike Kaply [:mkaply]]

Checked in

Comment 27

[sujay]

Michael/Simon, can one of you please verify this and mark this bug
VERIFIED-FIXED? thanks.

Comment 28

[Simon Montagu :smontagu]

v=[e-mail]