818893 - Keyboard can't get the caret position of contenteditable elements from the mozKeyboard.onfocuschange event

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Keyboard, defect, P2)

Description

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

Keyboard app, that listens to the show/hide IME event by the mozKeyboard.onfocuschange handler, should be able to get the caret position of the content editable element(for allowing predictive text for example).

Steps to reproduce:

Run UI Tests app -> Keyboard tests -> go to the Contenteditable section -> touch the "Edit Me!"

Actual results:

No caret position information is received from mozKeyboard.onfocuschange.

Expected results:

The caret position information of selectionStart and selctionEnd is received from  mozKeyboard.onfocuschange.

Comment 1

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

Attachment: Get caret position of the contenteditable elements from mozKeyboard.onfocuschange

Comment 2

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

Comment on attachment 689240 [details] [diff] [review]
Get caret position of the contenteditable elements from mozKeyboard.onfocuschange

The patch is broken with the latest nightly build.

Comment 3

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

Attachment: get the selection range and text content of the content editable element.

Get the selection range of the content editable element as well as the input and textaream element.

Comment 4

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

The bug prevents content editable element from making prediction. It breaks the text prediction feature and is a blocking bug.

Comment 5

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

Comment on attachment 691350 [details] [diff] [review]
get the selection range and text content of the content editable element.

I'm sorry for uploading the wrong patch.

Comment 6

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

Attachment: get the selection range and text content of the content editable element.

Comment 7

[Dylan Oliver [:doliver]]

Triage: blocking- for now

Yuan, can you provide more information about the end user impact of this bug and then re-nominate if you think this needs to be part of v1?

Comment 8

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

To do predictions, the virtual keyboard needs to know when the user moves the cursor so that it can throw out its current predictions. As the content editable element can't give the cursor position, if the user inputs in such an element, no predictions will be provided.


Steps to reproduce:

Run UI Tests app -> Keyboard tests -> go to the Contenteditable section -> touch the "Edit Me!" and type the word "go" at the end.

Actual results:

The prediction list is empty.

Expected results:

predictions such as "going", "government" and "God" should be given.

Comment 10

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Sigh. I close the wrong bug - sorry for the spam.

Comment 11

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 691360 [details] [diff] [review]
get the selection range and text content of the content editable element.

David has done the original selection code. Let's redirect the review to him.

Comment 12

[David Flanagan [:djf]]

Comment on attachment 691360 [details] [diff] [review]
get the selection range and text content of the content editable element.

Review of attachment 691360 [details] [diff] [review]:
-----------------------------------------------------------------

I'm giving this patch a r- because I think the code is more complicated than necessary. And it is not clear to me whether there are license issues involved in using it.

I've tried to suggest an alternative approach in my comments.  Feel free to followup by email or on irc if you have questions about how to handle this.

::: b2g/chrome/content/forms.js
@@ +86,5 @@
>  
>    handleEvent: function fa_handleEvent(evt) {
>      let focusedElement = this.focusedElement;
>      let target = evt.target;
> +    let range = [-1, -1];

This initialization is not necessary because range is always set before being used.  And, this initialization creates a new object every time an event is handled, so just let range = null here, I think.

@@ +106,5 @@
>          // We only listen for this event on the currently focused element.
>          // When the mouse goes down, note the cursor/selection position
> +        range = getSelectionRange(this.focusedElement);
> +        this.selectionStart = range[0];
> +        this.selectionEnd = range[1];

Since you ultimately have different functions for getting the start and end of the range for the inputeditable case, why not have getSelectionStart() and getSelectionEnd() and not have to deal with arrays at all?

@@ +391,5 @@
>    }
>  
>    return result;
>  };
> +  

nit: blank space

@@ +402,5 @@
> +    start = element.selectionStart;
> +    end = element.selectionEnd;
> +  } else {
> +    // Get the selection range of contenteditable elements
> +    let win = windowFromNode(element);

Can't you just use window here?  Is there any way our contenteditable element could be in a window different than the one that forms.js gets injected into?

@@ +407,5 @@
> +    let range = win.getSelection().getRangeAt(0);
> +    if (!isOutsideNode(range, element)) {
> +      start = getSelectionStart(range, element);
> +      end = getSelectionEnd(range, element);
> +    }

If this if statement is not executed, then we'll send a cursor position of -1 to the keyboard.  And I don't think it will know what to do with that value.  I suspect that returning [0,0] would be safer.

And in fact, it looks like the start and end methods here already handle the range check, so maybe you don't need the if at all.

@@ +417,5 @@
> +  if (node && node.ownerDocument) {
> +    return node.ownerDocument.defaultView;
> +  }
> +  return null;
> +}

This is an unnecessarily complex function. We know that node will never be null.  And the code that uses this function doesn't test for a return value of null.

So I'd just use window, or element.ownerDocument.defaultView, and cut this function

@@ +434,5 @@
> +}
> +
> +// The original code is from http://stackoverflow.com/questions/4767848/
> +// get-caret-cursor-position-in-contenteditable-area-containing-html-content,
> +// and was created by Tim Down

I'm not comfortable using stackoverflow code in a file this critical. 

1) it is not clear what the license is

2) the code is likely written to be cross-browser and is therefore probably more complicated than what we need here.

@@ +446,5 @@
> +  }
> +
> +  let treeWalker = win.document.createTreeWalker(
> +  node, win.NodeFilter.SHOW_TEXT, function(node) {
> +    let nodeRange = win.document.createRange();

TreeWalkers, NodeFilters, new Range objects... this is just way too complicated.

There must be an easier way.  For license reasons, the code needs to be rewritten. If you understand exactly what this code is doing and would choose to write it this way yourself, then I'll accept your rewritten version of it.

But it seems to me that it would be much easier to just start at the contenteditable element and do a depth-first traversal until you find the selection.anchorNode (no need to use the Range object at all), and count the length of the text nodes you encounter along the way. And when you get to the anchorNode, add the anchorOffset.

A nice little optimization is is to use Node.compareDocumentPosition() to avoid traversing into elements that come before the anchorNode.  If a node does not contain the anchorNode, then you can just add its textContent.length to your count and skip on to its nextSibling.

@@ +466,5 @@
> +// The original code is from http://stackoverflow.com/questions/4767848/
> +// get-caret-cursor-position-in-contenteditable-area-containing-html-content,
> +// and was created by Tim Down
> +function getSelectionEnd(range, node) {
> +  let win = windowFromNode(node);

It seems unnecessary and inefficient to have two different functions for the start and the end.  It would be much quicker to find the end of the range relative to the start of the range rather than the start of the contenteditable element. So do that, and then add the offset to the start position.

Comment 13

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

David, thanks for your review and it really helps. I'll create a new patch with your suggested approach later. 

(In reply to David Flanagan [:djf] from comment #12)
> Comment on attachment 691360 [details] [diff] [review]
> get the selection range and text content of the content editable element.
> 
> Review of attachment 691360 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm giving this patch a r- because I think the code is more complicated than
> necessary. And it is not clear to me whether there are license issues
> involved in using it.
> 
> I've tried to suggest an alternative approach in my comments.  Feel free to
> followup by email or on irc if you have questions about how to handle this.
> 
> ::: b2g/chrome/content/forms.js
> @@ +86,5 @@
> >  
> >    handleEvent: function fa_handleEvent(evt) {
> >      let focusedElement = this.focusedElement;
> >      let target = evt.target;
> > +    let range = [-1, -1];
> 
> This initialization is not necessary because range is always set before
> being used.  And, this initialization creates a new object every time an
> event is handled, so just let range = null here, I think.
> 

I agree with you.

> @@ +106,5 @@
> >          // We only listen for this event on the currently focused element.
> >          // When the mouse goes down, note the cursor/selection position
> > +        range = getSelectionRange(this.focusedElement);
> > +        this.selectionStart = range[0];
> > +        this.selectionEnd = range[1];
> 
> Since you ultimately have different functions for getting the start and end
> of the range for the inputeditable case, why not have getSelectionStart()
> and getSelectionEnd() and not have to deal with arrays at all?
> 
> @@ +391,5 @@
> >    }
> >  
> >    return result;
> >  };
> > +  
> 
> nit: blank space
> 
> @@ +402,5 @@
> > +    start = element.selectionStart;
> > +    end = element.selectionEnd;
> > +  } else {
> > +    // Get the selection range of contenteditable elements
> > +    let win = windowFromNode(element);
> 
> Can't you just use window here?  Is there any way our contenteditable
> element could be in a window different than the one that forms.js gets
> injected into?
>

They were the same, but are different from a recently build. So we have to get the window from the focused element. 

> @@ +407,5 @@
> > +    let range = win.getSelection().getRangeAt(0);
> > +    if (!isOutsideNode(range, element)) {
> > +      start = getSelectionStart(range, element);
> > +      end = getSelectionEnd(range, element);
> > +    }
> 
> If this if statement is not executed, then we'll send a cursor position of
> -1 to the keyboard.  And I don't think it will know what to do with that
> value.  I suspect that returning [0,0] would be safer.
> 
> And in fact, it looks like the start and end methods here already handle the
> range check, so maybe you don't need the if at all.
> 
> @@ +417,5 @@
> > +  if (node && node.ownerDocument) {
> > +    return node.ownerDocument.defaultView;
> > +  }
> > +  return null;
> > +}
> 
> This is an unnecessarily complex function. We know that node will never be
> null.  And the code that uses this function doesn't test for a return value
> of null.
> 
> So I'd just use window, or element.ownerDocument.defaultView, and cut this
> function
> 
> @@ +434,5 @@
> > +}
> > +
> > +// The original code is from http://stackoverflow.com/questions/4767848/
> > +// get-caret-cursor-position-in-contenteditable-area-containing-html-content,
> > +// and was created by Tim Down
> 
> I'm not comfortable using stackoverflow code in a file this critical. 
> 
> 1) it is not clear what the license is
> 
> 2) the code is likely written to be cross-browser and is therefore probably
> more complicated than what we need here.
> 
> @@ +446,5 @@
> > +  }
> > +
> > +  let treeWalker = win.document.createTreeWalker(
> > +  node, win.NodeFilter.SHOW_TEXT, function(node) {
> > +    let nodeRange = win.document.createRange();
> 
> TreeWalkers, NodeFilters, new Range objects... this is just way too
> complicated.
> 
> There must be an easier way.  For license reasons, the code needs to be
> rewritten. If you understand exactly what this code is doing and would
> choose to write it this way yourself, then I'll accept your rewritten
> version of it.
> 
> But it seems to me that it would be much easier to just start at the
> contenteditable element and do a depth-first traversal until you find the
> selection.anchorNode (no need to use the Range object at all), and count the
> length of the text nodes you encounter along the way. And when you get to
> the anchorNode, add the anchorOffset.
> 
> A nice little optimization is is to use Node.compareDocumentPosition() to
> avoid traversing into elements that come before the anchorNode.  If a node
> does not contain the anchorNode, then you can just add its
> textContent.length to your count and skip on to its nextSibling.

I agree and I'll rewrite the code.

> 
> @@ +466,5 @@
> > +// The original code is from http://stackoverflow.com/questions/4767848/
> > +// get-caret-cursor-position-in-contenteditable-area-containing-html-content,
> > +// and was created by Tim Down
> > +function getSelectionEnd(range, node) {
> > +  let win = windowFromNode(node);
> 
> It seems unnecessary and inefficient to have two different functions for the
> start and the end.  It would be much quicker to find the end of the range
> relative to the start of the range rather than the start of the
> contenteditable element. So do that, and then add the offset to the start
> position.

Comment 14

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

Attachment: Get the selection start and end position without traversing node tree.

The patch uses a Range object to compute selection start position. Then we can the selection end position by adding the selection length to the start position.

In addition, the patch removes an unused local variable `focusedElement` and adds the missing ";" to a line.

Comment 15

[David Flanagan [:djf]]

Comment on attachment 693425 [details] [diff] [review]
Get the selection start and end position without traversing node tree.

Review of attachment 693425 [details] [diff] [review]:
-----------------------------------------------------------------

I really like the new way that you get the selection.  Nice trick with Range.toString()!

r+ on this, but allow me to make two suggestions:

1) First of all, how about treating contenteditable like textareas instead of text, so that they get auto-capitialization by default?  That is change forms.js:297 to set type to "textarea" instead of "text"

2) Consider updating the "Edit me" text in the test_apps/uitest/tests/keyboard.html to something like "Edit me <i>please</i>" so that you can demonstrate that your patch works even when the content editable element includes formatting.

Comment 16

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

Attachment: Support multiple line editing.

The contenteditable is much like the textareas, as it supports multiple line editing. The patch fixes the bugs related to multiple line editing:

1. The contenteditable type is changed from "text" to "textareas". 

2. The `Range.toString()` is replaced by `nsIDocumentEncoder.encodeToString()`. The former doesn't recognize the "\n". That makes us get wrong selection start position when there exists "\n" before the selection start position.

Comment 17

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

Attachment: Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/7081

Pointer to Github pull-request

Comment 18

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

Comment on attachment 693796 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/7081

Update the "Edit me" text in the test_apps/uitest/tests/keyboard.html to test content editable elements including formatting.

Comment 19

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

Attachment: Support multiple line editing.

Update the patche.

The content editable is much like the textarea, as it supports multiple line editing. The patch fixes the bugs related to multiple line editing:

1. The contenteditable type is changed from "text" to "textareas". 

2. The `toString()` is replaced by `nsIDocumentEncoder.encodeToString()`. The former doesn't recognize the "\n". That makes us get wrong selection position and content text when there exists "\n" in the content editable element.

Comment 20

[David Flanagan [:djf]]

Comment on attachment 693796 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/7081

Including hidden elements is particularly devious! Good test case!

Comment 21

[David Flanagan [:djf]]

Comment on attachment 693858 [details] [diff] [review]
Support multiple line editing.

Review of attachment 693858 [details] [diff] [review]:
-----------------------------------------------------------------

I don't have much experience with chrome JavaScript, and had never heard of nsIDocumentEncoder before doing this review.  That interface sounds like it is really important but appears to be completely undocumented on MDN.  So I'm nervous about approving this patch myself.

I'm giving r+ here (Assuming you address my comment in the code) because the patch seems low risk: it can only affect the handling of contenteditable elements and can't break the keyboard for other types of input elements I think.

But I'd still like someone more qualified to take a look at this. Since Henri's name appears in the nsiDocumentEncoder.idl logs, I'm asking him for feedback on the way the interface is used here to compute the content and selection range of a contenteditable element.  Xulei: if you can find someone else who is familiar with nsIDocumentEncoder to give r+ or feedback+ on this patch, then you can cancel the feedback request from Henri.

::: b2g/chrome/content/forms.js
@@ +431,2 @@
>  
>      end = start + sel.toString().length;

Don't you have to use the documentEncoder trick here, too, in case the selection contains newlines?

Comment 22

[David Flanagan [:djf]]

Comment on attachment 693858 [details] [diff] [review]
Support multiple line editing.

Oops. I said I was giving r+, but I forgot to actually set that flag.

I should add that the use of nsIDocumentEncoder seems perfect in this patch. It is just that I'm unqualified to review that code that makes me nervous.

Comment 23

[Henri Sivonen (:hsivonen)]

Comment on attachment 693858 [details] [diff] [review]
Support multiple line editing.

The people who wrote the plaintext serializer were at Netscape and have moved on from the project. I don't think there's anyone who is currently active and understands the guts of the plaintext serializer all the way. Previously, I had to understand a large part of the plaintext serializer, but I did didn't absolutely have to understand it all, so I didn't.

I'm somewhat surprised that you are not passing the OutputSelectionOnly flag. If the serializer does what you want already, great. If you see some edge cases go wrong, you might try adding that flag.

You probably should be passing the flag OutputRaw to avoid line wrapping by added line feed characters in the serializer output.

Even though B2G probably defines line feed as the platform default break, it would be better to request a certain type of line breaks instead of relying on the platform defaults. Assuming that the rest of the code depends on line breaks being line feeds, the flag to pass is OutputLFLineBreak.

I'm a bit surprised that you are not passing OutputDropInvisibleBreak. If the code works better without that flag, great. But I suggest double checking.

Finally, it should be permissible to instantiate the serializer once and then to reuse it. However, since configuration is part of the instance, it seems incorrect to use getService. Instead, it seems to me that this code should create an instance, configure it and then hold onto it instead of relying on the service manager to hold on to it.

f+ if the above points are addressed / double checked.

Comment 24

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

Attachment: get the selection range and text content of the content editable element.

First, rebase and merge the two reviewed patches as they are out of date. 

Then make the following changes according to Henri's feedback.

1. Test and add OutputRaw, OutputLFLineBreak and OutputDropInvisibleBreak flags to ensure the nsIDocumentEncoder outputs correct result.

2. Instantiate the nsIDocumentEncoder using createInstance instead of getService.

The OutputSelectionOnly flag isn't used, since we need to serialize all the text within the range, no only the selected text.

Comment 25

[David Flanagan [:djf]]

Comment on attachment 696873 [details] [diff] [review]
get the selection range and text content of the content editable element.

Review of attachment 696873 [details] [diff] [review]:
-----------------------------------------------------------------

I have not tested this, but it looks reasonable enough to me.  Please wait for review by Henri, though, since he actually understands this stuff :-)

::: b2g/chrome/content/forms.js
@@ +289,5 @@
> +  let range = doc.createRange();
> +  range.selectNodeContents(element);
> +  documentEncoder.setRange(range);
> +  return documentEncoder.encodeToString();
> +}

The patch would be tidier if this block of new code was at the bottom with the other block of new code. JavaScript allows forward references, so it should be fine to put it down below.

But that is just a nit.

@@ +332,5 @@
>    } else {
>      inputmode = '';
>    }
>  
> +  let range = getSelectionRange(element);

How expensive is the call to getDocumentEncoder()? You're doing it twice here in getJSON().  Maybe you could explicitly pass a document encoder object to getContentEditableText() and getSelectionRange() so that you only have to create it once?

I honestly don't know whether that is worth the trouble though.

Comment 26

[Henri Sivonen (:hsivonen)]

Comment on attachment 696873 [details] [diff] [review]
get the selection range and text content of the content editable element.

r+ for the serializer instantiation. Like the previous reviewer, I wondered if the serializer object should be reused, but that might be a premature optimization.

Comment 27

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

Attachment: Tide and reuse document encoder.

1. Tidy the code.

2. Reuse the serializer object of the document encoder in getJSON() to avoid calling getDocumentEncoder twice.

Comment 28

[David Flanagan [:djf]]

Comment on attachment 699012 [details] [diff] [review]
Tide and reuse document encoder.

Review of attachment 699012 [details] [diff] [review]:
-----------------------------------------------------------------

I don't like that this now creates an encoder for every mousedown and mouseup, even when the click is over regular form elements instead of a contenteditable element.

What I was imagining is that you would write getSelectionRange() with an encoder as an optional second argument.  When invoked with just an element, it would create its own encoder, if it needed one.  But if an encoder was passed, it would use that one rather than creating its own.

In the event handlers, you wouldn't have to create an encoder.

And in getJSON() you would create the encoder only if it was a contenteditable element.  Then you'd pass that encoder (or null) to getSelectionRange().

Does that make sense?

Comment 29

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

Attachment: Only creates one document encoder for each contenteditable element

David, I made further improvement base on your comment.
--------------------------------
A document encoder will be create when user focus on the contenteditable element and it will exist until the element loses focus.

So for each contenteditable element, the patch creates only one encoder; while for regular form elements, the patch creates no encoder.

Comment 30

[David Flanagan [:djf]]

Comment on attachment 699044 [details] [diff] [review]
Only creates one document encoder for each contenteditable element

Review of attachment 699044 [details] [diff] [review]:
-----------------------------------------------------------------

r+ assuming you've tested it and are confident that FormAssistant.documentEncoder will always be non-null when you need one.

::: b2g/chrome/content/forms.js
@@ +221,5 @@
>        this.focusedElement.removeEventListener('mousedown', this);
>        this.focusedElement.removeEventListener('mouseup', this);
>        if (!element) {
>          this.focusedElement.blur();
>        }

Just a nit: do you want to set _documentEncoder to null here?

Comment 31

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

(In reply to David Flanagan [:djf] from comment #30)
> Comment on attachment 699044 [details] [diff] [review]
> Only creates one document encoder for each contenteditable element
> 
> Review of attachment 699044 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ assuming you've tested it and are confident that
> FormAssistant.documentEncoder will always be non-null when you need one.
> 
> ::: b2g/chrome/content/forms.js
> @@ +221,5 @@
> >        this.focusedElement.removeEventListener('mousedown', this);
> >        this.focusedElement.removeEventListener('mouseup', this);
> >        if (!element) {
> >          this.focusedElement.blur();
> >        }
> 
> Just a nit: do you want to set _documentEncoder to null here?

Yes, I do. It will free memory and clear invalid document encoder when focus changing.

Comment 32

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

Vivien, I would like to set the bug as blocking-basecamp and land it.

Comment 33

[David Scravaglieri [:scravag]]

We would take a patch on this.

Comment 34

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

@ttaubert, could you help to check in?
Thanks.

Comment 35

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

(In reply to Yuan Xulei [:yxl] from comment #34)
> @ttaubert, could you help to check in?
> Thanks.

Sorry, I made a mistake and the patch shoudn't be checked in.

Comment 36

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

(In reply to David Scravaglieri [:scravag] from comment #33)
> We would take a patch on this.

Do you need any help with it?

Comment 37

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

Attachment: Get selection range from contenteditable element

Rebase the reviewed patch. 

I hope that this patch could be land as soon as possible as it is blocking bug 844716. 

@David, is your patch on this ready?

Comment 38

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

Comment on attachment 718827 [details] [diff] [review]
Get selection range from contenteditable element

Just a rebase of the previous r+ed patched 
https://bugzilla.mozilla.org/attachment.cgi?id=699044

Comment 39

[Evelyn Hung]

Comment on attachment 693796 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/7081

merged to gaia-master:
https://github.com/mozilla-b2g/gaia/commit/96aeccdf4d91d53ba59e8fdeb0b4031012d50ffa

Comment 40

[Kan-Ru Chen [:kanru] (UTC+9)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4cf6c8896c93

Comment 41

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 718827 [details] [diff] [review]
Get selection range from contenteditable element

Clear reviews since the patch has been landed.

Comment 42

[[:fabrice] Fabrice Desré]

Don't mark gecko bugs as FIXED unless they land on inbound or b2g18 please.

Comment 43

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/4cf6c8896c93

Comment 44

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/36918357c7a5

Comment 45

[Mike Lee [:mlee]]

Marking as blocking tef? and requesting uplift.

Comment 46

[Daniel Coloma:dcoloma]

What is the impact for the end-user of this bug? It is not clear to me how does it affect users.

Comment 47

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

If this bug was not fixed, the end user couldn't do prediction when focusing on a content editable element, which is often used as WYSIWYG editor.

See comment 8(https://bugzilla.mozilla.org/show_bug.cgi?id=818893#c8) for details.

Note: 

If this bug is uplifted, bug 848660(https://bugzilla.mozilla.org/show_bug.cgi?id=848660) should be also uplifted, which made a patch on this bug to prevent breaking the gaia value selector.

Comment 48

[Daniel Coloma:dcoloma]

I don't believe we should block the release because of this bug. Adding a couple of product guys so they can renominate if they disagree.