input boxes fail to register typing after contents cleared

RESOLVED FIXED in Firefox 56

Status

()

Core
Editor
P1
normal
RESOLVED FIXED
8 months ago
8 months ago

People

(Reporter: Harlan Lieberman-Berg, Assigned: Ehsan)

Tracking

({nightly-community, regression})

56 Branch
mozilla57
nightly-community, regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox55 unaffected, firefox56? fixed, firefox57+ fixed)

Details

Attachments

(3 attachments)

(Reporter)

Description

8 months ago
Created attachment 8907932 [details]
mwe.html

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20170913220121

Steps to reproduce:

When the value of an inputbox is cleared through javascript, the inputbox retains focus visually but no typing is registered.


Actual results:

Unfocusing and refocusing the inputbox will allow typing to be registered in that input box again.


Expected results:

A MWE is attached.
(Reporter)

Updated

8 months ago
Keywords: nightly-community, regression
(Reporter)

Comment 1

8 months ago
I bisected this problem to the following changeset:

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=9360c4975d3652abcae684e4af531fae7eb93789&tochange=c579ac37ac11a44b9af0c75720f519a40c439b19

Considering the content of the log, I think it is very probable that this is the commit which introduces the regression.  Adding the blocking.
(Reporter)

Updated

8 months ago
Blocks: 1385514
(Reporter)

Updated

8 months ago
status-firefox57: --- → ?
tracking-firefox57: --- → ?
(Reporter)

Comment 2

8 months ago
For the minimum working example, select the text box, type anything, hit enter, and then try to type again.

Updated

8 months ago
Has Regression Range: --- → yes
Component: Untriaged → Editor
Product: Firefox → Core

Updated

8 months ago
Status: UNCONFIRMED → NEW
status-firefox56: --- → affected
status-firefox57: ? → affected
Ever confirmed: true
Priority: -- → P1

Comment 3

8 months ago
ehsan, this is regression by bug 1385514.  Could you look this?  If no time, I will look this.
Flags: needinfo?(ehsan)
(Assignee)

Comment 4

8 months ago
I'll take a look.
Assignee: nobody → ehsan
Flags: needinfo?(ehsan)
(Assignee)

Comment 5

8 months ago
Created attachment 8908659 [details]
minimized testcase
(Assignee)

Updated

8 months ago
Attachment #8908659 - Attachment mime type: text/plain → text/html
(Assignee)

Comment 6

8 months ago
I didn't end up having time to look into it today, sorry.  I took half the day off sick.  :/
Likely too late for 56 now.
status-firefox55: --- → unaffected
status-firefox56: affected → fix-optional
status-firefox-esr52: --- → unaffected
tracking-firefox57: ? → +
Version: 57 Branch → 56 Branch
Should thsi maybe be a 56 blocker? Maybe back something out instead?
Flags: needinfo?(ryanvm)
Flags: needinfo?(ehsan)
(Assignee)

Comment 9

8 months ago
[Tracking Requested - why for this release]:

Yes, this needs to block 56.  The decision in comment 7 was wrong.  We should be able to very safely back out bug 1385514 from 56.
status-firefox56: fix-optional → affected
tracking-firefox56: --- → ?
Flags: needinfo?(ehsan)
OK, sounds like we should do that backout. This may affect many sites.

Comment 11

8 months ago
uplift
Fixed for Fx56 by backing out bug 1385514.

https://hg.mozilla.org/releases/mozilla-beta/rev/678bea28fe03 (FIREFOX_56b13_RELBRANCH)
https://hg.mozilla.org/releases/mozilla-release/rev/2bb9e6526ce9
status-firefox56: affected → fixed
Flags: needinfo?(ryanvm)
(Assignee)

Comment 12

8 months ago
The cause of the bug is that this branch ends up not being taken after bug 1385514:

https://searchfox.org/mozilla-central/rev/1c13d5cf85f904afb8976c02a80daa252b893fca/editor/libeditor/EditorBase.cpp#2747

SetTextImpl() only gets called by WillSetText(), so we can fix this either by backing out the original change or by removing that condition.  But I think the right thing to do is to do both actually, since there is no actual transaction any more, so it doesn't make much sense for SetTextImpl() to check whether subtransactions are supposed to modify the selection in the first place.
(Assignee)

Comment 13

8 months ago
Created attachment 8909597 [details] [diff] [review]
Don't prevent changing the selection in the editor when setting the value attribute
Attachment #8909597 - Flags: review?(masayuki)
Comment on attachment 8909597 [details] [diff] [review]
Don't prevent changing the selection in the editor when setting the value attribute

>diff --git a/editor/libeditor/tests/test_bug1399722.html b/editor/libeditor/tests/test_bug1399722.html
>+<script class="testbody" type="application/javascript">
>+SimpleTest.waitForExplicitFinish();
>+SimpleTest.waitForFocus(function() {
>+  let elm = document.querySelector("input");
>+
>+  elm.focus();
>+  synthesizeKey("A", {});
>+  synthesizeKey("B", {});
>+  synthesizeKey("VK_RETURN", {});

nit: Could you use "KEY_Enter" instead of legacy "VK_*"?
Attachment #8909597 - Flags: review?(masayuki) → review+
(Assignee)

Updated

8 months ago
Depends on: 1401225
(Assignee)

Comment 15

8 months ago
I need to land this with the patch I posted in bug 1401225 which is pending review.

Comment 16

8 months ago
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4193c11e97ae
Don't prevent changing the selection in the editor when setting the value attribute; r=masayuki

Comment 17

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4193c11e97ae
Status: NEW → RESOLVED
Last Resolved: 8 months ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.