952724 - [InputMethid API] InputContext#replaceSurroundingText doesn't match spec

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Yuan Xulei [:yxl] (Leave from May 18)]

The spec:

Promise<void> replaceSurroundingText(DOMString text, optional long offset, optional long length);

The webidl is the same with the spec:

http://mxr.mozilla.org/mozilla-central/source/dom/webidl/InputMethod.webidl#124

Promise replaceSurroundingText(DOMString text, optional long offset, optional long length);

The last two parameters give the offset from the cursor position where replacing starts and the length of text to replace.

But we made a mistake with the implementation:

Promise<void> replaceSurroundingText(DOMString text, optional long beforeLength, optional long afterLength);

The last two parameters give the text lengths before and after the cursor to replace.

Comment 1

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: geck patch v1

https://tbpl.mozilla.org/?tree=Try&rev=11eea4a1cf57

Comment 2

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: gecko part v1

https://tbpl.mozilla.org/?tree=Try&rev=a4d63f088dbc

Comment 3

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: gecko part v2

Sorry, I uploaded the wrong patch. This is the right one ;-)

Comment 4

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: gaia part

Comment 5

[Jan Jongboom [:janjongboom] (inactive)]

So what I don't like here is that we're making a breaking change in the API as of v1.4 with this patch. Is there any way we can deprecate the old call and make a new function that behaves correct (via an overload or something). I guess there will be people writing keyboards for 1.2 and up and there is no way to know which version of replaceSurroundingText they'll be dealing with...

Comment 6

[Yuan Xulei [:yxl] (Leave from May 18)]

The fix should be uplifted to v1.2 as well. We'll use the same API for v1.2 and above. Our implementation doesn't follow all our public wiki, webidl and documents. It'll make keyboard developer confusing. 

Since v1.2 hasn't support 3rd party keyboard yet, and we mainly use the keyboard API for the built-in keyboard, I think we don't need to be compatible with the old call ;-)

Comment 7

[Jan Jongboom [:janjongboom] (inactive)]

Since when was 3rd party keyboard scrapped from 1.2? If this will be the only version people will ever see I'm fine with it :p

Comment 8

[Jan Jongboom [:janjongboom] (inactive)]

Comment on attachment 8350874 [details] [diff] [review]
gecko part v2

r=janjongboom.

Unrelated; why is  INFO TEST-KNOWN-FAIL | /tests/dom/inputmethod/mochitest/test_basic.html | The input context type should match. - got text, expected input failing?

Comment 9

[David Flanagan [:djf]]

Comment on attachment 8350879 [details] [review]
gaia part

Looks good to me. Note, though, that I have not compiled the gecko patch and tested myself.

Thanks for patching the test_apps/demo-keyboard too!

Comment 10

[[:fabrice] Fabrice Desré]

(In reply to Jan Jongboom [:janjongboom] from comment #7)
> Since when was 3rd party keyboard scrapped from 1.2? If this will be the
> only version people will ever see I'm fine with it :p

3rd party keyboards didn't make 1.3 either, they'll start in 1.4. No need to backport anywhere please.

Comment 11

[Yuan Xulei [:yxl] (Leave from May 18)]

(In reply to Jan Jongboom [:janjongboom] from comment #8)
> Comment on attachment 8350874 [details] [diff] [review]
> gecko part v2
> 
> r=janjongboom.
> 
> Unrelated; why is  INFO TEST-KNOWN-FAIL |
> /tests/dom/inputmethod/mochitest/test_basic.html | The input context type
> should match. - got text, expected input failing?

From the webild, InputContext#type should be "input", "textarea", or "contenteditable", but we get "text".

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/eccb5de0533a

The Gaia PR needs updating before it can be merged.

Comment 13

[Jan Jongboom [:janjongboom] (inactive)]

Attachment: gaia part v2

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Master: https://github.com/mozilla-b2g/gaia/commit/7a0e960c5a208ea6135f4449644e281a8d94f2f4

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/eccb5de0533a

Comment 16

[Yuan Xulei [:yxl] (Leave from May 18)]

Jan & Ryan, thanks for the help :)