Closed Bug 1496118 Opened 3 years ago Closed 3 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla67
Tracking Status
firefox-esr60 --- wontfix
firefox62 --- wontfix
firefox63 --- wontfix
firefox64 --- wontfix
firefox65 --- wontfix
firefox66 --- wontfix
firefox67 --- fixed

People

(Reporter: Fanolian+BMO, Assigned: m_kato)

References

Details

(Keywords: regression, reproducible)

Attachments

(3 files)

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.
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.
Has STR: --- → yes
(The bug form did not include buildID... weird)

It is reproducible on the latest Nightly 2018-10-03.
BuildID: 20181003100127
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
Flags: needinfo?(bzbarsky)
Blocks: 1290636, 1290264
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...
Flags: needinfo?(bzbarsky)
(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.
Flags: needinfo?(bzbarsky)
a/xxxx.html 30k/xxxx.html 2k/
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?
Flags: needinfo?(mats)
I implemented the new attachment uploader on BMO. Let me know if something has to be changed at content side.
Will do, though really nothing a web page does should get us in this state.
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?
Flags: needinfo?(bzbarsky) → needinfo?(tnikkel)
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.
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.
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...)
Flags: needinfo?(enndeakin)
I’m on Mac and can reproduce the issue. The caret moves slow after drag n’ dropping an attachment.
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.
Flags: needinfo?(enndeakin)

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!

Adding NI to Makoto for priority setting.

Flags: needinfo?(m_kato)

Yoshino-san, could you create simple test case?

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

Flags: needinfo?(m_kato) → needinfo?(kohei.yoshino)
Duplicate of this bug: 1489929
Assignee: nobody → 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.

Priority: -- → P2

Ah, I could create test case.

Flags: needinfo?(tnikkel)
Flags: needinfo?(mats)
Flags: needinfo?(kohei.yoshino)
Attached file drop.html

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.

Pushed by m_kato@ga2.so-net.ne.jp:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b4b115e5252
Clean up caret when destroying editor. r=masayuki
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67

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

Flags: in-testsuite+
See Also: → 1515204
You need to log in before you can comment on or make changes to this bug.