search

wordpress-mobile / WordPress-Android

WordPress for Android
http://android.wordpress.org
GNU General Public License v2.0
2.92k stars 1.3k forks source link

Drag and Drop into the editor in split-view mode #4397

Closed hypest closed 7 years ago

hypest commented 7 years ago

Fixes #4378

To test:

Same procedure is supported for dropping images, though a gallery app that supports drag-and-drop needs to be installed.

Needs review: Hey @aforcier , want to have a look at this one? Thanks!

/cc @oguzkocer

hypest commented 7 years ago

A known issue at this point is that the cursor movement when dragging seems sluggish on a device. Some throttling/debouncing of the JS cursor movement is probably needed.

aforcier commented 7 years ago

Following the steps and dragging a URL from Chrome into the editor is just inserting null for me into the editor. This happens on a Nexus 9 API 7.0, both on a physical device and an emulator. Looks like a problem with getHtmlText() - the clip data is not null when ACTION_DROP is triggered.

aforcier commented 7 years ago

Same procedure is supported for dropping images, though a gallery app that supports drag-and-drop needs to be installed.

Do you have a good example of one to try? Looks like Google Photos isn't one of them.

hypest commented 7 years ago

Following the steps and dragging a URL from Chrome into the editor is just inserting null for me into the editor.

Strange, that's exactly what I fixed with https://github.com/wordpress-mobile/WordPress-Android/pull/4397/commits/48602cafe6d9c28c3a5a5237b288b61a446f1394 I'll have another look. Thanks!

Do you have a good example of one to try?

I'm afraid not. On my Samsung Tab S2, the stock gallery app does support it, though I didn't quickly find something relevant for the emulator. I'll keep looking and let you know if I find one!

oguzkocer commented 7 years ago

I have been testing this with Android 7 emulator (x86-64 image) and it's been working fine. You can also drag and drop into the link pop-up (the one where you enter the link & text). I think that's more valuable than just adding it as a text.

A small tip for testing: I was quite frustrated because I was unable to drag the text from Chrome no matter how many times I tried. Finally, I figured out that you need to tap and hold the text for a second or so before dragging.

Also, you can drop the text in multiple places, paragraph endings mostly, which is pretty useful since you don't always want to drop it to where the carret is.

hypest commented 7 years ago

You can also drag and drop into the link pop-up (the one where you enter the link & text).

Nice find!

hypest commented 7 years ago

Also, I'm sure we can probably do more "smart" stuff after this basic functionality gets reviewed and merged. Like detecting links and dropping them directly as such. I guess that's what you're basically implying @oguzkocer .

oguzkocer commented 7 years ago

Also, I'm sure we can probably do more "smart" stuff after this basic functionality gets reviewed and merged. Like detecting links and dropping them directly as such. I guess that's what you're basically implying @oguzkocer .

Actually I wasn't but it's a good idea. Since we are testing it from the Chrome's url bar, I just wanted to try linking things and it does work right now. It seems like you can't drag any text from Chrome though. I am guessing it needs to be implemented in Chrome rather than our app, right?

I've tried to drag images from Chrome & the stock gallery app without success.

hypest commented 7 years ago

I am guessing it needs to be implemented in Chrome rather than our app, right? I've tried to drag images from Chrome & the stock gallery app without success.

Yeap, AFAICT, the dragging part is a responsibility of the originating app.

aforcier commented 7 years ago

Following the steps and dragging a URL from Chrome into the editor is just inserting null for me into the editor. This happens on a Nexus 9 API 7.0, both on a physical device and an emulator. Looks like a problem with getHtmlText() - the clip data is not null when ACTION_DROP is triggered.

Note: this also happens to me when dragging text, e.g. from a Play Store app description - the editor just inserts null.

hypest commented 7 years ago

Note: this also happens to me when dragging text, e.g. from a Play Store app description - the editor just inserts null.

Reproduced that, thanks! Will work on it. EDIT: seems this is not consistently happening.

aforcier commented 7 years ago

There's an issue with dragging and dropping text into the editor content field when it's empty: the text doesn't get wrapped in paragraph tags. This doesn't have immediate consequences, but when more text is added later it can cause paragraph spacing issues, autocorrect issues, cursor issues, etc.

I've been meaning to take the time to clean up the ZSSRichTextEditor.js methods a little to offer a better 'API' for insertions. Here's what I suggest: add a new method to ZSSEditor which wraps the text in paragraph tags if necessary (content field is selected, and it's empty):

ZSSEditor.insertText = function(text) {
    var focusedField = ZSSEditor.getFocusedField();
    if (focusedField.isMultiline() && focusedField.getHTML().length == 0) {
        text = Util.wrapHTMLInTag(text, ZSSEditor.defaultParagraphSeparator);
    }

    ZSSEditor.insertHTML(text);
};

And call that from EditorFragment instead of calling insertHTML directly.

aforcier commented 7 years ago

Following the steps and dragging a URL from Chrome into the editor is just inserting null for me into the editor. This happens on a Nexus 9 API 7.0, both on a physical device and an emulator. Looks like a problem with getHtmlText() - the clip data is not null when ACTION_DROP is triggered.

It turned out I was experiencing this bug, but with API 24. Applying the solutions from that thread and restarting everything eventually fixed it, and I'm not getting the null issue anymore. 🎉

aforcier commented 7 years ago

When dragging a text selection with line breaks, dropping doesn't work and I get this error:

org.wordpress.android E/WordPress-EDITOR: Uncaught SyntaxError: Invalid or unexpected token -- From line 1 of

This is happening because the \ns need to be escaped. Wrapping the calls to insertHTML (or insertText) in Utils.escapeHtml() should take care of this issue.

hypest commented 7 years ago

@aforcier , I've addressed your comments, thanks for the Utils.escapeHtml() proposal! I'm now working on making the editor update on the spot to more visually show the line change (it only does it when exiting and re-entering the editor atm).

Let me poke your brain @aforcier : is it OK to use Formatter.htmlToVisual() to transform the dropped text before doing the final .insertHTML()? Goal is to have the dropped text be visualy "correct" (albeit, without any bold/etc formatting).

aforcier commented 7 years ago

Let me poke your brain @aforcier : is it OK to use Formatter.htmlToVisual() to transform the dropped text before doing the final .insertHTML()? Goal is to have the dropped text be visualy "correct" (albeit, without any bold/etc formatting).

Yes, that's fine to use if it's improving the formatting of the dropped text. :+1:

hypest commented 7 years ago

Ready for another pass @aforcier , thanks!

hypest commented 7 years ago

An issue I found now is that the photo drop fails if the app doesn't have the permission to access storage (to download media there first). Edit: fixed with 23b98c5.

hypest commented 7 years ago

Another permission related issue I've encountered is when dropping image files from "Downloads" or similar file manager. The app accepts images but cannot handle the content URIs correctly.

aforcier commented 7 years ago

I took a look at image dropping with the sample app you modified (thanks!), it works great!

I encountered one issue: dragging an image into the title field will 'work' and insert the image, when it shouldn't. We should probably abort the image drop if mIsFormatBarDisabled.

hypest commented 7 years ago

Heads up @aforcier , with 6b3ac04 you can now drag and drop (single) photos via the "Downloads" app in the emulator. I'll work on dropping of multiple selected photos.

aforcier commented 7 years ago

Unfortunately the approach in https://github.com/wordpress-mobile/WordPress-Android/pull/4397/commits/74cc72ee87adb5aed58ee2dfdedf5ae93258c1b9 breaks the cursor positioning when the fields are empty on API < 19:

broken-cursor-api18

This is due to some CSS issue with ::before on those API levels (ref).

IMO it's not worth the trouble to work around this. As a somewhat separate issue, I'm also not sure it's necessary to have visual feedback to show that dragging photos to the title field isn't allowed (which would also eliminate any need for changing the padding).

If we decide to keep some form of visual feedback, we should get some design input on that.

hypest commented 7 years ago

Unfortunately the approach in 74cc72e breaks the cursor positioning when the fields are empty on API < 19:

Aha, I see. I'll revert the change.

...I'm also not sure it's necessary to have visual feedback to show that dragging photos to the title field isn't allowed...

If we decide to keep some form of visual feedback, we should get some design input on that.

I'll demote this to a Toast so the user will at least have a clue why the drop wasn't successful. I wouldn't want us to block this on a design issue. Thanks for the feedback!

hypest commented 7 years ago

Another issue found: the code currently allows dropping images while in HTML mode. We shouldn't allow that. I'll work on this next.

hypest commented 7 years ago

@aforcier , ready for another pass. Thanks!

aforcier commented 7 years ago

I'm getting 'Warning: not all dropped items are supported!' messages whenever I try to drag-and-drop images from a gallery app, and the image doesn't get added. It looks like contentResolver.getType(uri) is returning null. This doesn't look like the same issue I ran into earlier with the API 24 SDK, since text and download manager drag+drop is working fine.

hypest commented 7 years ago

I'm getting 'Warning: not all dropped items are supported!' messages whenever I try to drag-and-drop images from a gallery app, and the image doesn't get added

What a regression :(. Will work on it!

hypest commented 7 years ago

So, I opted for not supporting dropping web URLs after all as those are inefficient to check whether they really point to images or not. We would need to initiate the HTTP GET to retrieve the mimetype. The downside of the change is that the custom gallery app I shared before cannot be used any more (it drops images as web URLs). In the end, with 08bbdb1, only simple text, html and "content://" image files are supported. Since the relevant code lines were touched, please try again any scenario you can @aforcier . Thanks!

Ready for another pass :)

aforcier commented 7 years ago

Full round of re-testing:

We're good to go! One thing I might suggest is to open a new issue in the editor repo for supporting dropping web URLs, we might want to revisit that in the future and perhaps find a good way to implement it (perhaps the drag-drop feature will be improved to pass some context when a web url is dragged, so we don't have to do a HTTP GET).

Great work on this @hypest. :shipit:

hypest commented 7 years ago

Thanks for the detailed review on this @aforcier ! Appreciate it and that little tests list you added is awesome!

Great suggestion about opening a ticket for supporting dropping web URLs. Will do! Done: https://github.com/wordpress-mobile/WordPress-Editor-Android/issues/441

Again, thanks and happy drag-and-dropping :smiley: