Further Attachments Overhaul

[effone]

• Make use of "jGrowl" error in place of native alert();
• Frontend validation for Add / Update attachment for file availability.
• Post.checkAttachments considers only php_max_file_uploads and not $mybb->settings['maxattachments'] resulting having frontend validation failure triggering backend return errors only.
• attachment_too_big_upload error string says "bytes" while representing figure in MB.
• Fix wrong calculation of file size in some browsers (lower version FF, say...).
• Inappropriate form element selection through Init: Quick Search submission triggers Post.checkAttachments;
• Effectively use the "Update Attachment" functionality. Currently if the user attaches a file with a filename that is not already attached; clicking on "Update Attachment" button silently adds the new file in place of throwing error. (no-js fallback improvement)
• Fix usage of undefined language variable post_fetch_error in error messages (this is only available for forumdisplay and not applicable for attachment handling)
• Instantly upload attachments as user selects file/s (Resolves Enhancement: Remove double confirmation of upload #3566)
• Obtain consent from users for updating attachment/s in a scenario when user selects file/s with filename/s that is already attached (instant upload check).
• Remove update attachment button when no file is attached (AJAX remove event).
• Remove add attachment button when maximum allowed files are attached (AJAX add event).
• Update attachment usage on AJAX response.
• Implement AJAX file upload progress bar.
• Implement a file drop zone to support drag-and-drop upload.

[Eldenroot]

One idea - in phpbb if you insert the image into the post -> there is a text next to the picture (in attachment box) that this picture was inserted into the post and the insert button is hidden. Maybe a good idea to discuss :) Next really cool feature is just drag the file from attachment directly into the post and it is inserted to the desired area.

Also the file size validation is before upload, so you are informed that the selected file is too big.

Small features but really useful in phpbb.

[effone]

File size and file number validations are already in place.
Regarding insert: what if user wants to insert the attachment to the post multiple times? In that case hiding insert button is pointless.

[Eldenroot]

Ok, but lets think about it, this is an overhaul, so make it perfect :) the besf way is to check other forum softwares and take ideas from them...

[Sama34]

I agree the insert button should be left as is, though the reasoning behind hiding it is valid.

However, replacing it with a drag and drop of attachments would be nice.

[effone]

However, replacing it with a drag and drop of attachments would be nice.

Point well taken; however it might be a good idea to consider that in a separate PR to keep things clean and ease of PR review. This one is already huge by now.

[euantorano]

I would certainly find it easier to review a separate PR for drag and drop :D

…

On Sat, 19 Dec 2020, at 05:59, Debabrata Trivedi wrote: @effone <https://github.com/effone> requested your review on: #4195 <#4195> Further Attachments Overhaul. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#4195 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFW24I36PGPPJSGPHVKJT3SVQ6LNANCNFSM4U57BT7Q>.

[Eldenroot]

Yep, drag and drop would be amazing :)

[effone]

Note to theme developers:
There are significant changes in three templates: post_javascript, post_attachments_new & post_attachments_attachment.
You must implement the changes made in the mentioned templates in order to get the new enhancements working properly.

[Eldenroot]

What about click and drag already uploaded image and add it into the post.

This is how it works in phpbb and it is really useful. Also small icon or symbol for images which are added into the post would be nice. Just my ideas, no requests... Good job anyway, this PR will improve attachment system a lot

[effone]

I worked for what you said. But you know, if there is SCEditor; there is pain. This will take some times and digging.
I have done everything here what I did in my AJAX file upload plugin which you have already tested a year back @Eldenroot . As we are talking about modern UI; I felt this should be in core. I maintained minimum template changes.

If this gets rejected I will release my plugin :P

[euantorano]

I don’t think there’s any chance of it getting rejected :) Just got to review the code to make sure there’s nothing that jumps out at us.

…

On Fri, 25 Dec 2020, at 19:17, Debabrata Trivedi wrote: I worked for what you said. But you know, if there is SCEditor; there is pain. This will take some times and digging. I have done everything here what I did in my AJAX file upload plugin which you have already tested a year back @Eldenroot <https://github.com/Eldenroot> . As we are talking about modern UI; I felt this should be in core. I maintained minimum template changes. If this gets rejected I will release my plugin :P — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#4195 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFW24O7WHRDLWRWN33MVDTSWTQMFANCNFSM4U57BT7Q>.

[Eldenroot]

@effone I know, and this is great! This will make mybb a little bit closer to friendlier gui and follow standards...

Sceditor is really bad, I am really happy that I xan use ckeditor from martec - plugin rineditor because it is great. I hope that mybb 1.9 will not use sceditor anymore, it is a dead project without a lead dev, we need another alternative-maybe why not ckeditor?

Also we maybe have misunderstood a little bit, in phpbb when you upload an image, you can take this image and put it into the text directly from attachment list just with mouse drag. It is added in img tag. I have a working phpbb local copy, I can give you an access tommorow just to try. It is not a core feature, it is just a cherry pick.

I hope that all will be fine and we will get this PR into next mybb release. Thy

[Sama34]

Hi, I found several issues with updating a board with this code.

• Dragging and dropping a single file results in it being uploaded 4 times.
• Using the dialog box, once one single file is selected and the dialog closed the dialog opens back, as if the first selection didn't occur. The file selected the first time is ignored and the second file is uploaded multiple times as well.
  
• Since I have a limit of 6 files it appears the plugin attempts to keep uploading the second files (previous point), which causes a jGrowl error I have no way to bypass.
  
• When updating an attachment, I receive the message you describe, but all previous attachments get deleted and the new file inserted.
  
• Add attachment button is never displayed, regardless of the post having zero attachments (unsure if this is expected when the drag and drop feature is working)
• While uploading invalid files you get double (four if dragging and dropping) jGrowl error messages (expected as described in points above)
• I didn't see any progress bar.

[Sama34]

I did a fresh install with your branch and nothing of the above mentioned points seems to exists. But I think more people should test by updating to find potential issues.

Regarding this fresh install I only found two suggestions, the first is this error being too complex:

I think it should be simplified somehow. Regular users don't care above much of what it says even if they understand it. Maybe we can send errors to the console in case they the developers tools?

The second, when selecting files to upload after reaching the limit, if any selected file is meant to be updated the user gets a maximum files exceeded error, completely ignoring files that could be updated.

[effone]

@Sama34 All the points mentioned by you in your first post seems to be non-reproducible for me. Although this can happen only in case of a faulty cache copy of the old post.js and / or improper required template edits. Kindly note that the file detection heavily relies on the required template edits.

Regarding your second post:
The backend error message was there like that only. I didn't alter it, only the message appears in jGrowl now.

The situation pointed by you regarding uploading multiple files of mixed state is valid. However; it was the easiest method to deal with the situation to block the upload altogether as you may know the FileList is read only. I tried to exercise with FileReader API to manipulate and alter the existing input but the fact is Base64 encoding a file will increase its size by around 30% which is not desired as the size is one of the considerable factor of our attachment system. Any better idea is welcome while I rework on it further to explore possibilities ...

The Add Attachments and Update Attachments buttons are hidden by design as they are not required while upload zone is present. If someone choose not to implement upload zone the buttons will appear and change their state as per the availability of the attachments with proper validation, but that's kinda pointless too as the auto upload trigger is functional.

[Eldenroot]

@Sama34 - please could you review again? I have applied to my board with latest changes so we will see :) thx!

[effone]

I guess he has done his review and provided all required feedbacks.

[Starpaul20]

I've done some testing and all looks good.

[effone]

Thanks for testing Paul.

[lairdshaw]

@effone, this is some great work, and a big improvement on what came before. Kudos, man.

That said, in my (limited) testing, I've encountered a few (minor) issues. I have not (yet?) though dug into the code to try to identify what's causing them, let alone to offer any solutions. In any case, here they are:

• The title of the dialogue box that pops up when updating an attachment is literally __title__ .
• The (textual descriptions of the) sizes of two files I'd attached became swapped on clicking "Preview Post".
• At one point (on Firefox), the drop zone message got stuck on "Release to initiate upload" even though I wasn't dragging a file, but I'm not sure what the exact conditions were at the time, and haven't been able to reproduce this issue.
• (At least on Firefox) When dragging a file into the drop zone, if the mouse cursor is hovered exactly over the "Release to initiate upload" text, that text reverts to "Click or drop some files here to upload...", and releasing the mouse button over that text results in a full page reload of the file at its filesystem URL.

[effone]

@lairdshaw
Your first point is depending on this:
#4198
Apply that PR first.

I couldn't produce the rest of the points. I'll try again to look in deep.

[lairdshaw]

Your first point is depending on this:
#4198
Apply that PR first.

Ah, thanks. I'll simply trust you that that PR fixes this issue. Seems plausible based on skimming its diff.

I couldn't produce the rest of the points.

OK. I've checked them again. Turns out that this seems to have been a misdiagnosis:

• The (textual descriptions of the) sizes of two files I'd attached became swapped on clicking "Preview Post".

It seems that the appearance of "swapped image sizes" may simply have been due to the Image Auto-Resizer plugin's auto-resizing of the images. I'm not 100% sure of this as an explanation but it's my best guess.

Re this:

At one point (on Firefox), the drop zone message got stuck on "Release to initiate upload" even though I wasn't dragging a file, but I'm not sure what the exact conditions were at the time, and haven't been able to reproduce this issue.

I wouldn't worry about it, since it only occurred once and hasn't recurred since.

This, however...

(At least on Firefox) When dragging a file into the drop zone, if the mouse cursor is hovered exactly over the "Release to initiate upload" text, that text reverts to "Click or drop some files here to upload...", and releasing the mouse button over that text results in a full page reload of the file at its filesystem URL.

...remains an issue for me.

So, that's three out of four down. Progress...

[effone]

Hmm, doesn't happen with webkit. Will check placing stopPropagation().

[effone]

...remains an issue for me.

Alright, so further investigation reveals that there is a strange known issue remaining with Firefox. Exactly what we are facing.
https://stackoverflow.com/questions/14194324/firefox-firing-dragleave-when-dragging-over-text

I am gonna push a patch in a few.

[effone]

@lairdshaw Great catch.
I have pushed a patch commit. Can you please cross-check and reconfirm?

[lairdshaw]

@lairdshaw Great catch.
I have pushed a patch commit. Can you please cross-check and reconfirm?

Looks good to me, @effone. Seems to be fixed. Very pleased also to see that there's a graceful downgrade in this PR on disabling Javascript. Excellent work.

[effone]

Thanks for the review.
Hope this is multi-tested and ready to merge? @euantorano

[euantorano]

Yep, good to merge! I'm going to squash and merge though if that's okay?

[effone]

Sure thing. Take your time Boss. Just wanted to let you know that its ready in all aspects.