Skip to content

AO3-7214 AO3-7215 Bookmark Javascript Improvement #5499

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

Open
ReverM wants to merge 12 commits into
base: master
Choose a base branch
from
Open

AO3-7214 AO3-7215 Bookmark Javascript Improvement #5499

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
69ca95a
First batch of change
ReverM Dec 4, 2025
3af400b
Merge branch 'otwcode:master' into AO3-7215
ReverM Dec 8, 2025
3f055af
Added test and translation
ReverM Dec 8, 2025
df354d9
Added tests
ReverM Dec 10, 2025
9146e6d
Adressed style issues
ReverM Dec 10, 2025
3678690
Fixed translation
ReverM Dec 10, 2025
3affd95
Remove leftover test
ReverM Dec 10, 2025
dc7d11b
These spaces thought we wouldn't see them
ReverM Dec 10, 2025
d2dfbd3
Normalizing
ReverM Dec 10, 2025
9c90fe5
Changed an id to class
ReverM Jan 1, 2026
b4bbe6e
fixed linter issue
ReverM Jan 1, 2026
7a33cf6
Normalized
ReverM Jan 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions app/views/bookmarks/_bookmark_blurb.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
<li id="bookmark_<%= bookmark.id %>" class="<%= css_classes_for_bookmark_blurb(bookmark) %>" role="article">

<% if bookmarkable.blank? %>
<p class="message"><%= ts("This has been deleted, sorry!") %></p>
<p class="message"><%= t(".deleted") %></p>
<% # Bookmarks of deleted items need a div because they can still be edited. %>
<div class="new dynamic" id="bookmark_form_placement_for_<%= bookmark_form_id %>"></div>
<% else %>
Expand All @@ -25,9 +25,9 @@
<% if logged_in? && !is_author_of?(bookmark) %>
<li>
<% if (current_user_bookmark ||= bookmark_if_exists(bookmarkable)) %>
<%= link_to ts("Saved"), edit_bookmark_path(current_user_bookmark), id: "bookmark_form_trigger_for_#{bookmark_form_id}", remote: true %>
<%= link_to t(".saved"), edit_bookmark_path(current_user_bookmark), class: "bookmark_form_trigger_for_#{bookmark_form_id}", remote: true %>
<% else %>
<%= link_to ts("Save"), get_new_bookmark_path(bookmarkable), id: "bookmark_form_trigger_for_#{bookmark_form_id}", remote: true %>
<%= link_to t(".save"), get_new_bookmark_path(bookmarkable), class: "bookmark_form_trigger_for_#{bookmark_form_id}", remote: true %>
<% end %>
</li>
<% end %>
Expand Down
6 changes: 3 additions & 3 deletions app/views/bookmarks/_bookmark_owner_navigation.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
<% # expects "bookmark" %>
<% bookmark_form_id = (bookmark.bookmarkable.blank? ? "#{bookmark.id}" : "#{bookmark.bookmarkable.id}") %>
<ul class="actions" role="menu">
<li><%= link_to ts("Edit"), edit_bookmark_path(bookmark), id: "bookmark_form_trigger_for_#{bookmark_form_id}", remote: true %></li>
<li><%= link_to ts("Delete"), confirm_delete_bookmark_path(bookmark), data: {confirm: ts('Are you sure you want to delete this bookmark?')} %></li>
<li><%= link_to t(".edit"), edit_bookmark_path(bookmark), class: "bookmark_form_trigger_for_#{bookmark_form_id}", remote: true %></li>
<li><%= link_to t(".delete"), confirm_delete_bookmark_path(bookmark), data: { confirm: t(".delete_confirmation") } %></li>
<li id="add_collection_link"><%= collection_link(bookmark) %></li>
<% if bookmark.bookmarkable.is_a?(Work) && !bookmark.bookmarkable.unrevealed? %>
<li class="share hidden">
<%= link_to_modal ts("Share"), for: share_bookmark_path(bookmark), title: ts("Share Bookmark") %>
<%= link_to_modal t(".share"), for: share_bookmark_path(bookmark), title: t(".share_bookmark") %>
</li>
<% end %>
</ul>
10 changes: 2 additions & 8 deletions app/views/bookmarks/_bookmarkable_blurb.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,9 @@
<ul class="actions" role="navigation">
<li>
<% if current_user_bookmark ||= bookmark_if_exists(unwrapped) %>
<%= link_to ts("Saved"),
edit_bookmark_path(current_user_bookmark),
id: "bookmark_form_trigger_for_#{bookmarkable.id}",
remote: true %>
<%= link_to t(".saved"), edit_bookmark_path(current_user_bookmark), class: "bookmark_form_trigger_for_#{bookmarkable.id}", remote: true %>
<% else %>
<%= link_to ts("Save"),
new_polymorphic_path([bookmarkable, Bookmark]),
id: "bookmark_form_trigger_for_#{bookmarkable.id}",
remote: true %>
<%= link_to t(".save"), new_polymorphic_path([bookmarkable, Bookmark]), class: "bookmark_form_trigger_for_#{bookmarkable.id}", remote: true %>
<% end %>
</li>
</ul>
Expand Down
20 changes: 9 additions & 11 deletions app/views/bookmarks/bookmark_form_dynamic.js.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,21 +2,19 @@
<% bookmark_form_id = (@bookmarkable.blank? ? "#{@bookmark.id}" : "#{@bookmarkable.id}") %>

var bookmark_div = $j('#bookmark_form_placement_for_<%= bookmark_form_id %>');
var bookmark_close = $j('#bookmark_form_close_for_<%= bookmark_form_id %>');
var bookmark_open = $j('#bookmark_form_trigger_for_<%= bookmark_form_id %>');

bookmark_div.html("<%= escape_javascript(render "bookmarks/bookmark_form", :bookmarkable => @bookmarkable, :bookmark => @bookmark, :button_name => @button_name, :action => @action, :in_page => true, :dynamic => true) %>");
bookmark_open.hide();
$j('.bookmark_form_trigger_for_<%= bookmark_form_id %>').hide();

$j('#bookmark_form_close_for_<%= bookmark_form_id %>').click(function(){
bookmark_div.hide();
bookmark_open.show();
$j('#bookmark_form_close_for_<%= bookmark_form_id %>').click(function(){
$j('#bookmark_form_placement_for_<%= bookmark_form_id %>').hide();
$j('.bookmark_form_trigger_for_<%= bookmark_form_id %>').show();
$j('.bookmark_form_trigger_for_<%= bookmark_form_id %>').attr('href', '#');
Copy link
Collaborator

@sarken sarken Dec 31, 2025

Choose a reason for hiding this comment

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

I was a little confused by this line. It didn't seem necessary at first, but when I removed it, I noticed that it seems to fix an issue introduced by the other changes in this file.

Here's what I noticed:

Currently, if I go to a bookmarks index, press "Save" on something I don't have bookmarked, enter some text in the form, and then use the "x" to close the form without saving, the form opens with my text entered the next time I press "Save."

It seems like that behavior breaks when this line is removed: the second time I press "Save," the form gets reloaded and my text is lost.

Is that indeed what this line is fixing? If so, do you know why that bug happens, and if there is any other way we could avoid it? Removing the original href means it's no longer possible to right-click "Save" and open the form in a new window, which is somewhat unfortunate. We can live with it if we have to, but it would be a good idea to have a code comment explaining why this line is needed.

Copy link
Contributor Author

@ReverM ReverM Dec 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Which line in particular are you referring to?

As for the href, the reason why it's removed is the route of the bookmark is rendering the form as part of the controller (see line 176 of the bookmark controller). However, hiding with the x goes through the dynamic.js.erb file (above). If we don't do that, 7215 is reintroduced (since it will go through the controller to find the route of the link instead of the javascript watcher).

An alternate solution would be to delete the form instead of hiding it. That way, rendering would always work since the form would not be there already. Is the behavior you mentioned not already occurring if you close and open a bookmark on the works page having written stuff already?

Copy link
Collaborator

@sarken sarken Jan 1, 2026

Choose a reason for hiding this comment

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

Which line in particular are you referring to?

The line removing the href, sorry! GitHub told me the comment was only on line 12 before I submitted the review, but it appears to have lied.

If we don't do that, 7215 is reintroduced (since it will go through the controller to find the route of the link instead of the javascript watcher).

For me, 7215 remained fixed without this line. The only difference in behavior was the information I had entered in the form was lost upon re-opening it (which would be consistent with opening a new form instead of un-hiding the original). But based on your explanation, I suspect if I had re-opened the form twice instead of just once, I would've run into 7215.

An alternate solution would be to delete the form instead of hiding it.

That would mean losing any information you had entered in the form before closing it, so we don't want to do that. While we could store that information somewhere and retrieve it when you re-open the form, it's probably not worth the extra code. If we get complaints about the removal of the right-click functionality, we can reconsider.

Is the behavior you mentioned not already occurring if you close and open a bookmark on the works page having written stuff already?

The form currently (i.e., on production) keeps what I've written.

Copy link
Contributor Author

@ReverM ReverM Jan 1, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

I believe if you go on the works index, open a bookmark form and close it again you can't open it with a right click anymore.

I've tried changing some stuff to make the right click work, but I'm unsure how to approach it.

});

// if canceled we don't want to generate the form a second time, just reopen it
bookmark_open.attr('href', '#');
$j("#bookmark_form_trigger_for_<%= bookmark_form_id %>").click(function(event){
bookmark_div.show();
bookmark_open.hide();
$j(".bookmark_form_trigger_for_<%= bookmark_form_id %>").click(function(event){
$j('#bookmark_form_placement_for_<%= bookmark_form_id %>').show();
$j('.bookmark_form_trigger_for_<%= bookmark_form_id %>').hide();
event.preventDefault();
});
});
10 changes: 5 additions & 5 deletions app/views/bookmarks/index.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,23 +14,23 @@
<% if current_user.is_a?(User) || @tag || @facets.present? %>
<ul class="navigation actions" role="navigation">
<% if logged_in? && (@user == current_user || (@owner.blank? && @bookmarkable.blank?) || @collection) %>
<li><%= link_to ts("Bookmark External Work"), new_external_work_path %></li>
<li><%= link_to t(".navigation.new_external_work"), new_external_work_path %></li>
<% elsif params[:work_id] || params[:series_id] || params[:external_work_id] %>
<% bookmark_form_id = (@bookmarkable.blank? ? "#{@bookmark.id}" : "#{@bookmarkable.id}") %>
<% # let the user reading this bookmark save a copy for themselves %>
<% if logged_in? && !is_author_of?(@bookmark) %>
<li>
<% if (current_user_bookmark ||= bookmark_if_exists(@bookmarkable)) %>
<%= link_to ts("Edit Bookmark"), edit_bookmark_path(current_user_bookmark), :id => "bookmark_form_trigger_for_#{bookmark_form_id}", :remote => true %>
<%= link_to t(".navigation.edit"), edit_bookmark_path(current_user_bookmark), class: "bookmark_form_trigger_for_#{bookmark_form_id}", remote: true %>
<% else %>
<%= link_to ts("Bookmark"), get_new_bookmark_path(@bookmarkable), :id => "bookmark_form_trigger_for_#{bookmark_form_id}", :remote => true %>
<%= link_to t(".navigation.new"), get_new_bookmark_path(@bookmarkable), class: "bookmark_form_trigger_for_#{bookmark_form_id}", remote: true %>
<% end %>
</li>
<% end %>
<% end %>
<% if @tag %>
<li><%= span_if_current ts('Works'), tag_works_path(@tag) %></li>
<li><%= span_if_current ts('Bookmarks'), tag_bookmarks_path(@tag) %></li>
<li><%= span_if_current t(".navigation.works"), tag_works_path(@tag) %></li>
<li><%= span_if_current t(".navigation.bookmarks"), tag_bookmarks_path(@tag) %></li>
<% end %>
<% if @facets.present? %>
<% # Filters button for narrow screens jumps to filters when JavaScript is disabled and opens filters when JavaScript is enabled %>
Expand Down
19 changes: 19 additions & 0 deletions config/locales/views/en.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -536,11 +536,30 @@ en:
hide_works: hide their works or bookmarks from you
intro: 'Blocking a user will not:'
bookmarks:
bookmark_blurb:
deleted: This has been deleted, sorry!
save: Save
saved: Saved
bookmark_owner_navigation:
delete: Delete
delete_confirmation: Are you sure you want to delete this bookmark?
edit: Edit
share: Share
share_bookmark: Share Bookmark
bookmark_user_module:
bookmarked_by_html: Bookmarked by %{pseud_link}
bookmarkable_blurb:
save: Save
saved: Saved
index:
advanced_search: try our advanced search
choose_fandom: choose a fandom
navigation:
bookmarks: Bookmarks
edit: Edit Bookmark
new: Boorkmark
new_external_work: Bookmark External Work
works: Works
recent_bookmarks_html: These are some of the latest bookmarks created on the Archive. To find more bookmarks, %{choose_fandom_link} or %{advanced_search_link}.
challenge:
gift_exchange:
Expand Down
41 changes: 41 additions & 0 deletions features/bookmarks/bookmark_javascript.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
@bookmarks
Feature: Edit bookmarks with javascript enabled
In order to have a good user experience
As a humble user
I want to be able to edit bookmarks with javascript enabled

Background:
Given a canonical fandom "The Bookmarks"
And the work "Bookmark: The Beginnings" by "bookmarker" with fandom "The Bookmarks"
And the work "Bookmark: The Sequel" by "bookmarker" with fandom "The Bookmarks"
And all indexing jobs have been run
And the dashboard counts have expired
And I am logged in as "bookmarker"

@javascript
Scenario: Opening multiple edit forms lets you close all of them (AO3-7214)
Given I am logged in as "recengine"
And I bookmark the work "Bookmark: The Beginnings"
And I bookmark the work "Bookmark: The Sequel"
And all indexing jobs have been run
When I follow "Bookmarks"
And I follow "Edit"
And I follow "Edit"
Then I should not see "Edit"
When I exit the bookmark edit form
And I exit the bookmark edit form
Then I should not see "save a bookmark!"

@javascript
Scenario: The edit form can be reopened after closing it with "X" on some pages (AO3-7215)
Given I am logged in as "recengine"
And I bookmark the work "Bookmark: The Beginnings"
When I view the work "Bookmark: The Beginnings"
And I follow "1"
And I follow "Edit"
Then I should see "save a bookmark!"
And I should not see "Edit Bookmark"
When I exit the bookmark edit form
Then I should see "Edit Bookmark"
When I follow "Edit"
Then I should see "save a bookmark!"
5 changes: 5 additions & 0 deletions features/step_definitions/bookmark_steps.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -384,6 +384,11 @@ def submit_bookmark_form(pseud, note, tags, collection)
visit work_path(work)
end

# Capybara will not find links without href with the click_link method (see issue #379 on the capybara repository)
When "I exit the bookmark edit form" do
find("a", text: "×").click
end

When /^I add my bookmark to the collection "([^\"]*)"$/ do |collection_name|
step %{I follow "Add To Collection"}
fill_in("collection_names", with: collection_name)
Expand Down
Loading