1496118 - Caret position in comment area not instantly updating after drag&dropping an attachment in BMO

Status: RESOLVED FIXED

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

Description

[Fanolian]

Attachment: Caret not updating position instantly.mp4

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:64.0) Gecko/20100101 Firefox/64.0

Steps to reproduce:

(Please refer to the attached video.)
1. Visit a "Create New Attachment" page in any bug report in BMO, e.g. https://bugzilla.mozilla.org/attachment.cgi?bugid=1495661&action=enter
2. Prepare some text in the comment area.
3. Scroll back to the top of the page, drag&drop a file in the File area.
4. Return to comment area, move caret around.


Actual results:

The caret position is not instantly updated but instead at every blink.


Expected results:

The caret should update once it is moved. Now it looks laggy.


Note:
The bug is only present if I drag&drop a file. There is no bugs when I select a file from "Browse a file", or paste an image from clipboard.

Comment 1

[Fanolian]

Mozregression points to https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2ea3d51ba1bb9f5c3b6921c43ea63f70b4fdf5d2&tochange=e5859dfe0bcbd40f4e33f4a633f73ea3473a7849 but I cannot further bisect the range since it cannot find the required builds.

Comment 2

[Fanolian]

(The bug form did not include buildID... weird)

It is reproducible on the latest Nightly 2018-10-03.
BuildID: 20181003100127

Comment 3

[Alice0775 White]

Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=81aab5c5f3d04bda4c8f9fda722db13fdc738ffc&tochange=49efb446f5b0977be697e0277cb27a2d5502fb76

Regressed by: Bug 1290636 or Bug 1290264

@:bzbarsky,
Your bunch of patch seems to cause regression, can you please look into this?

I can reproduce not only Comment area but also Description input field.
About comment area, once switch to Preview and back to Comment tab, then the problem is gone.

Comment 4

[Boris Zbarsky [:bzbarsky]]

I can't reproduce this on Mac.   Is this Windows-only?

None of the patches in the regression range should have any effect on caret painting...

Comment 5

[Alice0775 White]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #4)
> I can't reproduce this on Mac.   Is this Windows-only?
> 
> None of the patches in the regression range should have any effect on caret
> painting...

Umm..
I re-tested(2 times each) the regression window with [1], and confirmed the range is correct (100% reproducible). 

[1]
Good: https://ftp.mozilla.org/pub/firefox/tinderbox-builds/mozilla-inbound-win32/1469852485/
Bad : https://ftp.mozilla.org/pub/firefox/tinderbox-builds/mozilla-inbound-win32/1469854894/

And I can also reproduce the issue on Nightly64.0a1 Linux Mint x64.


Steps to reproduce:
1. Click "Add an attachment" link
2. Prepare some text in the comment area.
3. Scroll back to the top of the page, drag&drop a file( xxxx.html 30k) from desktop to the drop zone.
4. Return to comment area, move caret around.

Comment 6

[Alice0775 White]

a/xxxx.html 30k/xxxx.html 2k/

Comment 7

[Boris Zbarsky [:bzbarsky]]

OK.  I have reproduced on Linux, sort of.  For me the caret stops painting completely (though the editors are still editable).  It's due to this change: https://hg.mozilla.org/integration/mozilla-inbound/diff/49efb446f5b0/dom/webidl/NodeList.webidl

Now that change on its own is not the problem; presumably it's something the page does as a result.

I won't be able to further debut this until I get home from TPAC in a bit over a week...

Mats, do you have any idea what could cause the caret to stop painiting?

Comment 8

[Kohei Yoshino [:kohei]]

I implemented the new attachment uploader on BMO. Let me know if something has to be changed at content side.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Will do, though really nothing a web page does should get us in this state.

Comment 10

[Boris Zbarsky [:bzbarsky]]

OK, I poked at this for a while, and here is what I know:

1) Bug 1290636 is not really relevant.  Without that patch, the scripts on the page get an exception when doing document.querySelectorAll(stuff).forEach() and never reach some other part of the script that actually causes  the problem.  I can make the problem go away if I just remove the forEach method on nodelist, and I can make it come back if I leave forEach but just make it do nothing.

2) In the failure case, we seem to be failing to get a frame from nsCaret::GetFrameAndOffset.  In particular, we end up with aOverrideNode non-null (and a <div>) and aOverrideOffset == 0.  If I disable the aOverrideNode case in nsCaret::GetFrameAndOffset then I do see the caret, but it does not blink.

3) I _think_ in the failure case we land in nsCaret::GetCaretFrameForNodeOffset and it returns NS_ERROR_FAILURE.  I haven't tracked down yet which NS_ERROR_FAILURE return it is; the function has a bunch.

Timothy, do you still remember anything about this code?

Comment 11

[Boris Zbarsky [:bzbarsky]]

Some more info, now that I caught this in rr: The mOverrideContent of the nsCaret involved (or at least the one I think is involved) is a <div> with a null parent.

That <div> used to have a <textarea> parent but got disconnected from it when the nsTextControlFrame was destroyed.  Apparently that did not affect the caret's mOverrideContent, and it keeps pointing at the unlinked anon content?

Anyway, I looked at what set up the mOverrideContent, and that's set via this stack:

#3  0x000005ae32aced59 in nsCaret::SetCaretPosition(nsINode*, int) (this=0x6ef80ab0, aNode=0x3e461e61a820, aOffset=0)
    at /home/bzbarsky/mozilla/stylo/mozilla/layout/base/nsCaret.cpp:474
#4  0x000005ae3280a2f1 in mozilla::EditorEventListener::DragOver(mozilla::dom::DragEvent*) (this=0x7fac2b0333a0, 
    aDragEvent=0x3e461e6043a0) at /home/bzbarsky/mozilla/stylo/mozilla/editor/libeditor/EditorEventListener.cpp:804
#5  0x000005ae3280a0c7 in mozilla::EditorEventListener::DragEnter(mozilla::dom::DragEvent*) (this=0x7fac2b0333a0, aDragEvent=0x3e461e6043a0) at /home/bzbarsky/mozilla/stylo/mozilla/editor/libeditor/EditorEventListener.cpp:778
#6  0x000005ae328099d9 in mozilla::EditorEventListener::HandleEvent(mozilla::dom::Event*) (this=0x7fac2b0333a0, aEvent=0x3e461e6043a0)
    at /home/bzbarsky/mozilla/stylo/mozilla/editor/libeditor/EditorEventListener.cpp:388

which at least has something to do with drags!  If I comment out the SetCaretPosition call in EditorEventListener::DragOver, the bug goes away as far as I can tell.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Now we're getting somewhere.  The presshell's mCaret is the nsCaret above, but is not the same as its mOriginalCaret.  The only time those two should be different is between a call to SetCaret(), which only happens  in EditorEventListener::DragEnter, and RestoreCaret(), which happens from EditorEventListener::CleanupDragDropCaret.  So presumably the former happened and the latter did not.

Poking at EditorEventListener::HandleEvent a bit, I see an eDragEnter event, then an eDragOver event, then no  more drag-related events.  The eDragOver event has DefaultPrevented() true.

I do not see any eDrop or eDragExit events, which would clean up the caret state.  So we stick with the "wrong" caret and get this bug.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Neil, any idea how we end up with an enter+over but never get an exit or drop?  Those are what calls CleanupDragDropCaret, which is what's not happening.

I did check, and it's not that the EditorEventListener in question is getting destroyed. But maybe the node it's a listener on stops being in the DOM or something and that confuses things?

(It's also not clear to me why this is not reproducible on Mac...)

Comment 14

[Kohei Yoshino [:kohei]]

I’m on Mac and can reproduce the issue. The caret moves slow after drag n’ dropping an attachment.

Comment 15

[Neil Deakin]

I can reproduce on Mac also.

The drop event does fire on the content process, and it isn't cancelled as far as I can see, but the editor listener never seems to get called. The editor listener doesn't seem to get removed, although the listeners for some text fields in the attachment box get removed. I suspect this is some editor related issue.

Comment 16

[Liz Henry (:lizzard) (relman/hg->git project)]

Happy to take a patch in nightly 67, or potentially, in beta 66 for this.

Makoto, can you investigate, or help find an owner for the bug, or give it a priority? Thanks!

Comment 17

[Pascal Chevrel:pascalc (PTO until April 26)]

Adding NI to Makoto for priority setting.

Comment 18

[Makoto Kato [:m_kato]]

Yoshino-san, could you create simple test case?

Normal case receives dragexit event, but BMO doesn't.

Comment 20

[Makoto Kato [:m_kato]]

Maybe, drop event in BMO causes destroying editor... But I still need test case since I want to add test case in tree.

Comment 21

[Makoto Kato [:m_kato]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=962f1f48c619ed132d004421bfbcc47ecc041a72

Comment 22

[Makoto Kato [:m_kato]]

Ah, I could create test case.

Comment 23

[Makoto Kato [:m_kato]]

Attachment: drop.html

Comment 24

[Makoto Kato [:m_kato]]

Attachment: Bug 1496118 - Clean up caret when destroying editor. r?masayuki

Editor changes caret visibility during drag and drop. But when destroying
editor, we don't restore caret state. So we should restore it when destroying
editor.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b4b115e5252
Clean up caret when destroying editor. r=masayuki

Comment 26

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/1b4b115e5252

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Is this something which should be considered for Beta approval or can it ride the trains?