Closed Bug 1094000 Opened 10 years ago Closed 10 years ago

Caret position becomes out of message body text box

Categories

(Firefox OS Graveyard :: Gaia::SMS, defect)

Product:
Firefox OS Graveyard
Component:
Gaia::SMS
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: m_kato, Assigned: masayuki)

References

(Depends on 1 open bug,
URL
)

Details

Attachments

(4 files)

screenshot
38.25 KB, image/png
Details
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/25826
46 bytes, text/x-github-pull-request
Details | Review
part.1 Ensure that nsHTMLEditRules::InsertBRIfNeeded() runs after a text node is removed at handling Backspace key
2.18 KB, patch
roc
: review+
m_kato
: feedback+
Details | Diff | Splinter Review
Bug 1094000 part.2 Add tests of HTML editor inserting <br> node at removing a last character
5.52 KB, patch
roc
: review+
Details | Diff | Splinter Review 
When using text input, it cannot remove <br> element into message body. But, text selection can remove <br> element into message body, So this issue occurs.  I think that message body becomes real empty, we shoulld set <br> element.


- Step
1. Lanucn Messages App
2. Type 'Aaa'
3. Long Tap, then tap [cut] icon on selection bubble
4. Tap attachment, then select a picture.
5. Type 'Bbb'.  Message body becomes attachment picture and 'Bbb'
6. Long Tap, then tap [select all] icon on selection bubble
7. Tap [Paste] icon on selection bubble.  Message body becomes 'Aaa'
8. Tap [backspace] 3 times

- Result
Caret position becomes out of message body text box.  See screenshot

- Expected result 
Caret position is correct position
Attached image screenshot — 

    
    

    
  
Do you think the issue is that "cut" remove the <br>? Or that "select all" selects it?

I think we should fix this instead, if a <br> is essential to a working contenteditable, because this will hurt other places. Or fix that we need a <br> in the first place, this is hurting us in a lot of places.

I don't know who can help us here though.
Hey Daniel, do you know who can help on contenteditable issues?
Flags: needinfo?(dholbert)

    
    

    
  
NI me as well, I want to investigate doing some plain contenteditable elements out of the SMS app.
Flags: needinfo?(felash)

    
    

    
  
(In reply to Julien Wajsberg [:julienw] from comment #3)
> Hey Daniel, do you know who can help on contenteditable issues?

(ehsan, I think.)
Flags: needinfo?(dholbert)

    
    

    
  
(ah, ehsan's bugzilla username says he's out for the next couple weeks. 'hg log ./editor' suggests that :aryeh & :masayuki can probably also help with contenteditable questions.)

    
    

    
  
Looks like a bug of HTML editor...

1. Select all the editor
2. Type 'a'.
3. Type Backspace

Then, you can see same result even on Firefox (for desktop).

I'll take a look but anyway, I think that ehsan should check the behavior how it should behave as.
URL: http://jsfiddle.net/8umty4on/

    
    

    
  
Oops, the problem is simpler.

Load the new URL. And if you delete the 'a' with Backspace key, the editor will be collapsed. However, if you delete it with Delete key with/without "Select all", the editor isn't collapsed. I think that this is a simple bug of handling Backspace key.
URL: http://jsfiddle.net/8umty4on/ → http://jsfiddle.net/xdLt8ouh/

    
    

    
  


  
Although, I'm still testing this patch, I think that SMS's issue can be fixed by this patch.

Kato-san, do you have a time to check this patch with Firefox OS? If not, I'll try to do it but need a couple of days.
Assignee: m_kato → masayuki
Status: NEW → ASSIGNED

        Attachment #8517898 -
        Flags: feedback?(m_kato)

    
    

    
  
Comment on attachment 8517898 [details] [diff] [review]
part.1 Ensure that nsHTMLEditRules::InsertBRIfNeeded() runs after a text node is removed at handling Backspace key

This work fine on the latest Firefox OS with Flame.

        Attachment #8517898 -
        Flags: feedback?(m_kato) → feedback+

    
    

    
  
Hey Masayuki, Makoto, I thought it could be another issue, see bug 1094633. I'll let you decide what you want to fix.

From my point of view, the editor should not try to add <br>, it's only workarounds. In the SMS app we took great care to keep a <br> in all cases to avoid bug 1094633, but this is very cumbersome.

IMO the HTML creator should just add a min-height to keep the contenteditable big enough. But then the cursor should always be at the right location, which is not the case in some situations (bug 1094633).
Depends on: 1094633
Flags: needinfo?(felash)

    
    

    
  
Note that we correctly have a min-height in the SMS app (because I think it's better to not rely on the editor adding <br>). So we don't have the collapsing issue you're talking about, but we obviously have the cursor issue from bug 1094633 :)
Depends on: 904846

    
    

    
  
Actually, the cursor issue is bug 904846 and has r+ patches on it.

    
    

    
  
Although, I'm not sure if bug 904846 fixes the empty contenteditable height issue. I guess that the patch for bug 904846 fixes the position of caret.

At least about the height of contenteditable element, my patch makes better compatibility with IE 11 and Google Chrome. So, I'm thinking that my patch must be necessary for better compatibility...

    
    

    
  
I tested additionally on other browsers. Then, IE also appends <br> node if contenteditable element becomes empty. But Google Chrome does not.

And I forgot to say, Kato-san, thank you for your confirmation!

    
    

    
  
If the patch of bug 964846 doesn't keep the height of focused contenteditable when its content is empty, only our users see 0 height editor. It's difficult to set focus by mouse after it loses focus. I'll check the patch tomorrow what it will fix.

    
    

    
  
Yep, I don't think that Bug 904846's goal is to fix the height=0 issue :)

But from my point of view, we should not try to fix the height=0 issue. I mean, if there is <div contenteditable></div>, we have height=0 too. How is that different? From my point of view, the developer needs to add a min-height, like he would with any other element.

Not sure if this makes sense?

See http://jsbin.com/yogoyikoyu/1/edit to illustrate my thought.

    
    

    
  
(In reply to Julien Wajsberg [:julienw] from comment #17)
> But from my point of view, we should not try to fix the height=0 issue. I
> mean, if there is <div contenteditable></div>, we have height=0 too.

Yes. I don't touch this issue with the patch, though, IE and Chrome doesn't create 0-height element at this case too. IE inserts <br> in this case too. Chrome does NOT insert <br> and not specifying min-height of CSS with UA style sheet... I guess Chrome inserts invisible <br> element like our native anonymous element.

> How is that different? From my point of view, the developer needs to add a
> min-height, like he would with any other element.

Yes, I agree with that. However, in this case, other browsers behave differently from Gecko. This is bad for compatibility with other browsers.

This fact means that the empty contenteditable element case must be realized and hacked by web authors when they test with Firefox. However, if contenteditable editor always has some content, our user may remove all contents by Backspace key. Then, this bug occurs. I mean that this could not be realized by the authors because our "Backspace key" behavior isn't same as "Select All" + "Delete".

Anyway, I believe that the inconsistency should be fixed. Current our behavior is:

1. Removing last character with Backspace:                 Makes the editor 0-height
2. Removing last character with Delete:                    Inserting <br>
3. Removing all contents which is selected with Backspace: Inserting <br>
4. Removing all contents which is selected with Delete:    Inserting <br>

Only #1 causes making 0-height editor and it causes weird　look and feel.

> Not sure if this makes sense?
> 
> See http://jsbin.com/yogoyikoyu/1/edit to illustrate my thought.

Your thought is not wrong. However, better compatibility with other browsers is better for us. And also, our behavior is not constant between operations. I want to fix this inconsistency.

And I want to fix the empty conenteditable case too but I think that it causes a lot of oranges and we need discuss more. So, if we will fix the case, we should separate it to another bug.

    
    

    
  
I really agree with you about the compatibiltiy with other browsers, and you obviously know better than me here.

The only sane path forward is to spec contenteditable properly, but that's a huge task.

    
    

    
  
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #8)
> Oops, the problem is simpler.
> 
> Load the new URL. And if you delete the 'a' with Backspace key, the editor
> will be collapsed. However, if you delete it with Delete key with/without
> "Select all", the editor isn't collapsed. I think that this is a simple bug
> of handling Backspace key.

Correct behavior is that if all content in the editing host is deleted (such that it would collapse), the editor should insert a <br>.  Note that this is not just relevant to the editing host -- if you have some text like

<div contenteditable><p>abc</p><p>def</p></div>

and the user backspaces over "def", a <br> must be put in the second <p> so it doesn't collapse.  If this isn't happening sometimes, that's a bug.

(IIRC, WebKit magically adds min-height to contenteditable blocks, or did last I checked a couple of years ago.  But I don't think that's desirable.)

(In reply to Julien Wajsberg [:julienw] from comment #11)
> From my point of view, the editor should not try to add <br>, it's only
> workarounds. In the SMS app we took great care to keep a <br> in all cases
> to avoid bug 1094633, but this is very cumbersome.

What do you suggest the editor do with the <p> in the case I gave above?  It's the same case, just for a block within the editor instead of the editing host itself, and should be handled by the same code.  We need to insert a <br> here, and the fact that we're not is just a bug.

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #15)
> I tested additionally on other browsers. Then, IE also appends <br> node if
> contenteditable element becomes empty. But Google Chrome does not.

Chrome doesn't need to add the <br> because of their magic behavior, as noted above.

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #9)
> Created attachment 8517898 [details] [diff] [review]
> part.1 Ensure that nsHTMLEditRules::InsertBRIfNeeded() runs after a text
> node is removed at handling Backspace key

Does this patch pass tests?  If it passes tests and you don't want to wait for Ehsan to get back, you can ask roc for review.  I didn't look at the implementation, so I can't vouch for whether it's the right way to do things, but if it passes tests I imagine it's fine.

This should already be tested by imptests/editing/conformancetest/test_runtest.html, so you shouldn't need a new test, just remove the expected failures from the .json file.  (Actually it looks like that only tests the Ctrl-A case, not backspacing over the last letter.)

    
    

    
  
(In reply to Julien Wajsberg [:julienw] from comment #19)
> The only sane path forward is to spec contenteditable properly, but that's a
> huge task.

There is a spec, and it handles this case correctly (inserts a br):

https://dvcs.w3.org/hg/editing/raw-file/tip/editing.html#delete-the-selection

The step that does it is this: "If start block has no children, call createElement("br") on the context object and append the result as the last child of start block."

You can test yourself in a JavaScript implementation of the spec, by putting the input "a[]" without quotes into the box under "delete" and seeing what the spec outputs and what your browser outputs: https://dvcs.w3.org/hg/editing/raw-file/07d356bde327/autoimplementation.html#delete

The spec outputs "{}<br>", which is to say, a <br> is inserted.  Firefox does not match the spec here.

The problem with the spec in the end wasn't a lack of spec work -- it was lack of implementation.  Browsers would need to rewrite their editing code to converge, and they aren't willing to invest the resources.  (Which I think is fine, all things considered.)

    
    

    
  
Thanks Aryeh for correcting me, this is very informative.

    
    

    
  
(In reply to :Aryeh Gregor from comment #20)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #9)
> > Created attachment 8517898 [details] [diff] [review]
> > part.1 Ensure that nsHTMLEditRules::InsertBRIfNeeded() runs after a text
> > node is removed at handling Backspace key
> 
> Does this patch pass tests?  If it passes tests and you don't want to wait
> for Ehsan to get back, you can ask roc for review.  I didn't look at the
> implementation, so I can't vouch for whether it's the right way to do
> things, but if it passes tests I imagine it's fine.
> 
> This should already be tested by
> imptests/editing/conformancetest/test_runtest.html, so you shouldn't need a
> new test, just remove the expected failures from the .json file.  (Actually
> it looks like that only tests the Ctrl-A case, not backspacing over the last
> letter.)

Thank you for a lot of information. I'll ask review roc as you said.

I've already created new tests as mochitest-plain for this bug. I'll land them too.

    
    

    
  
Comment on attachment 8517898 [details] [diff] [review]
part.1 Ensure that nsHTMLEditRules::InsertBRIfNeeded() runs after a text node is removed at handling Backspace key

Due to Ehsan's vacation, I'm requesting this to roc. If you think we should wait Ehsan, let me know.


nsHTMLEditor does 2 jobs at pressing backspace key.

1. Remove a character from text node.
2. If the text node becomes empty, it removes the node.

After #2, nsHTMLEditRules::InsertBRIfNeeded() inserts <br> node. This is the bottom of the first chunk of this patch. Before that, it checks |if ((action == EditAction::deleteSelection) && mDidRangedDelete)|. mDidRangedDelete is set true only when Delete key is pressed without selection or either Delete key or Backspace key is pressed with a selection. Therefore, when Backspace key is pressed without selection, we should set mDidRangedDelete true too.

Note that when this patch sets mDidRangedDelete true in the second chunk, it's between #1 and #2. Therefore, an empty text node may be there. So, the call of InsertBRIfNeeded() in the second chunk does nothing in this case.

        Attachment #8517898 -
        Flags: review?(roc)

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: