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 | 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)
Change History (36)
#2
@
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.
#3
@
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
@
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
@
7 years ago
For some reason the blur
event is not triggering when I click the Text tab from Visual.
#6
@
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
@
7 years ago
The key to reproduce is to switch the tabs under the 1 second debounced NodeChange
event handling.
#8
@
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
@
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
@
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
@
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
@
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.
#13
@
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
@
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
@
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.
#16
@
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.
#17
follow-up:
↓ 23
@
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.
- 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
#18
@
7 years ago
Testing video: https://youtu.be/ReV6eZZhkp4
This ticket was mentioned in Slack in #core by jeffpaul. View the logs.
7 years ago
This ticket was mentioned in Slack in #core by jeffpaul. View the logs.
7 years ago
#23
in reply to:
↑ 17
@
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.
#24
@
7 years ago
@rinkuyadav999 would you test 40974.4.diff?
#26
@
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.
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,