76003 - bidi: rendering context's state corrupted

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect)

Description

[Erik van der Poel]

Subject: nsCaret::DrawCaret (IBMBIDI #ifdef)
Date: Thu, 12 Apr 2001 19:41:47 -0400 (EDT)
From: "L. David Baron" <[e-mail]>
To: [e-mail]
CC: [e-mail]

I was just looking at nsCaret::DrawCaret and happened to notice what
seems to be a bug in the |#ifdef IBMBIDI| part of the function.

Within the ifdef-ed part, there is a |return| statement.  This is bad
because it's between |mRendContext->PushState()| and
|mRendContext->PopState()| -- it would mess up the state stack for
all the callers.

I'm also wondering if:
 * the condition for the |return| should be |NS_FAILED| rather than
   |NS_SUCCEEDED|
 * the error condition should be propagated by making |DrawCaret|
   return |nsresult| rather than |void|.

-David

L. David Baron        <URL: http://www.people.fas.harvard.edu/~dbaron/ >
Mozilla Contributor                      <URL: http://www.mozilla.org/ >
Invited Expert, W3C CSS WG          <URL: http://www.w3.org/Style/CSS/ >

Comment 1

[Simon Montagu :smontagu]

Attachment: pop the state stack before returning

Comment 2

[Simon Montagu :smontagu]

NS_SUCCEEDED is the correct condition for the early return. If the language is
changed while the caret is between two characters with different directionality,
the caret will have to move to reflect where the next character typed is going
to appear. For example if I have

english|WERBEH

with a right-to-left caret, the logical position of the caret is after the W. If
the next character typed is Hebrew, it should appear to the left of the W, since
that is "after" it in Hebrew terms, but if it is English, it should appear to
the right of the whole Hebrew section. So when the language changes to English
the caret needs to jump to the right of the H.

Without the early return the caret will blink off and on once in the old
position and then reappear in the new position, which makes the program seem
unresponsive.

In the patch I've added a call to |mRendContext->PopState| before returning,
plus a comment which (I hope) clarifies what's going on.

Comment 3

[Frank Tang]

reassign to [e-mail]

Comment 4

[Frank Tang]

need code review

Comment 5

[Frank Tang]

ask  review from [e-mail] 2001-05-08 02:51

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

r=dbaron ... although I wonder if this logic really belongs somewhere else
(perhaps outside of the drawing functions entirely, or perhaps just earlier in
|DrawCaret|.)

Comment 7

[Simon Montagu :smontagu]

dbaron: I had the same thought as you, and tried moving this earlier, but it
didn't work. If we leave |DrawCaret| before mCaretRect gets updated, it leaves a
ghost caret in the old position.

Comment 8

[Frank Tang]

need sr 

Comment 9

[Frank Tang]

kin -can you sr this one ?

Comment 10

[kinmoz]

sr=[e-mail]

Comment 11

[Frank Tang]

fixed and check in

Comment 12

[sujay]

Frank can you verify this and mark verified-fixed? thanks...

Comment 13

[sujay]

marking verified.