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

Status: VERIFIED FIXED

(Core :: DOM: Editor, defect)

Description

[Ekanan Ketunuti]

Attachment: 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.

Comment 1

[Darkspirit]

Confirmed in my Nightly 57 x64 20170802100302 @ Debian Testing (and not a Stylo bug).

Comment 2

[Boris Zbarsky [:bzbarsky]]

[Tracking Requested - why for this release]: Obvious web content regression.

Comment 3

[Paul]

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
```

Comment 4

[Paul]

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
```

Comment 5

[Ekanan Ketunuti]

(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?

Comment 6

[Paul]

Hmm, it seemed so - I did consistently get good/bad on either sides of that point...

Comment 7

[Ekanan Ketunuti]

Paul, could you please retest with

linux https://queue.taskcluster.net/v1/task/UM_SRw60QQGSFpgO4vXwNQ/runs/0/artifacts/public/build/target.tar.bz2
mac https://queue.taskcluster.net/v1/task/B1nXJU8-R_WZMiyKej3f8A/runs/0/artifacts/public/build/target.dmg
win https://queue.taskcluster.net/v1/task/XQliavmuRGiZ7ms2M57jvw/runs/0/artifacts/public/build/target.zip

the above suppose to be a last good build and the following suppose to be a first bad.

linux https://queue.taskcluster.net/v1/task/beuU7ZjmSfi6WWA__6kZxA/runs/0/artifacts/public/build/target.tar.bz2
mac https://queue.taskcluster.net/v1/task/Et2JfqZpReyKS9Dqs4uy0w/runs/0/artifacts/public/build/target.dmg
win https://queue.taskcluster.net/v1/task/aXlDG024TMqpirVsaZudcA/runs/0/artifacts/public/build/target.zip

Comment 8

[Paul]

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?

Comment 9

[Paul]

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

Comment 10

[Ekanan Ketunuti]

The build id is in about:buildconfig. Bug 1385525 is a regressor.

Comment 11

[Ekanan Ketunuti]

Yup, It's b) The reported changeset range by mozregression is wrong.

Don't trust mozregression. It always point to the wrong direction.

Comment 12

[Paul]

Seems like a neat tool though, I'll see if I can dig up a fix sometime :)

Comment 13

[William Lachance (:wlach)]

(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.

Comment 14

[(no longer active)]

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.

Comment 15

[Marcia Knous [:marcia]]

Tracking 56/57+. This is a very visible regression - thanks Ehsan for taking on the fix.

Comment 16

[(no longer active)]

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.

Comment 17

[(no longer active)]

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.

Comment 18

[(no longer active)]

Attachment: Call nsTextInputListener::EditAction() manually after using the non-transaction based editor code path for setting values of input controls

Comment 19

[Boris Zbarsky [:bzbarsky]]

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.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
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

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fe97dc518033

Comment 22

[Darkspirit]

(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.

Comment 23

[(no longer active)]

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.

Comment 24

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

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.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e7cfe34db2a0

Comment 26

[Ekanan Ketunuti]

verify++ 56.0b2