Bug 20092 – Spelling markers positioned incorrectly in RTL text

• Status: RESOLVED
• Resolution: FIXED
• Component: Text

[Uri Bernstein]

In <textarea> and <input> elements, when "Check Spelling While Typing" is enabled, the red dotted underlines indicating misspelled words are incorrectly horizontally positioned for RTL (Hebrew, Arabic, etc.) mis-spelled words.

To reproduce, download and install a Hebrew spell checking service (e.g. from http://mitzpettel.com/software/hspell.php), enable the service, enable "Check Spelling While Typing", and type some misspelled Hebrew words into a text input or textarea.

[Jeremy Moskovich]

Created attachment 23676 [details]
Partial fix, still needs work...

I'm just attaching this as a starting point for discussion.  This patch works for single languages (see attached screenshot).

[Jeremy Moskovich]

Created attachment 23677 [details]
Results with initial patch.

You can see that using one language works, mixing 2 still doesn't work properly.

[Jeremy Moskovich]

Created attachment 23682 [details]
Patch for review

[Jeremy Moskovich]

Created attachment 23683 [details]
Results with patch.

Mixed languages now work.

[Darin Adler]

Comment on attachment 23682 [details]
Patch for review

A patch needs to include a test case and a ChangeLog entry. This doesn't have either. Dan Bernstein can help you make a "pixel test" for this, or you could construct a manual test case for the manual-tests directory.

 659     int startPosition = max(marker.startOffset - m_start, (unsigned)0);

This is wrong. Since both expressions are of type "unsigned", then it's going to be an unsigned "max", so it will always be startOffset - start, even if it's a negative number. Instead of casting the right side to unsigned we need to cast the left side to int.

I'd like to see test cases that exercise this "max" statement. Or we can leave the max out if it's not really needed -- if we can't create a test case that is affected then maybe it's not.

 660     int endPosition = min(marker.endOffset - m_start, (unsigned)m_len);

 677         int grammarEndPosition = min(marker.endOffset - m_start, (unsigned)m_len);

Might be the same problem here. We don't want a negative number to turn into m_len, so we need to do an int version of min instead of unsigned.

It occurs to me that another way to accomplish this is to call min<int>(a, b) instead of casting a or b to an unsigned.

 662     if (m_truncation != cNoTruncation) {
 663         endPosition = min(endPosition, startPosition + m_truncation);
 664     }

Out style guide doesn't use braces around the bodies of single line if statements.

review- because of the issues above

I made another couple of comments about the patch on IRC. Some notes about the regression test: spelling markers will only show under a misspelled word in editable HTML after the insertion point visited it, so you will probably need to create a editable HTML element with misspelled words and make sure to place the caret inside every word you want the marker to draw under. In the default configuration, DumpRenderTree uses the built-in English spelling dictionary, which ignores the right-to-left scripts, but you can use Latin text with the style "direction: rtl; unicode-bidi: bidi-override;" to get misspelled, right-to-left text.

[Jeremy Moskovich]

Created attachment 23719 [details]
Updated patch w/testcase

This should address all previous comments:
* Added testcase & ChangeLog entry.
* Changed min,max to <int> versions.
* Added back "full width" optimization.

Comment on attachment 23719 [details]
Updated patch w/testcase

The WebCore/ChangeLog entry should include a link to the bug and its title, and preferably a description of the code changes in the function.

I am not sure the proliferation of local variables in this function is helpful. One boolean saying whether the marker spans the entire box seems to be enough. "paintStart" and "paintEnd" are now used only once each, so I would consider getting rid of them. "Recalculate" is not a good verb to use because this is not replacing the result of an earlier calculation.

+            IntPoint grammarStartPoint = IntPoint(m_x + tx, y + ty);

I know this is code you are just moving, but it is buggy. Adding m_x again here is wrong. You can see it in this example:

data:text/html,%3Cdiv%20contenteditable%3EThis%20is%20%20is%20wrong. %20%3C/div%3E

(note the two spaces between "is" and "is"). If you enable English grammar checking and click in the sentence and then after the sentence, the "tool tip" for the second "is" will appear when you hover the letter g instead of when you hover the underlined word. You do not have to fix this bug in this patch. You could just add a FIXME. Is it important not to apply truncation for the grammar marker? If not, you could use the same selection rectangle for both spelling (when it is needed) and grammar.

+            IntRect grammerMarkerRect = enclosingIntRect(f->selectionRectForText(run, grammarStartPoint, selectionHeight(), startPosition, endPosition));

Typo: grammer.

The LayoutTests/ChangeLog entry should also include a link to the bug and the bug title. Your patch should include the expected test results (use run-webkit-tests --pixel to generate pixel results), and the change log should reflect that.

The test contains tabs so it cannot be landed. Please use spaces.

+	// Remove focus from the element, since the word under the curser won't have a misspelling marker.

Typo: curser

+This tests the correct placement of inline spelling and grammer markers in text.<br>

Typo: grammer.

Name: svn:executable
   + *

I do not think the above is necessary.

[Jeremy Moskovich]

Created attachment 23761 [details]
Updated patch.

* Added bug URL to changelog.
* Added expected results for pixel test
* Replaced a bunch of temp variables with one markerSpansWholeBox flag.
* Cleaned up typos and tabs.
* Removed svn:executable line from patch.
* Fixed existing m_x + tx bug that mitz pointed out, "tool tip"s now match up with grammar markers.

Note: For some reason the grammar markers aren't drawn in the pixel test image (?)

Comment on attachment 23761 [details]
Updated patch.

Looks great! I did notice a couple of issues, though, now that the code is cleaned up:

+            endPosition = min(endPosition, startPosition + m_truncation);        

That looks wrong, because m_truncation is relative to m_start, not startPosition.

+        IntPoint startPoint = IntPoint(tx, ty);

The current WebKit style for writing this is:
IntPoint startPoint(tx, ty);

However, the y coordinate is wrong. Currenrly, it is ty + selectionTop(), which is also wrong since earlier m_y is added to ty. This means that grammar "tool tips" only work in the first line of text in a block. What you really want there is the passed-in ty + selectionTop().

-    
+        

Extra white space.

Sorry about not catching the above earlier. I am going to r- because the endPosition issue looks like a regression.

[Jeremy Moskovich]

Created attachment 23769 [details]
Updated patch.

* Fixed regressions, and reworked tx & ty calcuation a bit.
* Changed startPoint declaration to WebKit style.

Comment on attachment 23769 [details]
Updated patch.

r=me with one change we discussed that I will make when landing the patch. Thank you!

Fixed in <http://trac.webkit.org/changeset/36874>.