WordPress.org

Make WordPress Core

Opened 4 months ago

Closed 3 months ago

#40974 closed defect (bug) (fixed)

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

Reported by: rinkuyadav999 Owned by: 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 4 months ago.
40974.1.diff (1.4 KB) - added by westonruter 3 months ago.
https://github.com/xwp/wordpress-develop/pull/237/files/b0fc12e..032a888
40974.2.diff (1.4 KB) - added by azaozz 3 months ago.
40974.3.diff (3.2 KB) - added by azaozz 3 months ago.
40974.4.diff (5.4 KB) - added by westonruter 3 months ago.
40974.5.diff (6.0 KB) - added by westonruter 3 months ago.

Download all attachments as: .zip

Change History (36)

#1 @ketuchetan
4 months 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
4 months 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 4 months ago by rinkuyadav999 (previous) (diff)

#3 @westonruter
4 months 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
4 months 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
4 months ago

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

#6 @rinkuyadav999
4 months 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
4 months ago

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

@westonruter
4 months ago

#8 @westonruter
4 months 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
4 months 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
4 months 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
4 months 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
3 months 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
3 months 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
3 months 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
3 months 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
3 months ago

#16 @azaozz
3 months 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 3 months ago by azaozz (previous) (diff)

#17 follow-up: @rinkuyadav999
3 months 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.

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

Last edited 3 months ago by rinkuyadav999 (previous) (diff)

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


3 months ago

#20 @westonruter
3 months ago

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

#21 @jbpaul17
3 months ago

  • Keywords needs-refresh added

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


3 months ago

#23 in reply to: ↑ 17 @azaozz
3 months 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 3 months ago by azaozz (previous) (diff)

@azaozz
3 months ago

@westonruter
3 months ago

#24 @westonruter
3 months ago

@rinkuyadav999 would you test 40974.4.diff?

#25 @azaozz
3 months ago

  • Keywords commit added; needs-refresh removed

40974.4.diff looks good.

@westonruter
3 months ago

#26 @westonruter
3 months 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.


3 months ago

#28 @westonruter
3 months 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
3 months ago

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

Opening for 4.8.1.

#30 @westonruter
3 months 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.