[Mobile] Fix omission of pasted image with some clipboard content#15016
[Mobile] Fix omission of pasted image with some clipboard content#15016
Conversation
|
|
||
| while ( wrapper && wrapper.nodeName !== 'P' ) { | ||
| wrapper = wrapper.parentElement; | ||
| wrapper = wrapper.parentNode; |
There was a problem hiding this comment.
Hi @ellatrix 👋 Do you foresee any issues with this single line change on the web side of things? My concern is, as we traverse upwards, that this could alter the behavior on web, because it may traverse all the way to the document, rather than stopping at the body. WDYT?
There was a problem hiding this comment.
Upon further inspection, I think it should exhibit the same behavior, as even in the case where it traverses slightly higher, it ultimately leaves wrapper undefined or null, which then adds the figure to the original parent (i.e. the only case where wrapper is used, it is guaranteed to be a <p> element, where the figure is removed from it, and prepended to it). Can you confirm if my understanding is correct?
There was a problem hiding this comment.
This change seems ok to me, but it would be great if you could write a test case that fails in master and doesn't fail here.
There was a problem hiding this comment.
Thanks for taking a look! :)
To clarify, do you mean a test case that fails on mobile? Maybe I should first clarify that this image omission only seems to occur on mobile (i.e. I tried pasting the same on web Gutenberg, and it successfully handles the image).
There was a problem hiding this comment.
Perhaps, yes. How do we prevent this from happening in the future?
ellatrix
left a comment
There was a problem hiding this comment.
Would love if the change could at least receive an inline comment.
Ideally, there should also be a test to prevent it from happening in the future, or some lint rules to prevent us from using some DOM APIs in the raw handling component.
Even better, it should be fixed so parentElement works on mobile too. I wouldn't be surprised if there are more instances of e.g. previousElementSibling. Does this work properly?
|
Do these all work? |
|
Hi @ellatrix 👋 Sorry for the delay in getting back to you on this. Thank you very much for pointing out the other code paths in the project where our mobile DOM implementation may be deficient. All of your suggestions are valuable, and I think it would be great if we can follow all of them. We've been working on a plan to bring tests that will hopefully improve this process, and I've also been looking into what is possible with the current implementation we are using: Once we better understand the limitations of the DOM implementation used in the React Native project, perhaps we can discuss the lint-rules that would be appropriate, and ensure that they don't interfere with development on |
|
I'm closing this, as we have now implemented the relevant level 4 DOM traversal functions via wordpress-mobile/gutenberg-mobile#1083, which fix the underlying issue. |
Description
This PR aims to fix an issue on mobile (wordpress-mobile/gutenberg-mobile#827) where pasted content from certain sources omits the images. Related comment: wordpress-mobile/gutenberg-mobile#617 (review), and a clipboard snippet of html that reproduces the issue on mobile here: wordpress-mobile/gutenberg-mobile#827 (comment).
How has this been tested?
Steps to test:
Screenshots (before: left, after: right)
Types of changes
This PR modifies
figure-content-reducer.jsto useparentNodeinstead ofparentElementwhen finding the containing<p>element (ancestor) surrounding an image element. BecauseparentElementis not implemented in the DOM for the mobile environment,<img>elements contained within<p>elements are not properly unwrapped, and are thus removed in a subsequent call tocleanNodeList.Checklist: