1327674 - "Edit as HTML" feature in inspector stores all text edits for other nodes, so undo/Ctrl+Z is unpredictable

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P1)

Description

[arni2033]

>>>   My Info:   Win7_64, Nightly 49, 32bit, ID 20160526082509
STR_1:
1. Open url   data:text/html,<a>Firefox regressions </a><b>since version 28</b>
2. Open devtools -> inspector
3. Click on element <a> in markup. Press F2 to edit it as HTML. Press Ctrl+Z

AR:  Text field opened in Step 3 becomes empty
ER:  Text in the text field shouldn't change

Use case:
 I edited the text, and I want to go back to some previous version that was close to initial version of the text. To do that, I hold Ctrl+Z (it works in normal <textarea>s, but not in this text field).



STR_2:
1. Open url   data:text/html,<a>Firefox regressions </a><b>since version 28</b>
2. Open devtools -> inspector
3. Click on element <a> in markup. Press F2 to edit it as HTML. Press Escape to cancel
4. Click on element <b> in markup. Press F2 to edit it as HTML.
5. Type "x", then press Ctrl+Z several times

AR:
 Before the 1st pressing of Ctrl+Z text in codemirror instance is   "x<b>since version 28</b>"    (OK)
 After  the 1st pressing of Ctrl+Z text in codemirror instance is   "<b>since version 28</b>"     (OK)
 After  the 2st pressing of Ctrl+Z text in codemirror instance is   "<a>Firefox regressions </a>"
 After  the 3st pressing of Ctrl+Z text in codemirror instance is   ""
ER:
 After N-th pressing of Ctrl+Z (N>0) text in codemirror instance should be "<b>since version 28</b>"

Comment 1

[Patrick Brosset <:pbro>]

Thanks for filing, this is indeed making undo unpredictable and useless. We should look into this quickly.
@Brian: you implemented this feature a few years ago, would you happen to still know how undo was implemented then? Is this something codemirror manages and it wasn't configured properly?

Comment 2

[Brian Grinstead [:bgrins]]

I'm not sure if this is due to a recent change or not, but we are only ever creating a single html editor instance for the markup view: https://dxr.mozilla.org/mozilla-central/source/devtools/client/inspector/markup/markup.js#1422.

We should be able to clear the undo stack in the show() method on the HTMLEditor object: https://dxr.mozilla.org/mozilla-central/source/devtools/client/inspector/markup/views/html-editor.js#117.  Maybe editor.setClean() will do this, or we'll need to expose another CM API to do so.

Comment 3

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

This is an assigned P1 bug without activity in two weeks. 

If you intend to continue working on this bug for the current release/iteration/sprint, remove the 'stale-bug' keyword.

Otherwise we'll reset the priority of the bug back to '--' on Monday, August 28th.

Comment 4

[Julian Descottes [:jdescottes]]

Attachment: Bug 1327674 - clear the html-editor undo/redo stack when switching to a new node;

Review commit: https://reviewboard.mozilla.org/r/172638/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/172638/

Comment 5

[Julian Descottes [:jdescottes]]

(In reply to Emma Humphries ☕️ (she/her) [:emceeaich] (UTC-8) +needinfo me from comment #3)
> This is an assigned P1 bug without activity in two weeks. 
> 
> If you intend to continue working on this bug for the current
> release/iteration/sprint, remove the 'stale-bug' keyword.
> 
> Otherwise we'll reset the priority of the bug back to '--' on Monday, August
> 28th.

Thanks for the reminder! Just uploaded a patch for review and removed the stale-bug keyword.

(In reply to Brian Grinstead [:bgrins] from comment #2)
> I'm not sure if this is due to a recent change or not, but we are only ever
> creating a single html editor instance for the markup view:
> https://dxr.mozilla.org/mozilla-central/source/devtools/client/inspector/
> markup/markup.js#1422.
> 
> We should be able to clear the undo stack in the show() method on the
> HTMLEditor object:
> https://dxr.mozilla.org/mozilla-central/source/devtools/client/inspector/
> markup/views/html-editor.js#117.  Maybe editor.setClean() will do this, or
> we'll need to expose another CM API to do so.

And thanks bgrins for the investigation. setClean() doesn't clear the history but clearHistory() is already exposed via the CM_MAPPINGS so we can just reuse that.

Comment 6

[Julian Descottes [:jdescottes]]

Comment on attachment 8901163 [details]
Bug 1327674 - clear the html-editor undo/redo stack when switching to a new node;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/172638/diff/1-2/

Comment 7

[Patrick Brosset <:pbro>]

Comment on attachment 8901163 [details]
Bug 1327674 - clear the html-editor undo/redo stack when switching to a new node;

https://reviewboard.mozilla.org/r/172638/#review177944

Simple fix. And a nice integration test. Thanks!

Comment 8

[Julian Descottes [:jdescottes]]

Thanks for the review! Try at https://treeherder.mozilla.org/#/jobs?repo=try&revision=bb75043a5c1d7620980318b641c0354b17e0d73d

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1a2a9031bf0e
clear the html-editor undo/redo stack when switching to a new node;r=pbro

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/1a2a9031bf0e

Comment 11

[Sajedul Islam]

I have reproduced this bug with Nightly 53.0a1 (2017-01-01) on Windows 8.1 ! 

This bug's fix is Verified with latest Nightly 57.0a1!

Build   ID    20170828100127
User Agent    Mozilla/5.0 (Windows NT 6.3; WOW64; rv:57.0) Gecko/20100101 Firefox/57.0

Comment 12

[Md Maruf Rahman]

I have reproduced this bug with Nightly 53.0a1 (2017-01-01) on Ubuntu 16.04 LTS!

This bug's fix is verified with latest Nightly!

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

[bugday-20170830]