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.

Sign up for GitHub

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

Jump to bottom

Invoke .focus() on <trix-editor> after calling fill_in_rich_text_area #46807

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Invoke .focus() on <trix-editor> after calling fill_in_rich_text_area #46807

wants to merge 1 commit into from

Conversation

seanpdoyle
Copy link
Contributor

@seanpdoyle seanpdoyle commented Dec 23, 2022 •
edited

Motivation / Background

Follow-up to rails/rails#46249
Closes rails/rails#46803

Detail

Invoke HTMLElement.focus on the element instead of the Trix.Editor instance when calling fill_in_rich_text_area.

Additional information

Defer merging this until #46808 is merged and #46804 is closed, this PR will re-introduce the focus behavior and test coverage.

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

@rails-bot rails-bot bot added the actiontext label Dec 23, 2022
@seanpdoyle seanpdoyle mentioned this pull request Dec 23, 2022
Closed
seanpdoyle added a commit to seanpdoyle/rails that referenced this pull request Dec 23, 2022
@seanpdoyle
Revert "Focus editor after calling fill_in_rich_text_area"
0e90130 
Closes [rails#46803][].

This reverts commit 67a4ac6.

Once the system test helper test suite executes in CI, we can
re-introduce the changes as part of [rails#46807][].

[rails#46803]: rails#46803 (comment)
[rails#46807]: rails#46807
@seanpdoyle seanpdoyle mentioned this pull request Dec 23, 2022
Merged
4 tasks
@seanpdoyle seanpdoyle force-pushed the action-text-system-helper branch 2 times, most recently from dda47cd to 1b2b91f Compare January 15, 2023 22:38
@seanpdoyle seanpdoyle changed the title Invoke .focus() on <trix-editor> instead of Editor instance Invoke .focus() on <trix-editor> after calling fill_in_rich_text_area Sep 22, 2023
@seanpdoyle
Copy link
Contributor Author

Both #46804 and rails/buildkite-config#26 have been closed, but according to rails/buildkite-config#26 (comment), the underlying issue is still present.

What can be done to progress this PR while those details are being worked through?

@seanpdoyle seanpdoyle force-pushed the action-text-system-helper branch 2 times, most recently from a5aa122 to af2b999 Compare September 24, 2023 15:03
@seanpdoyle
Copy link
Contributor Author

@jonathanhefner if you're available, this PR reverts the revert PR to re-introduce this behavior in the test helper.

While there's been some movement on the related CI changes, I want to emphasize that the system test changes in this diff pass the test suite when executed locally.

Would it be possible to merge this test helper change ahead of 7.1?

@seanpdoyle
Copy link
Contributor Author

@rafaelfranca are you available to review this change?

Since it's test-only, would it be worth back porting into the next 7.1 patch release if it were merged?

@seanpdoyle seanpdoyle force-pushed the action-text-system-helper branch 3 times, most recently from 3a90da2 to 5c3340e Compare October 16, 2023 22:15
@seanpdoyle
Copy link
Contributor Author

@eileencodes are you available to review this change? I'm pinging you since you reviewed and merged the original PR that was ultimately reverted.

@eileencodes
Copy link
Member

If you want to get eyes on your PRs it's better to use Discord to ask.

Anyway, is #46803 actually fixed? According to rails/buildkite-config#26 the fix was reverted and I don't see anymore linked PRs that claim to have fixed it. We should reopen that issue if it's not fixed.

@zzak
Copy link
Member

zzak commented Nov 4, 2023

AFAIK, running system tests for actiontext in CI depends on #49527 and rails/buildkite-config#33

@seanpdoyle seanpdoyle mentioned this pull request Dec 21, 2023
Open
4 tasks
@seanpdoyle
Invoke .focus() on <trix-editor> instead of Editor instance
eab5942 
Follow-up to [rails#46249][]
Closes [rails#46803][]

Invoke [HTMLElement.focus][] on the element instead of the `Trix.Editor`
instance when calling `fill_in_rich_text_area`.

[rails#46803]: rails#46803
[rails#46249]: rails#46249
[HTMLElement.focus]: https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/focus
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
actiontext
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Action Text fill_in_rich_text_area system test helper calls nonexistent Trix Editor method
3 participants
@seanpdoyle @eileencodes @zzak