349352 - Underline shifts when clicking the spacebar after an unrecognized Hebrew word on RTL textarea

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Niro]

Comment 1

[Niro]

Grr. forgot the description :)
On RTL textarea, try to write a misspelled Hebrew word and then click the spacebar.
result: the red underline shifts to the the left after any spacebar clicking.

Comment 2

[Simon Montagu :smontagu]

Same bug on Linux, but only with --enable-pango. Setting All | All

Comment 3

[Brett Wilson]

Can you please post a testcase? I don't know hebrew at all, or any good sites. If possible, can you also post some text I can paste in with exact steps to reproduce?

Comment 4

[Simon Montagu :smontagu]

I'm on this. The problem is at http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/generic/nsTextFrame.cpp&rev=1.587&mark=2588-2591#2576 which doesn't allow for Bidi.

Comment 5

[Brett Wilson]

Awesome, I would have never figured that out.

Comment 6

[Niro]

Attachment: testcase

the red underline should be under the word.

Comment 7

[Niro]

Attachment: testcase

I added charset=windows-1255 to this testcase.

Comment 8

[Simon Montagu :smontagu]

Attachment: Patch

Comment 9

[Uri Bernstein]

Comment on attachment 234811 [details] [diff] [review]
Patch

Looks good. r=me.

You might want to fix the annoying indentation problem (starting with |if (!hideStandardSelection || displaySelection)|) while you're there.

Comment 10

[Boris Zbarsky [:bzbarsky]]

I'd like to see some documentation for the new args before I can reasonably sr this...

Comment 11

[Simon Montagu :smontagu]

Attachment: Indentation fixed and documentation added

Comment 12

[Simon Montagu :smontagu]

Attachment: Same, with diff -w

Comment 13

[Simon Montagu :smontagu]

Comment on attachment 235197 [details] [diff] [review]
Indentation fixed and documentation added

Removing review request for now. I had an idea to simplify this and other things.

Comment 14

[Simon Montagu :smontagu]

Comment on attachment 235197 [details] [diff] [review]
Indentation fixed and documentation added

re-requesting sr. I will do the simplification later after the TextFrame rewrite.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 235197 [details] [diff] [review]
Indentation fixed and documentation added

>Index: layout/generic/nsTextFrame.cpp
>+   * @param aSelectionIsMirrored whether the offsets of the selection have been

So wouldn't it make more sense to call this aSelectionPointsReversed or even aSelectionCoordsReversed?  At least those names seem to make more sense to me...  If you're ok with that, make that change throught the patch?  If not, catch me and let's talk about naming this stuff?  I'm not familiar with standard bidi terms, so maybe "mirrored" is a technical term here?

>   void RenderString(nsIRenderingContext& aRenderingContext,

Same here, and this probably wants a cop of the comment on PaintTextDecorations.

>+  /**
>+   * @return whether the selection points were mirrored

And here, "reversed".

> nsTextFrame::PaintTextDecorations(nsIRenderingContext& aRenderingContext,

>+               * XXX In theory this should use the same logic as
>+               *     SELECTION_SPELLCHECK for right-to-left frames, but I don't
>+               *     know of any right-to-left language that uses an IME

Would handling this really make the code that much more complex?  If so, can we at least assert that whatever situation we're not handling is not happening? If not, can we do the right thing here?

>@@ -3003,128 +3038,147 @@ nsTextFrame::PaintUnicodeText(nsPresCont
>+        selectionIsMirrored = AdjustSelectionPointsForBidi(sdptr, textLength,
>+                                    CHARTYPE_IS_RTL(charType), 
>+                                    isOddLevel, isBidiSystem);

I'd prefer a linebreak after the '=' sign and more natural indenting of the arguments (lining up with the arguments right after the '(' ).

>+            PRBool     isSelection = iter.GetSelectionColors(
>+                                       &currentFGColor,
>+                                       &currentBKColor,
>+                                       &isCurrentBKColorTransparent);

How about nixing the extra spaces after the "PRBool" instead?  If things still don't fit in 80 chars, please put the linebreak after the '='.

By the way, check with roc before landing this reindentation?  If it's going to make life hell for him, please don't do it....

>@@ -3684,35 +3741,36 @@ nsTextFrame::PaintTextSlowly(nsPresConte
>+        selectionIsMirrored = AdjustSelectionPointsForBidi(sdptr, textLength,

Again, linebreak after the '='.

Comment 16

[Simon Montagu :smontagu]

Attachment: Simpler patch

I apologize for wasting reviewers' time. I was looking for a scenario where I could test IMEs, and I realized with Uri's help that the patch went through some unnecessary hoops. The condition 
  if ((NS_GET_EMBEDDING_LEVEL(this) & 1) != aSelectionIsMirrored)
is equivalent to 
  if ((NS_GET_EMBEDDING_LEVEL(this) & 1) != (isOddLevel ^ (isRTLChars && isBidiSystem))
but (NS_GET_EMBEDDING_LEVEL(this) & 1) and (isOddLevel) are the same thing, so it reduces to just
  if (isRTLChars && isBidiSystem)
and we already have a boolean for that, isRTLOnBidiPlatform.

Comment 17

[Uri Bernstein]

Comment on attachment 237956 [details] [diff] [review]
Simpler patch

Seems fine.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 237956 [details] [diff] [review]
Simpler patch

>Index: layout/generic/nsTextFrame.cpp
>   void PaintTextDecorations(nsIRenderingContext& aRenderingContext,

So... I should have caught this before, but could we make this arg NOT have a default value?  Put it before aText or something?  That way we can be sure we got all the callers, etc...

>   void RenderString(nsIRenderingContext& aRenderingContext,

Same here.

>@@ -2560,33 +2571,53 @@ nsTextFrame::PaintTextDecorations(nsIRen
>+               * If the rendering context is drawing text from right-to-left,

"from right to left" (no dashes).

sr=bzbarsky with those nits.

Comment 19

[Simon Montagu :smontagu]

Attachment: Branch patch

Requesting 1.8.1 approval. This bug makes spellchecking almost unusable for right-to-left languages. The fix is very low risk, and only changes behaviour for the underline marking for right-to-left text.

(To make assurance doubly sure, when backporting to the branch I removed the code for IME selection, since it would need to be rewritten for the branch and I have no way of testing it anyway, and replaced it by an assertion as bz suggested)

Comment 20

[Mike Schroepfer]

Comment on attachment 238195 [details] [diff] [review]
Branch patch

a=schrep

Comment 21

[(no longer active)]

Mass-assigning the new rtl keyword to RTL-related (see bug 349193).