Make WordPress Core

Opened 7 years ago

Closed 7 years ago

#40974 closed defect (bug) (fixed)

Updated text widget do not save text (when using paste)

Reported by: rinkuyadav999's profile rinkuyadav999 Owned by: azaozz's profile azaozz
Milestone: 4.8.1 Priority: normal
Severity: normal Version: 4.8
Component: Widgets Keywords: has-patch commit fixed-major
Focuses: Cc:

Description

Hi

I was testing updated text widget in 4.8. It saves text normally but when i switch visual to text 2-3 time and then

when i past text in visual and switch to text and press save button. it does not save text. Screenshot: https://i.gyazo.com/1e760893e6f830a3747204e2772a2407.mp4

Attachments (6)

40974.0.diff (1.0 KB) - added by westonruter 7 years ago.
40974.1.diff (1.4 KB) - added by westonruter 7 years ago.
https://github.com/xwp/wordpress-develop/pull/237/files/b0fc12e..032a888
40974.2.diff (1.4 KB) - added by azaozz 7 years ago.
40974.3.diff (3.2 KB) - added by azaozz 7 years ago.
40974.4.diff (5.4 KB) - added by westonruter 7 years ago.
40974.5.diff (6.0 KB) - added by westonruter 7 years ago.

Download all attachments as: .zip

Change History (36)

#1 @ketuchetan
7 years ago

Hi @rinkuyadav999,

I have tried to reproduce the issue as per your video. But, I am not facing this kind of issue.

Could you please give me instruction to reproduce the issue?

Thanks,

#2 @rinkuyadav999
7 years ago

Okay i have created a video for this issue : https://www.youtube.com/watch?v=q2ZeW2IxotI

System: Window 10 (64 bit), Firefox 53.0.3 (32 bit), WordPress 4.8, No plugin, default twenty seventeen theme.

Last edited 7 years ago by rinkuyadav999 (previous) (diff)

#3 @westonruter
7 years ago

@rinkuyadav999 I notice you're pasting the text into the widget. What about when you actually type text into the widget? It could be an issue with TinyMCE not picking up paste events properly to trigger a change.

#4 @westonruter
7 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 4.8.1

Actually, I can reproduce this issue. If you switch switch to the Text tab too quickly (in under a second specifically) after entering text into the Visual editor, then the text won't get synced into the underlying hidden due to the 1 second debouncing. That being said, it should also be syncing the TinyMCE value upon blur as well.

Here is the underlying code: https://github.com/WordPress/wordpress-develop/blob/4.8.0/src/wp-admin/js/widgets/text-widgets.js#L194-L198

#5 @westonruter
7 years ago

For some reason the blur event is not triggering when I click the Text tab from Visual.

#6 @rinkuyadav999
7 years ago

Yes, when i change changeDebounceDelay = 1000 to 1, it works but i think it creates other similar issues.

Past some text to text area in visual mode and click on text mode tab and save. it will save. Go back to Visual, remove pasted text, go to text tab and save. there will be removed text.

#7 @westonruter
7 years ago

The key to reproduce is to switch the tabs under the 1 second debounced NodeChange event handling.

@westonruter
7 years ago

#8 @westonruter
7 years ago

  • Keywords has-patch needs-testing added; needs-patch removed
  • Owner set to westonruter
  • Status changed from new to accepted

@rinkuyadav999 I think I see what is going on. When a user switches from the Visual to the Text tab, the TinyMCE editor is hidden and apparently in the process the blur event never triggers. I can see however that when the editor's hide method is called, it will do a save and trigger a SaveContent event. So if we switch from blur to SaveContent this seems to fix the issue. Please test 40974.0.diff.

@azaozz Does this make sense?

#9 @rinkuyadav999
7 years ago

Hi @westonruter

I have applied above patch. when i past text in visual and switch to text and press save button. it adds p tag and save value for first time. Now remove text in text mode and save. now go back to visual, past some text, go to text mode and save. there will be not pasted text (main issue again).

#10 @westonruter
7 years ago

  • Keywords reporter-feedback added

@rinkuyadav999 I'm having a very hard time reproducing this bug. It's hard for me to follow the steps. Could you create a video that shows the issue when you have the patch applied?

#11 @rinkuyadav999
7 years ago

Hi @westonruter

I have created a new video after applying patch 40974.0.diff : https://youtu.be/KxDlBAG0QsQ

First time, it adds p tag and second time same problem.

#12 @westonruter
7 years ago

  • Summary changed from Updated text widget do not save text to Updated text widget do not save text (when using paste)

@rinkuyadav999 thanks, I can reproduce that. So it seems specifically to be a problem when pasting content into the widget. It seems when paste happens, the editor's dirty state is not changed. So I've manually implemented that in 40974.1.diff. Please test.

@azaozz Please review, as there may be a TinyMCE defect here.

PR: https://github.com/xwp/wordpress-develop/pull/237

#13 @rinkuyadav999
7 years ago

  • Keywords reporter-feedback removed

Hi @westonruter

Yes, normally it works fine. But when we past text in visual and then switch to text, it creates issue.

I have applied 40974.1.diff and now it is working fine.

Thanks

#14 @westonruter
7 years ago

  • Keywords needs-testing removed
  • Owner changed from westonruter to azaozz
  • Status changed from accepted to assigned

Please add commit keyword if the changes here check out.

#15 @azaozz
7 years ago

Hm, this doesn't look right. There is also editor.on( 'hide', ... ) which AFAIK is used internally to save the content to the textarea. That's how it works in the "main" editor.

When testing editor.isDirty() also seems to work properly after pasting. I'll play with this a bit more.

@azaozz
7 years ago

#16 @azaozz
7 years ago

  • Keywords needs-testing added

In 40974.2.diff: use editor.on( 'hide', ... ) to run triggerChangeIfDirty() when the Text tab is clicked.

Seems to work well here. The editor.isDirty() is not yet set when the paste event fires, can pass the event and check it instead of setting it manually.

@rinkuyadav999 can you test if that works for you too please.

Last edited 7 years ago by azaozz (previous) (diff)

#17 follow-up: @rinkuyadav999
7 years ago

  • Keywords needs-testing removed

Hi @azaozz

This is working but when i past text in visual and then quickly switch to text and press save button, it remove text from text area and data does not save.

  1. When i past text in visual and wait for 2-3 seconds and then switch to text and press save button, it saves data perfectly.

Thanks

Version 0, edited 7 years ago by rinkuyadav999 (next)

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


7 years ago

#20 @westonruter
7 years ago

@azaozz do you have a recommendation on a way forward to resolve this?

#21 @jbpaul17
7 years ago

  • Keywords needs-refresh added

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


7 years ago

#23 in reply to: ↑ 17 @azaozz
7 years ago

Replying to rinkuyadav999:

This is working but when i past text in visual and then quickly switch to text and press save button, it remove text from text area and data does not save.

Yeah, can reproduce it too. Happens only on the Widgets screen not in the Customizer, and with any input type, typing does it too. It doesn't happen in the Customizer as it blocks saving. You cannot do a fast save there as the button is disabled. Have to wait for it to sync...

Happens because the editor is added on "fake" input fields, the "real" fields are hidden and sync from the fake fields on a delay. Not sure why the Text widget needs that, all seems to work better if the "real" fields are used directly.

If the fake fields are indeed needed, the way to fix this is to reduce the syncing delay or to introduce a way to sync with no delay.

Last edited 7 years ago by azaozz (previous) (diff)

@azaozz
7 years ago

@westonruter
7 years ago

#24 @westonruter
7 years ago

@rinkuyadav999 would you test 40974.4.diff?

#25 @azaozz
7 years ago

  • Keywords commit added; needs-refresh removed

40974.4.diff looks good.

@westonruter
7 years ago

#26 @westonruter
7 years ago

@azaozz I was still finding issues with previewing changes to the Text widget in the Customizer, namely when making a change in TinyMCE and then switching to the Text tab before the 1s debounced delay for checking if dirty.

Here's the change that fixes that issue: https://github.com/xwp/wordpress-develop/pull/240/commits/366c20afa49acfea1f0c34bae9a366718b6a7c69

Essentially we just keep around a second flag, in addition to the dirty state, to keep track of whether or not we need to trigger the change event for the sake of syncing.

This ticket was mentioned in Slack in #core by westonruter. View the logs.


7 years ago

#28 @westonruter
7 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 41094:

Widgets: Improve Text widget's reliability in syncing Visual tab's contents with Text tab and with hidden sync inputs.

Amends [40631].
Props azaozz, westonruter, rinkuyadav999 for testing.
See #35243.
Fixes #40974 for trunk.

#29 @westonruter
7 years ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Opening for 4.8.1.

#30 @westonruter
7 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 41095:

Widgets: Improve Text widget's reliability in syncing Visual tab's contents with Text tab and with hidden sync inputs.

Merges [41094] onto 4.8 branch.
Amends [40631].
Props azaozz, westonruter, rinkuyadav999 for testing.
See #35243.
Fixes #40974 for 4.8.1.

Note: See TracTickets for help on using tickets.