Closed Bug 1332433 Opened 7 years ago Closed 7 years ago

Cursor lost after applying colour to text

Categories

(Core :: DOM: UI Events & Focus Handling, defect)

Product:
Core
Component:
DOM: UI Events & Focus Handling
Version:
52 Branch
Type:
defect
Priority:
Not set
Severity:
major

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox51 --- unaffected
firefox52 + fixed
firefox53 --- fixed

People

(Reporter: jorgk-bmo, Unassigned)

References

Details

(Keywords: regression) 

STR:
Write a new message (open compose window)
type something
select the text
make it red

Result: Cursor is lost.

Not seen in TB 51 beta, see in TB 52 Earlybird and TB 53 trunk.

Seems to be TB specific, since I can't see it in a recent FF (FF 53 of 7th Jan 2017) in the MIDAS demo at:
http://www-archive.mozilla.org/editor/midasdemo/

Alice, this is pretty bad, so please help!
Flags: needinfo?(alice0775)
Hmm, so all this landed on mozilla53. How do you explain that it doesn't work in TB 52 (Aurora)? Did any of this get uplifted to M-A?

And why don't I see it in FF on the Midas demo. (I think) I have a recent trunk build of FF (80eac484366a at Tue Jan 17 13:47:20 2017 -0800) and I can't see it there either.
Can you see any bug that landed in the M-C range that got uplifted in the M-A range. I can't.
I can also reproduce the caret problem on Nightly53.0a1 if addon ScrapBook X 1.13.1(it can focus change from the editor to dialog).

Steps to reproduce:
1. Open http://www-archive.mozilla.org/editor/midasdemo/
2. type something
3. select the text
4. Right click and click on "Save selection as" (ScrapBook X's context menu) 
5. Cancel on save detail dialog
6. 

Result: Cursor is lost.

Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=e8b32aaa2eec84c15f44cfd677142a0047481152&tochange=f3ccf0f50160e169389aa26a2d95ec225effcd3b

Suspect: a60d62c55e27	Jessica Jong — Bug 1316330 - Cancel delayed keypress events if last keydown was canceled. r=smaug
(In reply to Alice0775 White from comment #5)
> I can also reproduce the caret problem on Nightly53.0a1 if addon ScrapBook X
> 1.13.1(it can focus change from the editor to dialog).
> 
> Steps to reproduce:
> 1. Open http://www-archive.mozilla.org/editor/midasdemo/
> 2. type something
> 3. select the text
> 4. Right click and click on "Save selection as" (ScrapBook X's context menu) 
> 5. Cancel on save detail dialog
> 6. 
> 
> Result: Cursor is lost.
> 
> Regression window:
> https://hg.mozilla.org/integration/mozilla-inbound/
> pushloghtml?fromchange=e8b32aaa2eec84c15f44cfd677142a0047481152&tochange=f3cc
> f0f50160e169389aa26a2d95ec225effcd3b
> 
> Suspect: a60d62c55e27	Jessica Jong — Bug 1316330 - Cancel delayed keypress
> events if last keydown was canceled. r=smaug

In addition to the above STR,
To reproduce on Nightly, it should be necessary to disable e10s.
Jessica and Olli, your bug 1316330 cause a regression. We lose the cursor when editing in Thunderbird, and as Alice outlines in comment #5 this can also be seen in FF alone.

We want to go to beta early next week, so I suggest to back your bug 1316330 out at least from Aurora *now*.
Flags: needinfo?(jjong)
Flags: needinfo?(bugs)
No need any addon.
I can reproduce the caret problem on Nightly53.0a1.
It reproduced with/without e10s.


Steps to reproduce:
1. Open http://www-archive.mozilla.org/editor/midasdemo/
2. type something
3. Click on "Inset link" icon 
5. Cancel on dialog
6. 

Result: Cursor is lost.
status-thunderbird52: affected → ---
status-thunderbird53: affected → ---
Component: Message Compose Window → Event Handling
Product: Thunderbird → Core
Version: Trunk → 52 Branch
[Tracking Requested - why for this release]: affected to web page as well as Thunderbird compose window
status-firefox51: --- → unaffected
status-firefox52: --- → affected
status-firefox53: --- → affected
tracking-firefox52: --- → ?
I found that, when event handling is suppressed, only focus events from a mouse or key are delayed, but all blur events are delayed [1]. So, in this case, when canceling the dialog, focus was fired first (the document was still under EventHandlingSuppressed() but aFocusMethod was false), then the delayed blur event was fired when event handling was unsuppressed. Hence, the final state was blurred.

I need to discuss with Olli how to fix this, so I think it's better to backout bug 1316330 from aurora first. Sorry for the inconvenience.

[1] http://searchfox.org/mozilla-central/rev/30fcf167af036aeddf322de44a2fadd370acfd2f/dom/base/nsFocusManager.cpp#2130
Flags: needinfo?(jjong)
(In reply to Jessica Jong [:jessica] from comment #10)
> I found that, when event handling is suppressed, only focus events from a
> mouse or key are delayed, but all blur events are delayed [1]. So, in this
> case, when canceling the dialog, focus was fired first (the document was
> still under EventHandlingSuppressed() but aFocusMethod was false), then the
> delayed blur event was fired when event handling was unsuppressed. Hence,
> the final state was blurred.
> 
> I need to discuss with Olli how to fix this, so I think it's better to
> backout bug 1316330 from aurora first. Sorry for the inconvenience.
> 
> [1]
> http://searchfox.org/mozilla-central/rev/
> 30fcf167af036aeddf322de44a2fadd370acfd2f/dom/base/nsFocusManager.cpp#2130

It's interesting that with a simpler test case, blur event is fired before the prompt is shown...
I agree, backing out bug 1316330 makes sense.
Flags: needinfo?(bugs)
Well, it's been backed out from Aurora, if you want to have it backed out from trunk, please take the appropriate steps.
Marking this fixed in 52 and 53 by
https://hg.mozilla.org/mozilla-central/rev/e674ae0954df
https://hg.mozilla.org/releases/mozilla-aurora/rev/4a274293090d
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox52: affected → fixed
status-firefox53: affected → fixed
tracking-firefox52: ? → +
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Jorg, Alice, I uploaded a follow-up patch in bug 1316330 to fix this. I'm wondering are there any other cases, besides from comment 8, that I can test to verify the patch? Thanks.
Flags: needinfo?(jorgk)
Flags: needinfo?(alice0775)
I've been experiencing the bug in TB, so I'm happy to try your patch in TB. Let's talk in the other bug.
Flags: needinfo?(jorgk)
Try build please.


And I think that an automate test should be provided.
Flags: needinfo?(alice0775)
(In reply to Alice0775 White from comment #17)
> Try build please.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8741a51c924fbeeda851087886bbdfd8efc8b2e6&group_state=expanded

> 
> And I think that an automate test should be provided.

Will add an automation test.
(In reply to Jessica Jong [:jessica] from comment #18)
> (In reply to Alice0775 White from comment #17)
> > Try build please.
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=8741a51c924fbeeda851087886bbdfd8efc8b2e6&group_state=e
> xpanded
> 

I tested with STR comment #8.
With enabled e10s:  It works as expected.
With disabled e10s: I can reproduce the caret problem.

https://hg.mozilla.org/try/rev/8741a51c924fbeeda851087886bbdfd8efc8b2e6
Mozilla/5.0 (Windows NT 10.0; WOW64; rv:54.0) Gecko/20100101 Firefox/54.0 ID:20170124042703
I can reproduce the problem, too, using the "Insert Link" on Midas. In TB the various "Insert" dialogues don't lose the cursor when applying the two patches from bug 1316330 in a local build.
(In reply to Jorg K (GMT+1) from comment #20)
> I can reproduce the problem, too, using the "Insert Link" on Midas. In TB
> the various "Insert" dialogues don't lose the cursor when applying the two
> patches from bug 1316330 in a local build.

Hmm, where you can reproduce the problem, is it a local build? with e10s or non-e10s? It seems that the fix only applies for e10s.

Thanks.
I can reproduce the problem in non-e10s with the try build you supplied. I was merely confirming comment #19. STR are to cancel the "Insert Link" on Midas and lose the cursor.
(In reply to Jorg K (GMT+1) from comment #22)
> I can reproduce the problem in non-e10s with the try build you supplied. I
> was merely confirming comment #19. STR are to cancel the "Insert Link" on
> Midas and lose the cursor.

I see, thank you both for help testing.
I'm working on a fix for e10s and non-e10s, will upload a new patch soon.
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.