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

Trim Reblogs: always show trim button #1013

Merged
merged 3 commits into from
Mar 20, 2023

Conversation

marcustyphoon
Copy link
Collaborator

Seeing a lot of user reports that Trim Reblogs is broken and I would really like to know if it's a button add failure or a misunderstanding about which posts should be trimmable. Hence:

Description

As described in the linked issue, this disables the trim button on editable but untrimmable posts, rather than not adding it at all.

This uses the native dom disabled attribute on the button element and moves the event handlers on control buttons to the button element itself.

Resolves #958.

Testing steps

  • Confirm that e.g. Quick Tags works as expected.
  • Try to press the Trim Reblogs button on an original post, a reblog with no added content and only one trail item, and a post with enough content to trim.

@marcustyphoon marcustyphoon requested a review from AprilSylph March 2, 2023 12:05
@marcustyphoon
Copy link
Collaborator Author

(oh, and yes, I checked what happens if a user removes the disabled attribute and clicks the button; nothing bad occurs.)

@marcustyphoon
Copy link
Collaborator Author

marcustyphoon commented Mar 3, 2023

Oh, major problem with this:

  • fix quick tags popup closing when you press space

I guess you have focus of the button element once you click it and the spacebar clicks it again or something...? Not sure yet, help appreciated.

(if this is unsolvable, we could skip using the disabled attribute and the button entirely, and just not attach handlers to the button container element if the button should be disabled. this feels less elegant, however, as buttons cannot be toggled on or off at will in that case without messing with the handlers.)

@AprilSylph
Copy link
Owner

I guess you have focus of the button element once you click it and the spacebar clicks it again or something...?

Are we not stopping keydown propagation in the Quick Tags field? We should be doing that for every <input type="text"> we add to the page

@marcustyphoon

This comment was marked as outdated.

@marcustyphoon
Copy link
Collaborator Author

marcustyphoon commented Mar 4, 2023

Oh, I bet I know what's happening: because I attached the handlers to the button element without changing the handlers themselves, and they reference currentTarget, the popups are being placed inside the button element instead of inside the container. Pressing spacebar inside the popup is thus pressing spacebar with the focus inside a button.

This could be fixed either by changing scripts' togglePopupDisplay/onButtonClicked handlers to look at the parent element of the button, or just by not using disabled as mentioned above, which is honestly probably easier? But then if I were ever trying to toggle a button later, I would be like, "seriously, past me?"

@AprilSylph
Copy link
Owner

oh right, yeah, definitely do not put popups inside button elements. that's no good on multiple levels.

Copy link
Owner

@AprilSylph AprilSylph left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works well with the exception that I can sneak a peak of cursor: pointer when hovering the very edge of a disabled button. We should attach cursor: not-allowed to the element that is actually disabled.

src/content_scripts/control_buttons.css Outdated Show resolved Hide resolved
Co-authored-by: April Sylph <28949509+AprilSylph@users.noreply.github.com>
@AprilSylph AprilSylph merged commit f064985 into AprilSylph:master Mar 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Trim Reblogs: always show trim button
2 participants