Closed Bug 1386960 Opened 2 years ago Closed 2 years ago

Text in input is garbled/overlapping/corrupted/not rendered correctly

Categories

(Core :: Editor, defect, major)

57 Branch
defect
Not set
major

Tracking

()

VERIFIED FIXED
mozilla57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 + verified
firefox57 + verified

People

(Reporter: ananuti, Assigned: ehsan)

References

Details

(Keywords: regression)

Attachments

(2 files)

Attached image Capture.jpg
+++ This bug was initially created as a clone of Bug #1386222 +++

Build ID: 20170802100302 (bug 1386222 doesn't fix this issue)

Steps:

1. Open http://searchfox.org/mozilla-central/search?q=firefox-compact-dark.
2. Click at any link on that page.
3. Click Back button.

See text in input is garbled/overlapping/corrupted/not rendered correctly.
Flags: needinfo?(ehsan)
Confirmed in my Nightly 57 x64 20170802100302 @ Debian Testing (and not a Stylo bug).
[Tracking Requested - why for this release]: Obvious web content regression.
Assignee: nobody → ehsan
Flags: needinfo?(ehsan)
Testing this using `mozregression`... 

```
4:58.80 INFO: Got as far as we can go bisecting nightlies...
 4:58.80 INFO: Last good revision: 87824406b9feb420a3150720707b424d7cee5915 (2017-07-31)
 4:58.80 INFO: First bad revision: 51ffb9283f0c7c00e08eb8c39b33fbee218c370d (2017-08-01)
 4:58.80 INFO: Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=87824406b9feb420a3150720707b424d7cee5915&tochange=51ffb9283f0c7c00e08eb8c39b33fbee218c370d
```
Done. 

```
14:00.38 INFO: No more inbound revisions, bisection finished.
14:00.38 INFO: Last good revision: 2729aa6de0c1783055d21d914c142300051b4a6d
14:00.38 INFO: First bad revision: 274298ae4f2376884dec227fd1eea6c66efecd91
14:00.38 INFO: Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=2729aa6de0c1783055d21d914c142300051b4a6d&tochange=274298ae4f2376884dec227fd1eea6c66efecd91
```
(In reply to Paul from comment #4)
> Done. 
> 
> ```
> 14:00.38 INFO: No more inbound revisions, bisection finished.
> 14:00.38 INFO: Last good revision: 2729aa6de0c1783055d21d914c142300051b4a6d
> 14:00.38 INFO: First bad revision: 274298ae4f2376884dec227fd1eea6c66efecd91
> 14:00.38 INFO: Pushlog:
> https://hg.mozilla.org/integration/mozilla-inbound/
> pushloghtml?fromchange=2729aa6de0c1783055d21d914c142300051b4a6d&tochange=2742
> 98ae4f2376884dec227fd1eea6c66efecd91
> ```

Well, i'm pretty sure that a false positive one. i never trust mozregression.

Are you sure that a real regresion?
Hmm, it seemed so - I did consistently get good/bad on either sides of that point...
Confirmed with the mac builds (good is good, bad is bad)

Both say 56.0a1 (2017-07-31) (64-bit) in the 'About Nightly' page though, is there a way to get a finer build number from within the browser?
Those patches definitely don't look like they could've caused this.

Possibilities: 

a) The builds are wrongly tagged
b) The reported changeset range by mozregression is wrong
c) Something outside the code (Build process?) changed
The build id is in about:buildconfig. Bug 1385525 is a regressor.
Yup, It's b) The reported changeset range by mozregression is wrong.

Don't trust mozregression. It always point to the wrong direction.
Seems like a neat tool though, I'll see if I can dig up a fix sometime :)
(In reply to Ekanan Ketunuti from comment #11)
> Yup, It's b) The reported changeset range by mozregression is wrong.
> 
> Don't trust mozregression. It always point to the wrong direction.

This isn't true, I and others get correct bisection results from mozregression all the time. 

Please file bugs when you get an unexpected result like this (in testing/mozregression) and attach a full log of the console output. I filed bug 1387424 to track down the issue here.
This is certainly a regression from bug 1385525.  I actually ran a local mozregression job last night which took me right to bug 1385525, so there is nothing wrong with mozregression.  :-)  If you make one mistake in one of the bisection steps though the rest of the bisection will go into the weeds and will be quite misleading, that's true for any bisection tool and not the fault of mozregression.

Anyway, I'm gonna dig into this today.
Tracking 56/57+. This is a very visible regression - thanks Ehsan for taking on the fix.
So when things start to go wrong, we have a call to HTMLInputValue::SetValue() with "firefox-compact-dark".  We get to <https://searchfox.org/mozilla-central/rev/30a47c4339bd397b937abdb2305f99d3bb537ba6/dom/html/HTMLInputElement.cpp#3027> and call SetValueChanged(true) which calls UpdateState() and sends notifications, but this is before we have set our value, so at this time we get an intrinsic state that states that our placeholder is visible because our value is empty.

Then we get to <https://searchfox.org/mozilla-central/rev/30a47c4339bd397b937abdb2305f99d3bb537ba6/dom/html/nsTextEditorState.cpp#2641> and we call mTextListener->SetValueChanged(true), which is intended to signal this code <https://searchfox.org/mozilla-central/rev/30a47c4339bd397b937abdb2305f99d3bb537ba6/dom/html/nsTextEditorState.cpp#1079> to send out notifications after the value has been set as well, but since we don't use transactions any more, the editor action listener isn't invoked any more and this code doesn't run, and we are left with our intrinsic state thinking that the placeholder is left shown.

Needless to say, the next time notifications are sent for any reason, things will fix themselves again.
So I spent quite a bit of time trying to create a minimized test case for this to no avail.  The issue is that when I try to create a simple test case, no matter what I try, the editor ends up with a bogus node under the DOM tree, and the TextEditor::SetText() call actually ends up creating a transaction when it tries to delete the bogus node, and then we end up with a placeholder transaction due to https://searchfox.org/mozilla-central/rev/30a47c4339bd397b937abdb2305f99d3bb537ba6/editor/libeditor/EditorBase.cpp#1049.

However the fix to the bug itself is pretty simple, all we have to do is call EditAction() manually because it won't be done for us by the transaction manager, so I will probably just do that for now.
OS: Windows 10 → All
Hardware: x86 → All
Comment on attachment 8893994 [details] [diff] [review]
Call nsTextInputListener::EditAction() manually after using the non-transaction based editor code path for setting values of input controls

This is going to also do all the undo/redo item bits, right?  Ca we skip those and just do the SetValueChanged()/OnValueChanged bits?  Basically, add a new function that we use both here and from EditAction.

r=me with that.
Attachment #8893994 - Flags: review?(bzbarsky) → review+
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe97dc518033
Call nsTextInputListener's callback manually after using the non-transaction based editor code path for setting values of input controls; r=bzbarsky
https://hg.mozilla.org/mozilla-central/rev/fe97dc518033
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
(In reply to Darkspirit from comment #1)
> Confirmed in my Nightly 57 x64 20170802100302 @ Debian Testing (and not a Stylo bug).

Verified fixed in Nightly 57 x64 20170809100326 @ Debian Testing.
Status: RESOLVED → VERIFIED
Comment on attachment 8893994 [details] [diff] [review]
Call nsTextInputListener::EditAction() manually after using the non-transaction based editor code path for setting values of input controls

Approval Request Comment
[Feature/Bug causing the regression]: bug 1385525
[User impact if declined]: See comment 0 for example.  This can impact <input> elements which have a palceholder attribute.
[Is this code covered by automated tests?]: Unfortunately no, I was unsuccessful in creating a minimized test case for it.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: Yes, and comment 0.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: Not that risky, since the code change is well understood.  However I'd rather land it on beta sooner than later so that we have time to deal with any potential fallout, especially now that the opportunity of dealing with this regression on Aurora doesn't exist any more.
[Why is the change risky/not risky?]:
[String changes made/needed]: None.
Attachment #8893994 - Flags: approval-mozilla-beta?
Comment on attachment 8893994 [details] [diff] [review]
Call nsTextInputListener::EditAction() manually after using the non-transaction based editor code path for setting values of input controls

Fix for a regression very noticeable to users, let's uplift for beta 2.
Attachment #8893994 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
verify++ 56.0b2
You need to log in before you can comment on or make changes to this bug.