1194763 - [M(4)] Permanent (win10) TEST-UNEXPECTED-FAIL | editor/libeditor/tests/test_bug332636.html | The backspace key should delete the UTF-16 surrogate pair correctly - got "<unicode>b", expected "ab"

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect)

Description

[Justin Wood (:Callek)]

On win10, permanent:

TEST-UNEXPECTED-FAIL | editor/libeditor/tests/test_bug332636.html | The backspace key should delete the UTF-16 surrogate pair correctly - got "

Comment 1

[Justin Wood (:Callek)]

Damnit BMO, truncated my text on me, repeated here with the unicode replaced with <unicode> because of that:

On win10, permanent:

TEST-UNEXPECTED-FAIL | editor/libeditor/tests/test_bug332636.html | The backspace key should delete the UTF-16 surrogate pair correctly - got "<unicode>b", expected "ab"

with log:


 21:12:19 INFO - 1213 INFO TEST-START | editor/libeditor/tests/test_bug332636.html
21:12:20 INFO - TEST-INFO | started process screenshot
21:12:20 INFO - TEST-INFO | screenshot: exit 0
21:12:20 INFO - 1214 INFO TEST-PASS | editor/libeditor/tests/test_bug332636.html | The backspace key should delete the UTF-16 surrogate pair correctly
21:12:20 INFO - 1215 INFO TEST-PASS | editor/libeditor/tests/test_bug332636.html | The backspace key should delete the UTF-16 surrogate pair correctly
21:12:20 INFO - 1216 INFO TEST-PASS | editor/libeditor/tests/test_bug332636.html | The backspace key should delete the UTF-16 surrogate pair correctly
21:12:20 INFO - 1217 INFO TEST-PASS | editor/libeditor/tests/test_bug332636.html | The backspace key should delete the UTF-16 surrogate pair correctly
21:12:20 INFO - 1218 INFO TEST-PASS | editor/libeditor/tests/test_bug332636.html | The backspace key should delete the UTF-16 surrogate pair correctly
21:12:20 INFO - 1219 INFO TEST-PASS | editor/libeditor/tests/test_bug332636.html | The backspace key should delete the UTF-16 surrogate pair correctly
21:12:20 INFO - 1220 INFO TEST-PASS | editor/libeditor/tests/test_bug332636.html | The backspace key should delete the UTF-16 surrogate pair correctly
21:12:20 INFO - 1221 INFO TEST-UNEXPECTED-FAIL | editor/libeditor/tests/test_bug332636.html | The backspace key should delete the UTF-16 surrogate pair correctly - got "<unicode>b", expected "ab"
21:12:20 INFO - SimpleTest.is@SimpleTest/SimpleTest.js:277:5
21:12:20 INFO - testWithMove@editor/libeditor/tests/test_bug332636.html:53:3
21:12:20 INFO - runTest@editor/libeditor/tests/test_bug332636.html:67:1
21:12:20 INFO - SimpleTest._newCallStack/rval@SimpleTest/SimpleTest.js:144:17
21:12:20 INFO - EventHandlerNonNull*this.addLoadEvent@SimpleTest/SimpleTest.js:169:13
21:12:20 INFO - @SimpleTest/SimpleTest.js:1294:5
21:12:20 INFO - MEMORY STAT | vsize 766MB | vsizeMaxContiguous 130027993MB | residentFast 269MB | heapAllocated 77MB
21:12:20 INFO - 1222 INFO TEST-OK | editor/libeditor/tests/test_bug332636.html | took 332ms 

( https://treeherder.mozilla.org/logviewer.html#?job_id=10401278&repo=try )

This is referring to the test for:

<div id="edit3b" contenteditable="true">a&#x10a0f;b</div><!-- plane 1 diacritic -->

...has no skip's defined http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/tests/mochitest.ini#25

Comment 2

[Justin Wood (:Callek)]

Ehsan, Jim, Masayuki,

Ben suggested I contact you 3 to see if one of you can tackle this.

My personal desire is "Make this test green", however that happens. Of course I note that ben (understandably) states [paraphrased] we shouldn't skip this test if its a user-visible-failure.

This is one of the few mochitest failures we see on win 10 (I'm going to be filing the rest soon)

Comment 3

[(no longer active)]

I don't have a Win19 machine at the moment, and don't really have time to look into this.  Sorry.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: screenshot of the testcase

testcase: http://jsfiddle.net/d_toybox/62g32gdv/

See this screenshot. On Win10, the accent character is separated from "a". I think that this is gfx issue. (So, the caret is moved from left of "a" to right of "a", then, backspace key is pressed.)

Comment 5

[Justin Wood (:Callek)]

:masayuki,

So on my local w10 VM, (out of the box, no "extra" fonts installed) It shows

"ab" where b has two dots above it.

Pressing "right" moves the cursor from left of a to right of a
"right" again doesn't visually move the cursor
"right" yet again moves it to "right" of b

backspace, at this point deletes "b" but leaves the two dots and moves cursor back next to a
again removes the two dots and doesn't move the cursor

If I reload and then press right twice and backspace, the two dots go away but 'ab' is left.

Can you articulate why you think this is related to gfx and/or recommend someone from graphics to investigate.

Can you also articulate how widespread of a user-problem you expect this to be, I admittedly have no idea what form of unicode this relates to, or how widespread that unicode use is in any form of editable area.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Justin Wood (:Callek) from comment #5)
> :masayuki,
> 
> So on my local w10 VM, (out of the box, no "extra" fonts installed) It shows
> 
> "ab" where b has two dots above it.

Wow, then, the test needs to set font-family explicitly... The default font of <input> now depends on the version of Windows due to the fix of bug 1123654.

> Pressing "right" moves the cursor from left of a to right of a
> "right" again doesn't visually move the cursor
> "right" yet again moves it to "right" of b

The test is:

> 42 function testWithMove(edit, offset) {
> 43   edit.focus();
> 44   var sel = window.getSelection();
> 45   sel.collapse(edit.childNodes[0], 0);
> 46   var i;
> 47   for (i = 0; i < offset; ++i) {
> 48     synthesizeKey("VK_RIGHT", {});
> 49     synthesizeKey("VK_LEFT", {});
> 50     synthesizeKey("VK_RIGHT", {});
> 51   }
> 52   synthesizeKey("VK_BACK_SPACE", {});
> 53   is(edit.textContent, "ab", "The backspace key should delete the UTF-16 surrogate pair correctly");
> 54 }

> 67   testWithMove(document.getElementById("edit3b"), 1);

So, the for loop is performed only once. Therefore, in my environment, the pressing right key moves the caret as: left of "a" -> right of "a" -> right of the two dots -> right of the "b".

> backspace, at this point deletes "b" but leaves the two dots and moves
> cursor back next to a
> again removes the two dots and doesn't move the cursor

The later behavior must be tested by this. However, the first cursor move must be unexpected behavior for the author of the test.

> If I reload and then press right twice and backspace, the two dots go away
> but 'ab' is left.
> 
> Can you articulate why you think this is related to gfx and/or recommend
> someone from graphics to investigate.

Caret move and deleted characters by backspace key are managed by gfx. (bug 658881)

> Can you also articulate how widespread of a user-problem you expect this to
> be, I admittedly have no idea what form of unicode this relates to, or how
> widespread that unicode use is in any form of editable area.

I'm not sure because I don't know how many users/websites uses Unicode combining character (like this test).

Comment 7

[Jonathan Kew [:jfkthame]]

(In reply to Justin Wood (:Callek) from comment #5)
> :masayuki,
> 
> So on my local w10 VM, (out of the box, no "extra" fonts installed) It shows
> 
> "ab" where b has two dots above it.

That sounds OK. (The positioning of the two dots is poor, but not really important to the editing behavior that's being tested here.)

> 
> Pressing "right" moves the cursor from left of a to right of a
> "right" again doesn't visually move the cursor

Ah, that's a problem. One "right" should put the cursor after the U+10a0f character, as the sequence <a, U+10a0f> should be treated as a single cluster for cursor-movement purposes. (Although given the poor rendering you're seeing it might be difficult to tell visually.) But two "right"s should move it beyond the "b"; the fact that it doesn't visually move indicates that it is stepping (logically) over the diacritic at that point, so the first "right" only stepped over the "a".

This seems to be affected by the fact that Win10 ships with a font (Segoe UI Historic) that supports the Kharoshthi script, whereas previously we'd have been getting a missing-glyph hexbox for the U+10a0f character (the "two dots" mark you're seeing), and the cursor moved over the "a<hexbox>" as a unit. I suspect that if you temporarily disable Segoe UI Historic in Windows, the test will then pass.

AFAICT this only happens when the combining-mark codepoint in the middle is a supplementary-plane character. I'm not sure offhand whether this is necessarily a gfx/textrun issue (something about the flags we set on the CharacterGlyph records), or a bug in the selection or editor code, which IIRC has to do some special handling for surrogate pairs.

Comment 8

[Justin Wood (:Callek)]

from #gfx

10:32 AM <Callek_cloud> vlad: given masayuki and jfkthame's comments on Bug 1194763 -- can you recommend anyone to tell me if there is a GFX issue underlying here (and/or if the test is just bogus)
10:32 AM <firebot> https://bugzil.la/1194763 — NEW, [e-mail] — [M(4)] Permanent (win10) TEST-UNEXPECTED-FAIL | editor/libeditor/tests/test_bug332636.html | The bac
10:33 AM <Callek_cloud> vlad: I'm basically trying to get a tl;dr on "is this a real user-facing issue" and "who should make that call and fix if necessary"
10:45 AM <milan> Callek_cloud: I'd say jfkthame is as good a person as any to make this call, and we can move the bug to Gfx:Text while we're sorting this out

So.. Jonathan,

What is the resulting impact here, and can you please help determine which area of ownership this issue belongs in?

Comment 9

[Jonathan Kew [:jfkthame]]

The user-facing impact is extremely minor. It can affect whether cursor movement steps over a <base character, combining diacritic> pair as a single unit, or can leave the cursor in between the base and diacritic. Most users will never actually try to edit such text anyway.

Furthermore, AFAICT this only affects cases with supplementary-plane Unicode characters which belong to rare/historical scripts, not to basic-plane characters used in common everyday scripts. So the number of people who encounter the issue will be minuscule; and even for the handful who do, the effect will be so minor they might not notice it.

As for who should own the bug, it's not clear to me without some deeper digging. It could be gfx:text, or it could be layout:selection, or it could be editor. IIRC the behavior here depends on all these pieces cooperating properly.

There's a genuine bug at some level, which we should fix. But if we have to mark this test as failing on Win10 for the time being, in order to enable test coverage in general, I think that's fine. It doesn't indicate a problem that's going to impact users to any significant degree.

Comment 10

[Jonathan Kew [:jfkthame]]

Attachment: Ensure non-cluster-start flag is set properly for a run-initial supplementary-plane combining mark when shaping text

Comment 11

[Jonathan Kew [:jfkthame]]

Yep, it's a gfx:text bug all right. But the edge case it affects will be vanishingly rare in practice, so not a huge concern.

Anyway, here's a patch that should fix it (comment 10). Tested on OS X with appropriate fonts installed locally to cause the failure to show up. Justin or Masayuki, if you're in a position to verify that this fixes the test failure on Win10, that'd be great - thanks.

Comment 12

[Jonathan Kew [:jfkthame]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ffb9e69cbcb3

Comment 13

[Justin Wood (:Callek)]

(In reply to Jonathan Kew (:jfkthame) from comment #12)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=ffb9e69cbcb3

As soon as the w64 job finishes (I just bumped its priority) I can actually run this against our win 10 machine on try. I'll kick that off as soon as the build completes.

Comment 14

[Justin Wood (:Callek)]

(In reply to Justin Wood (:Callek) from comment #13)
> (In reply to Jonathan Kew (:jfkthame) from comment #12)
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=ffb9e69cbcb3
> 
> As soon as the w64 job finishes (I just bumped its priority) I can actually
> run this against our win 10 machine on try. I'll kick that off as soon as
> the build completes.

And to close the loop, the try push passed on Opt M(4) with this

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ffb9e69cbcb3

Comment 15

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9e4c187a558c40bc0b66eb7c10174247195ec91c
Bug 1194763 - Ensure non-cluster-start flag is set properly for a run-initial supplementary-plane combining mark when shaping text. r=jdaggett

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9e4c187a558c