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

RESOLVED FIXED in mozilla29

Status

()

Core
DOM: Device Interfaces
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: yxl, Assigned: yxl)

Tracking

unspecified
mozilla29
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 3 obsolete attachments)

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.
Created attachment 8350871 [details] [diff] [review]
geck patch v1

https://tbpl.mozilla.org/?tree=Try&rev=11eea4a1cf57
Attachment #8350871 - Flags: review?(janjongboom)
Created attachment 8350873 [details] [diff] [review]
gecko part v1

https://tbpl.mozilla.org/?tree=Try&rev=a4d63f088dbc
Attachment #8350871 - Attachment is obsolete: true
Attachment #8350871 - Flags: review?(janjongboom)
Attachment #8350873 - Flags: review?(janjongboom)
Created attachment 8350874 [details] [diff] [review]
gecko part v2

Sorry, I uploaded the wrong patch. This is the right one ;-)
Attachment #8350873 - Attachment is obsolete: true
Attachment #8350873 - Flags: review?(janjongboom)
Attachment #8350874 - Flags: review?(janjongboom)
Created attachment 8350879 [details] [review]
gaia part
Attachment #8350879 - Flags: review?(dflanagan)
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...
Flags: needinfo?(xyuan)
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 ;-)
Flags: needinfo?(xyuan)
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
Flags: needinfo?(xyuan)
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?
Attachment #8350874 - Flags: review?(janjongboom) → review+
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!
Attachment #8350879 - Flags: review?(dflanagan) → review+
(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.
(Assignee)

Updated

4 years ago
Flags: needinfo?(xyuan)
Keywords: checkin-needed
(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".
https://hg.mozilla.org/integration/b2g-inbound/rev/eccb5de0533a

The Gaia PR needs updating before it can be merged.
Flags: in-testsuite+
Keywords: checkin-needed
Created attachment 8355564 [details] [review]
gaia part v2
Attachment #8350879 - Attachment is obsolete: true
Attachment #8355564 - Flags: review+
Master: https://github.com/mozilla-b2g/gaia/commit/7a0e960c5a208ea6135f4449644e281a8d94f2f4
https://hg.mozilla.org/mozilla-central/rev/eccb5de0533a
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Jan & Ryan, thanks for the help :)

Updated

4 years ago
Depends on: 978918
You need to log in before you can comment on or make changes to this bug.