User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:32.0) Gecko/20100101 Firefox/32.0 Build ID: 20140825202822 Steps to reproduce: Writing SMS with use of Messages App. Actual results: There are huge lags (up to 1-2s) when word must be suggested/corrected. Almost no lags when writing with disabled word suggestion and correction. Expected results: It should work in more fluent way. Found on: Alcatel One Touch Fire production (got from T-mobile Poland) B2G version: 2.2.0.0-prerelease master Platform version: 35.0a1 Build Identifier: 20140911064110 Git commit info: 2014-09-11 06:54:27 e3b9d0d6

I do feel this on my Nexus S. Let's see what I get on Flame.

I clearly feel it on Flame. STR: 0. Start typing a message in SMS app 1. Make sure you: - do mistakes so that it triggers autocorrection - make a long message Expected: There should be no latency when autocorrection replaces a word. Actual: We feel a slight latency when autocorrection does its replacement. The longer the SMS we are writing is, the bigger the latency. Reproducing on a Flame with Gecko/Gaia master from yesterday.

Such a bad perf regression makes it very hard to use the device at some point.

Can we get QA to confirm and provide a regression window? From my testing on my Flame, it starts to be really problematic with ~20 words in the SMS I'm writing.

This really looks related directly to the amount of data in the current text input: if I stop typing my SMS, close the app, restart b2g, and goes back into my draft, the latency is as bad as it was previously.

I tried profiling, but I could not get ANYTHING useful: http://people.mozilla.org/~bgirard/cleopatra/#report=dc8d6f7ede5630cbd43d9fc286c578867401e6c2 > alex@portable-alex:~/codaz/Mozilla/b2g/devices/Flame/B2G$ ./profile.sh start -p b2g -t Compositor,GeckoMain,InputReader,Gecko_IOThread,Binder_1,Binder_2 -i 1 > Process: b2g > Threads: Compositor,GeckoMain,InputReader,Gecko_IOThread,Binder_1,Binder_2 > Interval: 1 > Using default features js,leaf,threads > Starting profiling PID 3376.. > Profiler started > > alex@portable-alex:~/codaz/Mozilla/b2g/devices/Flame/B2G$ ./profile.sh start -p 3730 -t Compositor,GeckoMain,InputReader,Gecko_IOThread,Binder_1,Binder_2 -i 1 > Process: 3730 > Threads: Compositor,GeckoMain,InputReader,Gecko_IOThread,Binder_1,Binder_2 > Interval: 1 > Using default features js,leaf,threads > Starting profiling PID 3730.. > Profiler started

Issue occurs when trying to write in SMS App or Notes+ app (taken from Marketplace). But when trying to write in the Browser on an opened web page it works fluently, i.e. tested on: http://m.telepolis.pl/wiadomosci/zwyciezcy-konkursu-z-iii-dnia,2,3,31498.html go to the bottom of the page and try to write something longer in the vary last text box, called 'Tresc' (with making mistakes to allow word suggestion and correction to correct words).

This issue occurs on 2.2 KK Flame and 2.2 Flame on the v123 JB base. Environmental Variables: Device: Flame 2.2 BuildID: 20140915091417 Gaia: 855be6ade407c26e0596e7306a44deebc3f60933 Gecko: d08c31df0c1a Version: 35.0a1 (2.2) Firmware Version: v165 User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0 Environmental Variables: Device: Flame 2.2 BuildID: 20140915091417 Gaia: 855be6ade407c26e0596e7306a44deebc3f60933 Gecko: d08c31df0c1a Version: 35.0a1 (2.2) Firmware Version: v123 User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0 This issue does not occur on 2.1 KK Flame, 2.0 KK Flame, or the 1.4 Flame KK v165 base. Environmental Variables: Device: Flame 2.1 BuildID: 20140915073605 Gaia: 944e5b4582c4efa1b67cd33245dbb8f6aa25d09f Gecko: 2697f51cfe95 Version: 34.0a2 (2.1) Firmware Version: v165 User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0 Environmental Variables: Device: Flame 2.0 BuildID: 20140914162307 Gaia: 7edd3b0b9f65c3dde235c732d270e43e055a1254 Gecko: 13e04ab68621 Version: 32.0 (2.0) Firmware Version: v165 User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Profile: https://people.mozilla.org/~bgirard/cleopatra/#report=c5cf00287fb9f84b9a27a35f4afba292373ee6db replaceSurroundingText() spends a lot of time in setSelectionRange() for moving the caret to the start position from beginning as it is a div, not a textfield. replaceSurroundingText() should be able to be improved, I'll give it a try.

I will improve setSelectionRange() instead, but I am not sure why it needs two loops to move caret forward...

CC yxl as he knows the code.

I was thinking to move caret either from beginning forward or from the end backward, depends on start position near to which end. But a better fix should set the caret to start position directly instead of a loop moving it forward/backward. BTW, I found Selection::Collapse() is not implemented as API document on MDN: offset 0 - Collapses the selection from the anchor to the beginning of parentNode's text. 1 - Collapses the selection from the anchor to the end of parentNode's text. which selection.collapse(element, 1); will set range start/end 1, I am not sure is this a documentation error or implementation error.

(In reply to Ting-Yu Chou [:ting] from comment #10) > I will improve setSelectionRange() instead, but I am not sure why it needs > two loops to move caret forward... The APIs related to selection range management are tricky and buggy. I had to set the selection range by moving caret instead of setting position directly. The API to move the caret position may fail sometimes. To ensure the caret is moved to the expected position, we use an extra loop. Here is an example: http://jsfiddle.net/7oL5trk8/1/ 1. set the caret position at the beginning of the content editable 2. move the caret to the end by pressing right arrow key repeatedly until reaching the <img>. expected: press right arrow key will move the caret across the <img> actual: the caret is blocked by the <img> and cannot be moved.

Bug 1029943 seems like the cause here. B2g-inbound Regression Window Last working Environmental Variables: Device: Flame 2.2 BuildID: 20140905062812 Gaia: 04bfb7cab6a5485b650dac75cf99b8509ae148fc Gecko: 6aa95efb9322 Version: 35.0a1 (2.2) Firmware Version: v165 User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0 First Broken Environmental Variables: Device: Flame 2.2 BuildID: 20140905063113 Gaia: 04bfb7cab6a5485b650dac75cf99b8509ae148fc Gecko: 0bb6b880744e Version: 35.0a1 (2.2) Firmware Version: v165 User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0 Last working gaia / First broken gecko - Issue DOES occur Gaia: 04bfb7cab6a5485b650dac75cf99b8509ae148fc Gecko: 0bb6b880744e First broken gaia / Last working gecko - Issue does NOT occur Gaia: 04bfb7cab6a5485b650dac75cf99b8509ae148fc Gecko: 6aa95efb9322 Gecko Pushlog: http://hg.mozilla.org/integration/b2g-inbound/pushloghtml?fromchange=6aa95efb9322&tochange=0bb6b880744e

broken by Bug 1029943 ? Can you take a look Morris?

(In reply to Yuan Xulei [:yxl] from comment #13) > The APIs related to selection range management are tricky and buggy. I had > to set the selection range by moving caret instead of setting position > directly. > > The API to move the caret position may fail sometimes. To ensure the caret > is moved to the expected position, we use an extra loop. Does this mean current implementation is the best we can do, unless we fix the buggy code? Is there a bug for the "fail sometimes"?

It seems the best we can do now before we have an API for content editable that is similar to the setSelectionRange of the <input> element. We don't have a bug for the selection range related bugs. Feel free to file bugs for that, I you have resouces to investigate into it.

I understand the status, but we cannot leave this in such a bad situation: typing SMS is nearly not possible even on a Flame with full specs.

This is unfortunate because bug 1029943 is a much needed feature! Ethan, would you be able to give us more guidance on this bug? It's sad that we have to regress the performance to get a feature done.

I am working on a patch now, which tries to add a range to the selection directly in setSelectionRange().

(In reply to Alexandre LISSY :gerard-majax from comment #18) > I understand the status, but we cannot leave this in such a bad situation: > typing SMS is nearly not possible even on a Flame with full specs. Let's find the cause of the regression and try to solve it.

Still buggy, but I'd like to upload the wip so people may find I am doing something wrong before I realize it.

Why don't we backout bug 1029943 then? We want a stable, usable nightly, not dragging regressions like that.

(In reply to Ting-Yu Chou [:ting] from comment #9) > Profile: > https://people.mozilla.org/~bgirard/cleopatra/ > #report=c5cf00287fb9f84b9a27a35f4afba292373ee6db > > replaceSurroundingText() spends a lot of time in setSelectionRange() for > moving the caret to the start position from beginning as it is a div, not a > textfield. > > replaceSurroundingText() should be able to be improved, I'll give it a try. Which setSelectionRange() is that? I can't find one in this profile. (In reply to Tim Guan-tin Chien [:timdream] (OOO ~Sep 19) (MoCo-TPE) (please ni?) from comment #19) > This is unfortunate because bug 1029943 is a much needed feature! > > Ethan, would you be able to give us more guidance on this bug? It's sad that > we have to regress the performance to get a feature done. The immediate thing to do is to backout bug 1029943 while we figure out the performance issues. Also, if you guys are hitting issues with the selection API, please file bugs on them instead of just working around them. (And also, s/Ethan/Ehsan/. ;)

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!, PTO on Thu/Fri) from comment #24) > Which setSelectionRange() is that? I can't find one in this profile. > Enter setSelectionRange in the filter, https://people.mozilla.org/~bgirard/cleopatra/#report=c5cf00287fb9f84b9a27a35f4afba292373ee6db&search=selectionRange

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!, PTO on Thu/Fri) from comment #24) > (And also, s/Ethan/Ehsan/. ;) Ouch, I am sorry.

I tested the patch with my Flame, it works all right. It is based on the assumption that the contenteditable element contains only one level of children which is either a text node or a <br> element. I am not confident this assumption covers all the cases.

Comment on attachment 8491249 [details] [diff] [review] patch v1 Review of attachment 8491249 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Ting-Yu Chou [:ting] from comment #27) > Created attachment 8491249 [details] [diff] [review] > patch v1 > > I tested the patch with my Flame, it works all right. > > It is based on the assumption that the contenteditable element contains only > one level of children which is either a text node or a <br> element. I am > not confident this assumption covers all the cases. The contenteditable can contain non-text nodes, which constitute <div>, <img>, <audio>, <iframe> ... And it may have more than one level of children, such as an embedded table, a <div> with child elements. It is non-trivial to calculate the cursor position within contenteditable. You have to deal with hidden nodes and hierarchical layouts. You may need to refer to nsIDocumentEncoder's implementation on how to traverse nodes and calcuate text length within contenteditable: http://mxr.mozilla.org/mozilla-central/source/content/base/public/nsIDocumentEncoder.idl#317

(In reply to Yuan Xulei [:yxl] from comment #28) > > It is based on the assumption that the contenteditable element contains only > > one level of children which is either a text node or a <br> element. I am > > not confident this assumption covers all the cases. > The contenteditable can contain non-text nodes, which constitute <div>, > <img>, <audio>, <iframe> ... > And it may have more than one level of children, such as an embedded table, > a <div> with child elements. Since forms.js is placed under inputmethod, and only MozKeyboard.js and Keyboard.jsm are triggering setSelectionRange(), so I thought it may not need to handle things like a general contentededitable element...

(In reply to Ting-Yu Chou [:ting] from comment #29) > (In reply to Yuan Xulei [:yxl] from comment #28) > > > It is based on the assumption that the contenteditable element contains only > > > one level of children which is either a text node or a <br> element. I am > > > not confident this assumption covers all the cases. > > The contenteditable can contain non-text nodes, which constitute <div>, > > <img>, <audio>, <iframe> ... > > And it may have more than one level of children, such as an embedded table, > > a <div> with child elements. > > Since forms.js is placed under inputmethod, and only MozKeyboard.js and > Keyboard.jsm are triggering setSelectionRange(), so I thought it may not > need to handle things like a general contentededitable element... Why? forms.js is injected into all the content pages and we encounters all kinds of contenteditable elements. For you example, the message content field of the new message composing page of the gaia messages app is a contentelement element, which allows us to input audios and images as nested non text child elements. Also rich editors (such as http://www-archive.mozilla.org/editor/midasdemo/) are often implemented by complex contenteditables. Does you patch work for these cases?

Thanks for explaining me, I understand and was just explaining why I had the assumption. Another idea is to change |replaceSurroundingText| since it is to replace the text around caret, which selection.anchorNode should be the text node already. Does this sound reasonable? If it does not, probably the hard path you mentioned (nsIDocumentEncoder) can't be avoid.

I just give the idea a try, and it works for the cases in comment 30.

Comment on attachment 8491402 [details] [diff] [review] wip v2 Hmm... it does not always work if I change the caret position manually, anchorNode could be something other than a text node.

So, why don't we back out bug 1029943 until we fix this problem? Any objections?

(In reply to Ting-Yu Chou [:ting] from comment #33) > Hmm... it does not always work if I change the caret position manually, > anchorNode could be something other than a text node. I am not sure is it expected that the caret is not within a text node when word suggestion comes up...

Jan, we generally allow the engineer to make their own decision on how to fix/resolve a regression, unless it's a smoketest breakage (preventing a nightly channel user from using basic phone functionality) or a serious QA/automation/etc blocker. Ting-Yu seems to be making progress already. I am redirecting needinfo to him on this.

There's no reason not to back out bug 1029943, what should I do to make that happen?

(In reply to Ting-Yu Chou [:ting] from comment #37) > There's no reason not to back out bug 1029943, what should I do to make that > happen? Can the sheriffs take care of this?

Anyone else is welcome to perform a backout too - I'm keen for the sheriffs to not end up on the critical path for things like this (read: we're pretty busy too, and this isn't 100% our remit :-)) - though of course we will try and help if needs be. Also note due to bug 179701, needinfo'ing an alias doesn't work as expected (sadly) - just CCing is probably best.

I backed out bug 1029943 at https://hg.mozilla.org/integration/b2g-inbound/rev/ac4bfb631c1e

(In reply to Ting-Yu Chou [:ting] from comment #35) > (In reply to Ting-Yu Chou [:ting] from comment #33) > > Hmm... it does not always work if I change the caret position manually, > > anchorNode could be something other than a text node. > > I am not sure is it expected that the caret is not within a text node when > word suggestion comes up... No, non-text node will be regarded as word separator similar to white space and word suggestion won't show after word separator.

(In reply to Yuan Xulei [:yxl] from comment #41) > No, non-text node will be regarded as word separator similar to white space > and word suggestion won't show after word separator. In forms.js, the selection start/end of selectionchange event for contenteditable element are calculated from |getContentEditableSelectionStart| and |getContentEditableSelectionLength|, it doesn't consider whether the anchorNode is a text node. Is this a bug that contenteditable should make sure caret sync its calculated selection? If not to consider the problem above, does comment 31 make sense to you?

I think the performance issue here is word suggestion trigger so many selection change event and [1] would dispatch dom event while selection change event occurred so the performance downgrade. Since bug 1029943 is backed out, we'll continue discussing how to decrease overhead in bug 1029943. [1]: http://dxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp?#9317

The case WIP v2 does not work is when change caret to the end of a line, the selection.anchorNode is the contenteditable <div> (id=message-input).

(In reply to Ting-Yu Chou [:ting] from comment #44) > The case WIP v2 does not work is when change caret to the end of a line, the > selection.anchorNode is the contenteditable <div> (id=message-input). Morris just explained me why, since the end of a line can be represented as a offset of the <div>, or a offset of the text node, and seems it is always former case.

Added code to handle the case of comment 45, tested with the cases in comment 30.

(In reply to Morris Tseng [:mtseng] from comment #43) > I think the performance issue here is word suggestion trigger so many > selection change event and [1] would dispatch dom event while selection > change event occurred so the performance downgrade. Since bug 1029943 is > backed out, we'll continue discussing how to decrease overhead in bug > 1029943. > > [1]: > http://dxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow. > cpp?#9317 To be more specific, many selection change events of NO_REASON and KEYPRESS_REASON will be fired in this scenario. I've confirmed that changing line [1] to the following will fix this issue. if (gSelectionCaretPrefEnabled && mDoc && anAction.EqualsLiteral("selectionchange") && (aReason != nsISelectionListener::NO_REASON) && !(aReason & nsISelectionListener::KEYPRESS_REASON)) [1] http://hg.mozilla.org/mozilla-central/file/c8e325eee9e1/dom/base/nsGlobalWindow.cpp#l9289

Comment on attachment 8492038 [details] [diff] [review] patch v2 There are still cases the patch does not handle, for instance, when caret is in the beginning of contenteditable, when caret is among multiple empty lines. As I don't have enough background knowledge of selection, I'll stop making try&error fixing. Note Morris/TYLin are making progress on eliminating triggered callback from moving caret forward.

I just talked to Peter's team, even the callback is eliminated there's still a short pause after selecting a candidate. My current idea is: 1. In forms.js, |replaceSurroundingText| changes selection directly if the anchorNode is a text node (for the case of comment 31), 2. If it is not the case 1, fallback to |setSelectionRange| to search the node for set selection, and an implementation for content editable like Yuan suggested in comment 17 would be the best, but probably this won't be out soon. When user is entering, the caret usually stays in the last text node, I expect case 1 will be hit often. Any other ideas?

Implement comment 49. There's one thing I am not sure, if selection is not collapsed, the cursor is in anchorOffset or focusOffset? I'll try to implement the set selection range API for content editable element, but probably will take some time. If it is urgent, this patch along with callback elimination (bug 1065955) can be a band aid, or the API can be done in a follow-up bug.

(In reply to Ting-Yu Chou [:ting] from comment #50) > Created attachment 8494229 [details] [diff] [review] > part1 v1: avoid setSelectionRange when the caret is in a text node > > Implement comment 49. There's one thing I am not sure, if selection is not > collapsed, the cursor is in anchorOffset or focusOffset? In InputMethod API, we assume the cursor is the smaller one of anchorOffset and focusOffset. > > I'll try to implement the set selection range API for content editable > element, but probably will take some time. If it is urgent, this patch along > with callback elimination (bug 1065955) can be a band aid, or the API can be > done in a follow-up bug. I'd like to keep this quick fix and make a follow-up for the set selection range later.

Comment on attachment 8494229 [details] [diff] [review] part1 v1: avoid setSelectionRange when the caret is in a text node Review of attachment 8494229 [details] [diff] [review]: ----------------------------------------------------------------- Please add some tests for this. ::: dom/inputmethod/forms.js @@ +1169,5 @@ > + let start = range[0] + offset; > + if (start < 0) { > + start = 0; > + } > + let end = start + length; Check if the target selection range is the same as the current selection range. If it is, we don't need to call setSelectionRange().

Thank you for quick response. Addressed the issues, and updated comments a bit. As there're already some basic tests for replaceSurroundingText(), what else tests should I add?

Comment on attachment 8494286 [details] [diff] [review] part1 v2: avoid setSelectionRange when the caret is in a text node Review of attachment 8494286 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/inputmethod/forms.js @@ +1153,5 @@ > + let fastPathHit = false; > + if (!isPlainTextField(element)) { > + let sel = element.ownerDocument.defaultView.getSelection(); > + let node = sel.anchorNode; > + if (sel.isCollapsed && node && node.nodeType == 3 /* TEXT_NODE */) { I expect a test for this `if` case. @@ +1154,5 @@ > + if (!isPlainTextField(element)) { > + let sel = element.ownerDocument.defaultView.getSelection(); > + let node = sel.anchorNode; > + if (sel.isCollapsed && node && node.nodeType == 3 /* TEXT_NODE */) { > + let start = sel.anchorOffset + offset; We assume the cursor is the smaller one of anchor offset and focus offset. anchorOffset is not always the cursor position. @@ +1157,5 @@ > + if (sel.isCollapsed && node && node.nodeType == 3 /* TEXT_NODE */) { > + let start = sel.anchorOffset + offset; > + let end = start + length; > + // Fallback to setSelectionRange() if the replacement span multiple nodes. > + if (start >= 0 && end <= node.textContent.length) { Also it is better to have a test to cover this `if` case.

(In reply to Yuan Xulei [:yxl] from comment #54) > @@ +1154,5 @@ > > + if (!isPlainTextField(element)) { > > + let sel = element.ownerDocument.defaultView.getSelection(); > > + let node = sel.anchorNode; > > + if (sel.isCollapsed && node && node.nodeType == 3 /* TEXT_NODE */) { > > + let start = sel.anchorOffset + offset; > > We assume the cursor is the smaller one of anchor offset and focus offset. > anchorOffset is not always the cursor position. Note anchorOffset equals to focusOffset when it is collapsed. I decided not to handle the uncollapsed case here for easier maintenance, otherwise it may be missed easily when the assumption is changed. For the test cases, I guess that means I should create test cases to make sure the fast path does work as original code. I'll create them in part 2.

This bug can no longer be a regression blocker since the offending patch is backed out at comment 40. Normally we should close bug like this as "fixed by backout" but given the activity on the bug we could morph it to something else. Please update the title of the bug to make this more descriptive, thanks.

I am also working on bug 1064800 which is a MTBF issue, will be slow on this one.

Ting-Yu, as we discussed before, I would like to reduce some selection change events that are triggered lots of DOM events to chrome process. In bug 1065955 comment 9, ehsan suggests to use 'sel.extend' instead of 'sel.modify' which is similar with your patch. And I also think it is a right way to solve this problem. Let me know if you have problem related to gecko side.

(In reply to peter chang[:pchang][:peter] from comment #58) > bug 1065955 comment 9, ehsan suggests to use 'sel.extend' instead of > 'sel.modify' which is similar with your patch. And I also think it is a > right way to solve this problem. Let me know if you have problem related to > gecko side. I guess Yuan had explianed why in comment 13?

(In reply to Ting-Yu Chou [:ting] from comment #59) > (In reply to peter chang[:pchang][:peter] from comment #58) > > bug 1065955 comment 9, ehsan suggests to use 'sel.extend' instead of > > 'sel.modify' which is similar with your patch. And I also think it is a > > right way to solve this problem. Let me know if you have problem related to > > gecko side. > > I guess Yuan had explianed why in comment 13? Oh I misunderstood, I thought you were asking why not use |sel.extend| instead. Yes, I see, thank you.

Guys, I'd really like to help more here, but I'm kind of lost trying to follow what's happening in this bug... Can someone please summarize the most recent findings? The only issue that I am aware of for now is comment 58. And if you have any questions, please feel free to ask too!

So the input method has a content editable div, which uses replaceSurroundingText() to replace current editing word with selected suggestion, the funtion works like 1) set selection range 2) delete the selection 3) insert the text. It uses setSelectionRange() for the step1, but if you look at setSelectionRange(), you'll find it moves the cursor one by one from the beginning until it reaches the start position for replacing See http://dxr.mozilla.org/mozilla-central/source/dom/inputmethod/forms.js#1050. So I am doing comment 49, part1 is out, am working on test cases, and still need to implement a setSelectionRange like API for content editable element. Please let me know if there're still things unclear.

Added a test case to make sure the fast path of replaceSurroundingText() works as expected. Yuan, does comment 55 answer your question for the attachment 8494286 [details] [diff] [review]? If yes, are there anything else I should do to get a r+?

Comment on attachment 8494286 [details] [diff] [review] part1 v2: avoid setSelectionRange when the caret is in a text node Comment 55 makes sense for me, so I set this to r+.

Comment on attachment 8496689 [details] [diff] [review] part2 v1: test case Review of attachment 8496689 [details] [diff] [review]: ----------------------------------------------------------------- Please also add a test case that setSelectionRange() spans multiple nodes. ::: dom/inputmethod/mochitest/test_bug1066515.html @@ +56,5 @@ > + > +function test_replaceSurroundingText() { > + // Change cursor position. > + gContext.setSelectionRange(1, 0); > + nit: trailing spaces.

(In reply to Yuan Xulei [:yxl] from comment #65) > Please also add a test case that setSelectionRange() spans multiple nodes. Could you please explain me a bit more, is this a basic test as it fallbacks to original path?

(In reply to Ting-Yu Chou [:ting] from comment #62) > So the input method has a content editable div, which uses > replaceSurroundingText() to replace current editing word with selected > suggestion, the funtion works like 1) set selection range 2) delete the > selection 3) insert the text. It uses setSelectionRange() for the step1, but > if you look at setSelectionRange(), you'll find it moves the cursor one by > one from the beginning until it reaches the start position for replacing See > http://dxr.mozilla.org/mozilla-central/source/dom/inputmethod/forms.js#1050. > > So I am doing comment 49, part1 is out, am working on test cases, and still > need to implement a setSelectionRange like API for content editable element. > > Please let me know if there're still things unclear. Sounds great!

(In reply to Ting-Yu Chou [:ting] from comment #66) > (In reply to Yuan Xulei [:yxl] from comment #65) > > Please also add a test case that setSelectionRange() spans multiple nodes. > > Could you please explain me a bit more, is this a basic test as it fallbacks > to original path? Not a basic test. The basic test (http://mxr.mozilla.org/mozilla-central/source/dom/inputmethod/mochitest/test_basic.html?force=1) doesn't cover content edtiable. Please add a test case to test_bug1066515.html like this: 1. the content editable contains multiple nodes: <div contenteditable>hello <b>world</b></div> 2. set the cursor inside "hello" and replace the whole text to "abc" 3. replaceSurroundingText() will cross two text nodes and falls back to the original setSelectionRange().

Yuan, thank you for your response, sorry I forgot it is holiday in China.

Added "r=yxl" to commit message.

Comment on attachment 8499364 [details] [diff] [review] part 2 v2: test cases Review of attachment 8499364 [details] [diff] [review]: ----------------------------------------------------------------- r=me. Thanks. Please remove trailing white spaces before checking in ::: dom/inputmethod/mochitest/test_bug1066515.html @@ +51,5 @@ > + > +function test_replaceSurroundingTextWithinTextNode() { > + // Set cursor position after 'f'. > + gContext.setSelectionRange(1, 0); > + nit: trailing white spaces. @@ +70,5 @@ > + > +function test_replaceSurroundingTextSpanMultipleNodes() { > + // Set cursor position to the beginning. > + gContext.setSelectionRange(0, 0); > + nit: trailing white spaces.

Fixed nits and added "r=yxl" to commit message. Thanks Yuan!

Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=83ca684e9c43

Ting-Yu, does this patch addequately address all of the performance issues that you were seeing here? Thanks!

Unfortunately, no. It is a partial fix. It helps replacing within the text node where cursor stays, which is usually the case when user select a suggestion while editing message. But if user moves the cursor around for instance to the beginning or end of a line, or replaceSurroundText() is called to replace text span multiple/nested nodes, it still fallback to original slow path. That's why a setSelectionRange() like API for content editable element Yuan suggested in comment 17. If the API can be implemented with good performance, maybe this quick fix won't be needed. But I am not confident to make it soon, if someone else you think is suitable to do it, please feel free to take it.

(In reply to Ting-Yu Chou [:ting] from comment #75) > Unfortunately, no. It is a partial fix. :( FWIW, in these cases in the future, please move the partil fix to a separate bug and make it block the original one to make it easier to tell when the bug gets fixed. > It helps replacing within the text node where cursor stays, which is usually > the case when user select a suggestion while editing message. > > But if user moves the cursor around for instance to the beginning or end of > a line, or replaceSurroundText() is called to replace text span > multiple/nested nodes, it still fallback to original slow path. That's why a > setSelectionRange() like API for content editable element Yuan suggested in > comment 17. You mean like this? <http://mxr.mozilla.org/mozilla-central/source/dom/webidl/Selection.webidl?force=1#39> > If the API can be implemented with good performance, maybe this quick fix > won't be needed. But I am not confident to make it soon, if someone else you > think is suitable to do it, please feel free to take it. I think you have done a good job here. :-) I didn't mean to rush you, just wanted to understand whether there is more work to be done here.

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #76) > FWIW, in these cases in the future, please move the partil fix to a separate > bug and make it block the original one to make it easier to tell when the > bug gets fixed. Thanks for telling me this. What if I keep this bug open even after landing, and create a bug for implementing the API which blocks this one? > > But if user moves the cursor around for instance to the beginning or end of > > a line, or replaceSurroundText() is called to replace text span > > multiple/nested nodes, it still fallback to original slow path. That's why a > > setSelectionRange() like API for content editable element Yuan suggested in > > comment 17. > > You mean like this? > <http://mxr.mozilla.org/mozilla-central/source/dom/webidl/Selection. > webidl?force=1#39> Not really. Range needs a start node for the start offset and an end node for end offset, but replaceSurroudningText() knows only an offset from the cursor position where replacing starts and the length of text to replace. So what we need is to search the start node and end node of the content editable element according to the input parameters of replaceSurroundingText().

(In reply to Ting-Yu Chou [:ting] from comment #73) > Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=83ca684e9c43 The new added test is failed on Try, but it is passed when I run only the test case locally.

(In reply to Ting-Yu Chou [:ting] from comment #78) > (In reply to Ting-Yu Chou [:ting] from comment #73) > > Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=83ca684e9c43 > > The new added test is failed on Try, but it is passed when I run only the > test case locally. Here're what I found after investigating the failure: 1. |SpecialPowers.wrap(im).setActive(true);| in test case does not alwasy trigger Keyboard:Register because it returns early from: http://dxr.mozilla.org/mozilla-central/source/dom/inputmethod/MozKeyboard.js#300. 2. Because of #1, MozKeyboard.js does not send Keyboard:Register to Keyboard.jsm, which later when Keyboard.jsm receives Keyboard.SetSelectionRange, it returns from: http://dxr.mozilla.org/mozilla-central/source/dom/inputmethod/Keyboard.jsm#165 and makes the test timed out. 3. Keyboard.jsm remembers only one keyboard ID (_keyboardID), but there could be multiple active keyboards like test_bug1043828.html, this seems a bug.

Filed bug 1078218 for #3 of comment 79. Even it is #3 breaks the test here, I will still try to understand why is Keyboard:Register is not always triggered.

Hi :ting, I found out the reason why |setActive(true)| doesn't trigger keyboard registration in your test case: It's because we never|setActive(false)| when we finish prior tests. Note that this could affect all the test cases; it's just that unfortunately my bug 1043828 patch made such side effect surface and would affect anyone that came after me. So here is a simple amending fix, to call |setActive(false)| on |inputmethod_cleanup()|. With this patch every test case is happy :) I agree that bug 1078218 is valid as long as we agree on the need to have multiple keyboards active at the same time. However, let's not have that bug block on this bug ;) Also, flagging feedback request on that patch from :yxl.

Elaboration on "Note that this could affect all the test cases": This actually affected those test cases that set current frame's mozInputMethod activeness (i.e. wrapping current frame's navigator.mozInputMethod with SpecialPowers), and did not affect test cases for 944397, 953044, 1043828. My patch above would not break these tests, since it'd just set such inactiveness on an already mozInputMethod-inactive frame.

Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=6db261c66d34

Yuan / Ehsan, I am not sure which is preferred to create the follow-up bug for implementing the setSelectionRange like API for content editable element: 1) Leave this bug open, and create a new one which blocks this one 2) Close this bug, and create a new one which blocks bug 1029943 3) Close this bug, and create a new one which blocks no one # I added leave-open keyword so it won't be closed before I know how to follow-up.

part 2 failed to apply: applying bug-1066515.part2.v3 patching file dom/inputmethod/mochitest/mochitest.ini Hunk #1 FAILED at 2 1 out of 1 hunks FAILED -- saving rejects to file dom/inputmethod/mochitest/mochitest.ini.rej could you take a look, thanks!

(In reply to Ting-Yu Chou [:ting] from comment #85) > Yuan / Ehsan, > > I am not sure which is preferred to create the follow-up bug for > implementing the setSelectionRange like API for content editable element: > > 1) Leave this bug open, and create a new one which blocks this one > 2) Close this bug, and create a new one which blocks bug 1029943 > 3) Close this bug, and create a new one which blocks no one > > # I added leave-open keyword so it won't be closed before I know how to > follow-up. Please close this bug once the patch is checked-in and put the follow-up in a new bug. Either 2 or 3 is OK for me.

Sorry about that. Rebased.

https://hg.mozilla.org/integration/mozilla-inbound/rev/f589902d8783 https://hg.mozilla.org/integration/mozilla-inbound/rev/986b48043280 https://hg.mozilla.org/integration/mozilla-inbound/rev/78627d76c4ec

I agree with Yuan. Thanks!

https://hg.mozilla.org/mozilla-central/rev/f589902d8783 https://hg.mozilla.org/mozilla-central/rev/986b48043280 https://hg.mozilla.org/mozilla-central/rev/78627d76c4ec

Filed bug 1082306 for the follow-up.