Closed Bug 1355792 Opened 7 years ago Closed 7 years ago

Cursor is moving to the Top when delete the content by pressing delete

Categories

(Core :: DOM: Editor, defect, P2)

Product:
Core
Component:
DOM: Editor
Version:
51 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox52 --- wontfix
firefox-esr52 --- wontfix
firefox53 --- wontfix
firefox54 --- wontfix
firefox55 --- fixed

People

(Reporter: pradeepmedikonda76, Assigned: ayg)

References

Details

(Keywords: regression)

Attachments

(2 files)

SampleEditor
(deleted), text/html
Details
Bug 1355792 - Consider invisible nodes to be editable;
(deleted), text/x-review-board-request
masayuki
: review+
Details 
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:52.0) Gecko/20100101 Firefox/52.0
Build ID: 20170329150444

Steps to reproduce:

Mozilla 52.0.0 version

1.Below file is a html editor. In the content when we deleting some of <br> tags by pressing delete button. cursor is moving to the top of the editor


Actual results:

The cursor is moving to the top of the editor document


Expected results:

The cursor has to continue with its position. Not to move to the top of the document
Attached file SampleEditor (deleted) — 

    
    

    
  
Regression range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=e70737edc80e82c9e5a4472c16bf7a8ccbdcfc30&tochange=38a6d608caeee3bf7d2e4482467c0de59d7d7cda

Regressed by bug 1265800.

Aryeh, is it expected?
Blocks: 1265800
Status: UNCONFIRMED → NEW
Has Regression Range: --- → yes
Has STR: --- → yes

          status-firefox52:
          --- → affected

          status-firefox53:
          --- → affected

          status-firefox54:
          --- → affected

          status-firefox55:
          --- → affected
Component: Untriaged → Editor
Ever confirmed: true
Flags: needinfo?(ayg)
Keywords: regression
Product: Firefox → Core
Summary: Curosor is moving to the Top when delete the content by pressing delete → Cursor is moving to the Top when delete the content by pressing delete

    
    

    
  
Sorry I Can't find any solutions for it. I don't know why the <br> tag reflecting  a problem... I think may be there is a lot of problem with them
Assignee: nobody → ayg
Status: NEW → ASSIGNED
Flags: needinfo?(ayg)

    
    

    
  
This is a really specific bug, it looks like.  This is about as minimal as I could get the test-case:

data:text/html,<!DOCTYPE html>
<div contenteditable><div><font><table><td>a</table><br><br><br><table><td>b</div></div>
<script>
getSelection().collapse(document.querySelector("font"), 1);
document.execCommand("forwarddelete");
document.execCommand("forwarddelete");
</script>

(The minimized version throws on Aurora, but reproduces the problem in a local compiled build.)  The cursor winds up in the wrong place, and extra <br>s are inserted.

    
    

    
  
The reduced test-case doesn't accurately match the original one.  Backing out the offending commit fixes the original test-case but not the reduced one.  Sigh.

    
    

    
  
So the problem is here:

https://hg.mozilla.org/mozilla-central/rev/7297b724b661#l2.12

I fixed IsVisBreak to correctly handle certain cases it was getting wrong, but I also regressed it in one case.  (As I noted in the commit message, this part of the change was always a bit scary.)  The exact bit of the markup seems to be:

  <font>82812345673<br></font><br><div></div>

We are checking the second <br>.  IsVisBreak used to return true, and now returns false.  In fact, there are effectively two consecutive <br>s here, although the first is inside a <font>.  Previously both were regarded as visible, and after bug 1265800, only the first is regarded as visible.

This causes HTMLEditRules::AdjustSelection to try to insert a new <br>.

The thing I don't understand is why the selection gets anywhere near that node.  I think what we had here is two bugs canceling each other out.  Which might be why my minimized test-case wasn't fixed by backing out the offending change -- it might have isolated the actual underlying bug.

Slightly better minimized test-case that may or may not show the same bug as the original case:

data:text/html,<!DOCTYPE html>
<div contenteditable><div><font><table><td>a</table><br><br><table><td>b</div></div>
<script>
getSelection().collapse(document.querySelector("font"), 1);
document.execCommand("forwarddelete");
</script>

This inserts an incorrect extra <br> at the top.
Priority: -- → P2

    
    

    
  
In my latest minimized test-case (end of last comment) I've tracked it down to the following: IsEmptyNodeImpl() thinks that the editable div in this case is empty (?!?!).  This is because it only looks at editable nodes, and it thinks that the editable div's first child is not empty, because it has no primary frame and the lazy frame bit is not set.  Something here is seriously wrong -- the div here obviously is editable.  I don't understand what's going on because I don't know anything about layout code.  List() on the editing host outputs this:

div@0x7f72fae60ee0 contenteditable="" state=[188040000002] flags=[00100208] primaryframe=0x7f72faec1f08 r
efcount=15<
  div@0x7f72fae60f70 state=[100040000000] flags=[01900200] primaryframe=(nil) refcount=6<
    font@0x7f72fb823080 state=[100040000000] flags=[00100208] ranges:1 primaryframe=(nil) refcount=21<
      table@0x7f7311eab520 state=[100040000000] flags=[00100200] primaryframe=(nil) refcount=3<
        tbody@0x7f72fb8240d0 state=[100040000000] flags=[00100200] primaryframe=(nil) refcount=3<
          tr@0x7f72fb824f70 state=[100040000000] flags=[00100200] primaryframe=(nil) refcount=3<
            td@0x7f72faec6080 state=[100040000000] flags=[00100200] primaryframe=(nil) refcount=3<
              Text@0x7f72faec6110 flags=[0c000200] primaryframe=(nil) refcount=4<a>
            >
          >
        >
      >
      br@0x7f72faec6230 state=[100040000000] flags=[00100200] primaryframe=(nil) refcount=3<>
      table@0x7f7311eab5c0 state=[100040000000] flags=[00100200] primaryframe=(nil) refcount=3<
        tbody@0x7f72faec62c0 state=[100040000000] flags=[00100200] primaryframe=(nil) refcount=3<
          tr@0x7f72faec6350 state=[100040000000] flags=[00100200] primaryframe=(nil) refcount=3<
            td@0x7f72faec63e0 state=[100040000000] flags=[00100200] primaryframe=(nil) refcount=4<
              Text@0x7f72faec6470 flags=[0c000200] primaryframe=(nil) refcount=2<b>
              script@0x7f72fae0bc80 state=[100040000000] flags=[00100208] primaryframe=(nil) refcount=14<
                Text@0x7f72faec6500 flags=[00000200] primaryframe=(nil) refcount=2<getSelection().collapse(document.querySelector("font"), 1);document.execCommand("forwarddelete");>
              >
            >
          >
        >
      >
    >
  >
>

Masayuki, do you know why all the primaryframes here are (nil), and why the div child is considered invisible and therefore non-editable?  This is the return that says it's invisible:

https://hg.mozilla.org/mozilla-central/file/722fdbff1efc/editor/libeditor/EditorBase.cpp#l3525

This causes IsEditable to return false, so IsEmptyNodeImpl returns true.  An easy workaround is to not have IsEmptyNodeImpl check for editability, which indeed IMO it shouldn't, because a block that contains non-editable content is still empty.  That fixes this reduced test-case, but doesn't fix the original case after all (oh well).  But it's still a bug that the div here is considered invisible and non-editable.
Flags: needinfo?(masayuki)

    
    

    
  
(In reply to Aryeh Gregor (:ayg) (working March 28-April 26) from comment #7)
> Masayuki, do you know why all the primaryframes here are (nil), and why the
> div child is considered invisible and therefore non-editable?

I don't know. I'm not so familiar with layout's design. (Although, I fixed some bugs around text.)

When did you get the list? It might be too early to get the frame information.

> This is the return that says it's invisible:
> 
> https://hg.mozilla.org/mozilla-central/file/722fdbff1efc/editor/libeditor/
> EditorBase.cpp#l3525
> 
> This causes IsEditable to return false, so IsEmptyNodeImpl returns true.  An
> easy workaround is to not have IsEmptyNodeImpl check for editability,

According to the method name, it should NOT use IsEmptyNodeImple(). Instead, there should be a method to check the visibility.

BTW, I confused about this.

> +  // A single line break before a block boundary is not displayed, so e.g.
> +  // foo<p>bar<br></p> and foo<br><p>bar</p> display the same as foo<p>bar</p>.
> +  // But if there are multiple <br>s in a row, all but the last are visible.
>    if (!nextNode) {
>      // This break is trailer in block, it's not visible
>      return false;
>    }
>    if (IsBlockNode(nextNode)) {
>      // Break is right before a block, it's not visible
>      return false;
>    }

Following 2 spaces between <p> elements are not same:

<p>a</p><br><p>b</p><p>c</p>

So, should the last <br> followed by a block should be visible?
Flags: needinfo?(masayuki)

    
    

    
  
Ehsan, do you know why the div mentioned in comment #7 isn't visible?  Sorry to bother you, but I don't know who else might know this kind of thing.


That looks like the root problem.  The code here is looking for a place to put the selection, and it skips over non-editable nodes:

https://hg.mozilla.org/mozilla-central/file/722fdbff1efc/editor/libeditor/HTMLEditRules.cpp#l7505

This leads it to go to the editing host root, causing the selection to jump to the top.  Commenting out that code fixes the original test-case, but obviously is not a real solution.  A reduced test-case that seems to more closely match the original one:

data:text/html,<!DOCTYPE html>
<div contenteditable><div><font><table></table>x<br><br><br><table></table>
<script>
getSelection().collapse(document.querySelector("font"), 3);
document.execCommand("forwarddelete");
</script>

(This doesn't seem to work properly in Aurora, but on a locally-compiled build it does.)
Flags: needinfo?(ehsan)

    
    

    
  
Sorry to jump in here, Mats is a layout expert and has helped in layout related issues, for example bug 1263357. Maybe consult with him.

    
    

    
  
Okay, Mats, do you know why the div mentioned in comment #7 isn't visible?  Whoever answers first, if you know the answer, please clear the needinfo for the other as well.
Flags: needinfo?(mats)

    
    

    
  
It looks like there's a pending nsChangeHint_ReconstructFrame restyle for the inner DIV (that has no frame).
As you noted, that element doesn't have HasFlag(NODE_NEEDS_FRAME) set, but that bit only
reflect lazy frame construction (AFAIK), not "there is a pending ReconstructFrame restyle".

I tend to think the editor shouldn't poke at these low-level flags in general.
It's brittle and can break at any moment if layout/style implementation details change.
If an editor operation needs an up-to-date frame tree then it should do
FlushPendingNotifications(FlushType::Frames), but beware that this may destroy arbitrary
objects, including the editor itself etc unless you have strong pointers on the stack.

In this case, perhaps you could flush at the end of TextEditor::DeleteSelection (before
AutoRules goes out-of-scope) but I'm not sure if it's safe to do that, or if it would
make more sense architecturally in some other place.
Flags: needinfo?(mats)
Flags: needinfo?(ehsan)

    
    

    
  
Editor does lots of DOM mutations, so you usually have to assume that anything might fire synchronous mutation events that remove your references to anything, so as a rule, all code keeps strong references to everything always, and anything that doesn't is probably somehow already a use-after-free (which fuzzing has caught more than one of).  :)  So I'm going to assume that it's safe to flush anywhere.

In general, it seems like we can't rely on visibility information here, so this seems like a more fundamental problem in the way we're doing things.  It seems like we should be willing to consider invisible nodes editable.  Ehsan and Masayuki, do you know why they're not?  I don't see any reason why they shouldn't be.
Flags: needinfo?(masayuki)
Flags: needinfo?(ehsan)

    
    

    
  
To be more specific, the editor calls IsEditable() all over the place, and this cannot change.  I assume it doesn't make sense to flush pending layout changes every time, so it doesn't seem like we can make IsEditable() depend on visibility.  But maybe there's some really good reason we do, in which case I don't know what we should do.

    
    

    
  
So current status is I have two patches that both fix this, and am waiting on the needinfos to decide which approach is better, as well as on try results to see if either one causes problems:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8555180904738353fe1063f0caa8b4f11ca73e65
https://treeherder.mozilla.org/#/jobs?repo=try&revision=06b989c28a393dd9080b2542ebaa0662872c2bde

    
    

    
  
Both patches seem to pass all tests, so I think removing the visibility requirement from IsEditable seems like the obvious right thing to do, unless someone knows why we need it.

    
    

    
  
How would the suggested changes affect the following testcase?

<div contenteditable>A<span style="display:none">BBBB</span>C</div>

If I place the caret after the "C" and then type a BACKSPACE, I get:
In Nightly: the "C" text node is removed
In Chrome: the "C" text node and the whole span element is removed

In Nightly: typing BACKSPACE a few more times removes one "B" at a time...
but no matter how many BACKSPACE I type, the <span> element stays.

    
    

    
  
Neither patch appears to change our behavior in that test-case.  FTR, our behavior is very silly here, and I think Chrome is correct.  WebKit matches Chrome as well, and that's the behavior I specced (not that anyone follows my spec).

    
    

    
  
Anyway, they are too risky, though, but I like https://hg.mozilla.org/try/rev/c58187fe8034ef06b2c35f27a105bfde27b9c10e better.

I agree with the Chrome's behavior too. However, contents in invisible node shouldn't be modifiable except deletable of entire of the invisible node.

If the approach has serious regression and I cannot work on the regression after you leave, I can take https://hg.mozilla.org/try/rev/2c7d2e7986ad3835ccefcb29bedaedc4981dfe23

# Perhaps, we need have two IsEditable() methods? One is for checking if it's modifiable in the node and the other is for checking if it's deletable?
Flags: needinfo?(masayuki)

    
    

    
  
Note that we had "flushing issues" in bug 795418. Mats fixed them like this:
https://hg.mozilla.org/mozilla-central/rev/045a0995bdec
So this indicates to me that the second solution is less risky.

    
    

    
  
(In reply to Masayuki Nakano [:masayuki] from comment #19)
> # Perhaps, we need have two IsEditable() methods? One is for checking if
> it's modifiable in the node and the other is for checking if it's deletable?

I think we don't need special cases outside of delete code.  The cursor can't normally go inside such a node anyway, so I don't think it matters what happens if the cursor somehow gets placed there and the user types something etc.  I also don't think it matters if we insert markup inside, it doesn't hurt anything.  Especially since we need to flush to check whether it's visible, and we don't want to flush more than necessary.  One flush before every delete operation isn't a big deal.

(In reply to Jorg K (GMT+2) from comment #20)
> Note that we had "flushing issues" in bug 795418. Mats fixed them like this:
> https://hg.mozilla.org/mozilla-central/rev/045a0995bdec
> So this indicates to me that the second solution is less risky.

Thanks for the link!  Good to know.  The second solution doesn't fix the underlying problem -- there are probably going to be loads of other cases where we need to flush to get correct results for IsEditable.  If we keep visibility as a condition for editability, it's not likely to be correct to call IsEditable without flushing every time, which is not going to be good for performance.  It's also a potential security risk every time we flush.  Better if we can just get rid of the check entirely, and then we can also get rid of the flushes in bug 795418.

The requirement for an associated frame goes back to Netscape, which I think is not in Mercurial, and has no explanation nor any associated bug:

https://github.com/mozilla/gecko-dev/commit/7bc9bfc2b03583bee56f48d44058572452b62720

And it seems we have no tests that cover it, and Chrome/WebKit don't have such a restriction, and our implementation of it is buggy.  So I think we should try to scrap it, unless Ehsan knows of a reason to keep it.

    
    

    
  
I think you are right. But the way how to fix the problem is not find out yet. Facing a lot bugs in, forward delete and delete operations. I think a proper arrangement of html can fix the problem. but why don't we remove those tags which are causing problem. Recorrect the html when we display it in html editor. Any better way to do that..

    
    

    
  
Sorry for my delay here.

The reasons why we have these IsEditable() checks all over the place are historical, and they predate me (IOW, I'm not quite sure!) but getting rid of them is probably impractical given how invasive the current call sites are.

Please don't assume that flushing anywhere in the editor code is automatically safe, code further up the call stack needs to be carefully audited.  Many references in the editor code are strong but I suspect there are still quite a few which aren't and in general flushing can be super dangerous and should be done with extreme care.

The first paragraph of comment 12 I think accurately describes the issue here.  I think Jorg is exactly right, this is very similar to bug 795418 and I suspect the solution would also be very similar.  I think Aryeh's suggestion is nice, and very brave...  The problem with doing things like this in the editor code base in my experience is that there may be any number of hidden assumptions based on the visibility checks, and I honestly can't predicts how many things will break if you attempt to rip out the visibility checks themselves.  It would be *super* nice if that happens one day as these checks have been the source of numerous bugs over the years (sometimes missing flushes, sometimes performance issues due to flushes, sometimes crashes and security holes due to flushes, etc.) but it is certainly a super high risk fix.  My recommendation would be that if you choose to go down that path, please be prepared for dealing with the fallout in the form of regressions.

(Also, Jorg, if this bug goes down the path of removing the visibility checks, please make sure to keep an eye on Thunderbird specific regressions for a while.  I seem to remember that some of the regressions caused by these visibility checks in the past may have affected Thunderbird more than Firefox -- or perhaps Thunderbird users would report the regressions sooner.  Unfortunately it's hard to search for this history in Bugzilla.  I tried to search a bit but it's been so long and I remember so little from those days that I couldn't get very far.  But please watch out in case we end up taking that path...)
Flags: needinfo?(ehsan)

    
    

    
  
So it looks like we either remove the visibility checks, which has unknown regression risks but has the potential to solve lots of bugs.  Or we add more flushes to fix reported problems, which isn't going to fix the bugs and might add security bugs.  It seems to me that it's worth trying to get rid of the visibility checks and weather any regressions.  What do you prefer, Masayuki?
Flags: needinfo?(masayuki)

    
    

    
  
I like the way to move us forward. So, I'd like you to remove the visibility check even though it's risky. If we'd get receive serious regression, we should take the flushing layout patch instead. It's also risky as you said, but its regression may be reproducible with different path because such regression means that there are hidden bugs.

On the other hand, I still have the question mentioned in comment 8. <br> immediately before block node shouldn't be treated as visible? Because I see empty line caused by such <br>.

If HTMLEditor::IsVisBreak() returns true from here:
https://hg.mozilla.org/mozilla-central/rev/7297b724b661#l2.31
, the bug of IsEditable() isn't a matter in this bug. (Or cannot fix the original reported case with this?)
Flags: needinfo?(masayuki) → needinfo?(ayg)

    
    

    
  
There is no difference in rendering between "a<p>b" and "a<br><p>b".  Of course, any time a break is invisible, if you put another break next to it it will be visible, e.g., "a<br><br><p>b".  In that case, IsVisBreak() returns true for the first break and false for the second.  I think this is correct -- we don't want to put the cursor after the second (invisible) break, but after the first (visible) is fine.

Do you want the removal of the visibility check to ride the trains like regular, or should we hold it back to early beta?
Flags: needinfo?(ayg) → needinfo?(masayuki)

    
    

    
  
(In reply to Aryeh Gregor (:ayg) (working March 28-April 26) from comment #26)
> There is no difference in rendering between "a<p>b" and "a<br><p>b".  Of
> course, any time a break is invisible, if you put another break next to it
> it will be visible, e.g., "a<br><br><p>b".  In that case, IsVisBreak()
> returns true for the first break and false for the second.

Ah, I understand. Indeed, it should be treated as invisible.

> Do you want the removal of the visibility check to ride the trains like
> regular, or should we hold it back to early beta?

Hmm, even if it's possible to disable the behavior with #ifdef or pref, it means that disabling "fixed" behavior causes this bug being back. So, I feel that it doesn't make sense.

So, I think that it should ride the train, but if we'll get some serious regression reports and there are no enough time to fix the regressions, we should back it out and take the other approach (or just back it out).

On the other hand, I'm still confused. Doesn't that mean that IsVisBreak() will ignore the visibility in spite of its name? It's too bad. I.e., current callers expect that they want editable and visible break.  However, we are now thinking that they should not check the visibility. So, perhaps, you should rename |IsVisBreak| to |IsEditableBreak|.

(And, this is random thought, though, is it safer IsEditable() to ignore the visibility only when the target is a <br>?)
Flags: needinfo?(masayuki)

    
    

    
  
I don't think IsVisBreak() cares whether the nodes have a frame or not.  It uses purely DOM logic AFAICT.  This change is strictly about editability.

    
    

    
  
Hmm, am I misunderstanding the point? I thought IsVisBreak() uses IsEditable(), but now I checked IsVisBreak() again, then, I realized that it doesn't use it. Could you tell me the stack trace when it reaches IsEditable() with the above cases? I'm really confused at who are the callers.
Flags: needinfo?(ayg)

    
    

    
  
Ah, is the stack trace like:

EditorBase::IsEditable()
HTMLEditor::IsEmptyNodeImpl()
HTMLEditor::IsEmptyNode()
HTMLEditRules::AdjustSelection()
HTMLEditRules::AfterEditInner()

?

    
    

    
      
  


  

    
    

    
  
I don't remember, but I think it's something like that, yes.  Nothing to do with IsVisBreak.
Flags: needinfo?(ayg)

    
    

    
  
Thanks, I check the code with your patch.

    
    

    
  
Comment on attachment 8860992 [details]
Bug 1355792 - Consider invisible nodes to be editable;

https://reviewboard.mozilla.org/r/133022/#review135772

Thank you for your work and sorry for the delay due to my misunderstanding.

I think that if we'll get serious regression reports with this patch, we should create EditorBase::IsElementVisible() and use it in some callers of IsEditable().

::: editor/libeditor/tests/test_bug1355792.html:14
(Diff revision 1)
> +is(getSelection().focusNode, font, "Selection node should not change");
> +is(getSelection().focusOffset, 1, "Selection offset should not move");

Could you check if the selection is collapsed with getSelection().isCollapsed after delete?

        Attachment #8860992 -
        Flags: review?(masayuki) → review+

    
    

    
      
  

    
    

    
  
Pushed by ayg@aryeh.name:
https://hg.mozilla.org/integration/autoland/rev/e05f84ea2a33
Consider invisible nodes to be editable; r=masayuki

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/e05f84ea2a33
Status: ASSIGNED → RESOLVED
Closed: 7 years ago

          status-firefox55:
          affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

    
    

    
  
Is this something we should consider for backport or can it ride the 55 train?
Flags: needinfo?(ayg)

    
    

    
  
This is a risky patch and definitely should not be considered for backport.
Flags: needinfo?(ayg)
Depends on: 1405747
Depends on: 1411345

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: