Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Discussion] Add Unique HTML IDs to Comment Forms to Fix Image Upload Cross-Wiring Issues #8926

Closed
2 tasks done
noi5e opened this issue Jan 3, 2021 · 0 comments · Fixed by #8987
Closed
2 tasks done
Assignees
Labels
feature explains that the issue is to add a new feature outreachy

Comments

@noi5e
Copy link
Contributor

noi5e commented Jan 3, 2021

Description

Creating a fresh issue page for this complex bug that appears in many forms. See #8670 and #8478 for two examples of this bug in action. Also see current PR #8897 (awaiting review) that fixes a small piece of this bug.

Comment editor functions are cross-wired:

  • Click on a main comment form, and upload an image.
  • Open an edit comment form and upload an image.
  • Expected Behavior: The image link should go into the edit comment form.
  • Actual Behavior: Instead, the image link ends up in the main comment form (see image). The image upload was 'cross-wired' into the main comment form.

103166998-428f5900-47dc-11eb-9441-b46ecf6af58a

Why This Happens

The JS scripts in /views/comments/_edit.html.erb and /app/assets/dragdrop.js have variables like $D.selected and $E.textarea that point toward the comment form that the user last interacted with. However, these are often pointing to the wrong form. They need to be updated every time the user takes particular actions: (toggling a form open, entering text, uploading an image).

Comment Forms: Uploading Images

Edit Comment

  • A: This div.dropzone wraps an input#fileinput.
  • B: This entire section is wrapped in a div#c1234div.
    • The user can drag-and-drop images for upload here.
      • On edit comment forms, this triggers embedded JS here to handle the upload.
      • On main comment forms (at the bottom of the page), and reply forms, this will trigger the code in dragdrop.js mentioned above.
  • C: There's another input#fileinput here, similar to the one above but distinct.
    • The user can click here to select an image for upload, which like the one above skips 'drop' event handlers, and goes straight to jQuery's fileupload handlers.

Why This Matters

Ideally, we could write code that sets $D.selected to the appropriate textarea whenever the user takes one of the actions above. To reiterate, $D.selected is a kind of place marker for the editor to know where to put the user's changes (an image link, bold text, etc.)

In practice, $D.selected would rely on e.target from a click or drop event handler. However, logging e.target shows that it's often inaccurate. In the example below, I upload an image in comment 22, and e.target (and the image upload) points toward comment 7!

Unclosed  div  tag

Potential causes for this:

  • multiple inputs and div class='dropzone's with the same IDs and classes running around.
  • in my other experiments, multiple e.targets will be logged to the console sometimes. so it could also have something to do with the way the event handlers are being written, and how the click event bubbles up through the DOM.

Next Steps

  • I committed some changes in Add Unique HTML IDs to .dropzones, #fileinputs #8927 setting unique IDs on all of the elements diagrammed above. I have a feeling that this will solve the problem of e.target being inaccurate. Another fix could potentially be rewriting the event handlers so that they're only searching for click or drop events within certain parts of the DOM tree.
  • Once unique IDs are set up, I should be able to write the eventHandlers to set $D.selected whenever a user clicks to upload images. [PR open: Fix CLICK-to-Upload Image Cross-Wiring Issues #8987]

(This issue is part of the larger Comment Editor Overhaul Project with Outreachy. Refer to Planning Issue #9069 for more context)

@noi5e noi5e added feature explains that the issue is to add a new feature outreachy labels Jan 3, 2021
@noi5e noi5e self-assigned this Jan 3, 2021
@noi5e noi5e changed the title Feature: Add Unique HTML IDs to Comment Forms to Fix Image Upload Cross-Wiring Issues Issue Add Unique HTML IDs to Comment Forms to Fix Image Upload Cross-Wiring Issues Jan 3, 2021
@noi5e noi5e changed the title Issue Add Unique HTML IDs to Comment Forms to Fix Image Upload Cross-Wiring Issues Issue: Add Unique HTML IDs to Comment Forms to Fix Image Upload Cross-Wiring Issues Jan 3, 2021
noi5e added a commit to noi5e/plots2 that referenced this issue Jan 5, 2021
jywarren pushed a commit that referenced this issue Jan 6, 2021
* generate unique HTML IDs for image upload .dropzones & inputs

* update system tests referring to #fileinput

* move get_dropzone_id logic to application_helper.rb #8926

* change comments to HTML comments

* fix indents to pass linter
noi5e added a commit to noi5e/plots2 that referenced this issue Jan 11, 2021
jywarren pushed a commit that referenced this issue Jan 11, 2021
* add new image upload system tests

* rename comment fixtures

* assign D.selected in start; rewrite drop assignment #8926

* specify E.refresh conditional call in E.wrap #8926
@noi5e noi5e changed the title Issue: Add Unique HTML IDs to Comment Forms to Fix Image Upload Cross-Wiring Issues Discussion: Add Unique HTML IDs to Comment Forms to Fix Image Upload Cross-Wiring Issues Jan 23, 2021
@noi5e noi5e changed the title Discussion: Add Unique HTML IDs to Comment Forms to Fix Image Upload Cross-Wiring Issues [Discussion] Add Unique HTML IDs to Comment Forms to Fix Image Upload Cross-Wiring Issues Jan 23, 2021
manchere pushed a commit to manchere/plots2 that referenced this issue Feb 13, 2021
* generate unique HTML IDs for image upload .dropzones & inputs

* update system tests referring to #fileinput

* move get_dropzone_id logic to application_helper.rb publiclab#8926

* change comments to HTML comments

* fix indents to pass linter
manchere pushed a commit to manchere/plots2 that referenced this issue Feb 13, 2021
* add new image upload system tests

* rename comment fixtures

* assign D.selected in start; rewrite drop assignment publiclab#8926

* specify E.refresh conditional call in E.wrap publiclab#8926
lagunasmel pushed a commit to lagunasmel/plots2 that referenced this issue Mar 2, 2021
* generate unique HTML IDs for image upload .dropzones & inputs

* update system tests referring to #fileinput

* move get_dropzone_id logic to application_helper.rb publiclab#8926

* change comments to HTML comments

* fix indents to pass linter
lagunasmel pushed a commit to lagunasmel/plots2 that referenced this issue Mar 2, 2021
* add new image upload system tests

* rename comment fixtures

* assign D.selected in start; rewrite drop assignment publiclab#8926

* specify E.refresh conditional call in E.wrap publiclab#8926
reginaalyssa pushed a commit to reginaalyssa/plots2 that referenced this issue Oct 16, 2021
* generate unique HTML IDs for image upload .dropzones & inputs

* update system tests referring to #fileinput

* move get_dropzone_id logic to application_helper.rb publiclab#8926

* change comments to HTML comments

* fix indents to pass linter
reginaalyssa pushed a commit to reginaalyssa/plots2 that referenced this issue Oct 16, 2021
* add new image upload system tests

* rename comment fixtures

* assign D.selected in start; rewrite drop assignment publiclab#8926

* specify E.refresh conditional call in E.wrap publiclab#8926
billymoroney1 pushed a commit to billymoroney1/plots2 that referenced this issue Dec 28, 2021
* generate unique HTML IDs for image upload .dropzones & inputs

* update system tests referring to #fileinput

* move get_dropzone_id logic to application_helper.rb publiclab#8926

* change comments to HTML comments

* fix indents to pass linter
billymoroney1 pushed a commit to billymoroney1/plots2 that referenced this issue Dec 28, 2021
* add new image upload system tests

* rename comment fixtures

* assign D.selected in start; rewrite drop assignment publiclab#8926

* specify E.refresh conditional call in E.wrap publiclab#8926
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature explains that the issue is to add a new feature outreachy
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant