zotero / zotero-connectors

Chrome, Firefox, Edge, and Safari extensions for Zotero
https://www.zotero.org/download/connectors
Other
531 stars 124 forks source link

add cancel button to undo unwanted save session #512

Open abaevbog opened 2 months ago

abaevbog commented 2 months ago

Depends on: https://github.com/zotero/zotero/pull/4694 that adds the /connector/cancel endpoint

Addresses: https://github.com/zotero/zotero-connectors/issues/301

A rough demo with the current state of /connector/cancel endpoint.

https://github.com/user-attachments/assets/0076bfdb-ae45-4e48-b3a1-943e83fb969f

abaevbog commented 2 months ago

@adomasven I wanted to have a look at one of the connector issues while a number of accessibility changes is being reviewed and this seemed like a good one. Let me know what you think! This ended up being spread across two PRs - I marked both of them as drafts to begin with to make sure it's a reasonably direction to move in.

adomasven commented 2 months ago

This is great!

I'm not a fan of the Done and Cancel button placement though, but that's something to address more generally perhaps, since even now Done is located somewhat randomly in the middle of the dialog. We cannot put buttons at the bottom as the dialog expands as items are saved. @yexingsha do you have any ideas?

yexingsha commented 2 months ago

We cannot put buttons at the bottom as the dialog expands as items are saved.

I'm not sure I understand the rationale behind this. A cancel button that doesn't appear without expanding the dialog first seems pretty counterintuitive. Is there a reason we can't put the buttons at the bottom of both expanded and unexpanded versions of this dialog?

abaevbog commented 2 months ago

We can! I feel like "Done" and "Cancel" buttons should be together, otherwise the "Cancel" button would just be surrounded by empty space. So we could place both of them onto the next line and let tags input fully expand. Something like either of these options?

This still looks a bit strange to me because of the empty space after/before the buttons but the second option seems somewhat more conventional?

Screenshot 2024-09-18 at 10 19 27 AM Screenshot 2024-09-18 at 10 21 38 AM
abaevbog commented 2 months ago

A cancel button that doesn't appear without expanding the dialog first seems pretty counterintuitive. Is there a reason we can't put the buttons at the bottom of both expanded and unexpanded versions of this dialog?

So I just realized that I did not fully understand the comment and just jumped to making suggestions with buttons still not visible unless the collection selector is expanded... Sorry about that! So my last comment is somewhat irrelevant though the screenshots are still possibilities to consider.

As far as I am aware, technically, there is nothing stopping us from showing the buttons at the very bottom of the dialog always. We could pull them out of the target selector component. Does it feels a bit strange because they end up being "pushed down" as the items are being added though?

https://github.com/user-attachments/assets/064232c9-6572-4089-8e2f-bd641d4922a2

And if we were to go this way, we'd want to do something with the "Done" button because it just hides the popup without stopping the save process, as opposed to confirming that "everything is finished". So in the recording above, it feels a bit misleading to me. Maybe we could disable it until we're really done or something?

yexingsha commented 2 months ago

I think the cancel/done buttons being pushed down is fine, since it's just like e.g. macOS "Save As" dialog, where expanding the dialog pushes the buttons out and down.

As to the "Done" button being misleading, I think when people click this button, they probably just want to dismiss the dialog. The button being disabled for a few seconds will feel annoying and laggy, and anything longer than a few seconds will make people think something has gone wrong. Borrowing from the "Save As" dialog again, maybe we can change the wording to "Save" so that it's more like acknowledging an action rather than confirming the action has finished?

adomasven commented 2 months ago

To give some context:

  1. Initially the progress window did not have a collection picker and clicking anywhere on it dismissed it. This is still the case with the unexpanded dialog. I think it's generally fine and is better than adding a "Done" button, since people might start assuming they have to click that otherwise saving won't go through.

  2. We added the "Done" button to the expanded dialog since you have a collection selector and tag entry box there, and we felt we needed users to give something to click so that they can feel certain their changes have been saved, but technically the button just closes the progress window.

To maintain 1, it would probably be best to only add a cancel (X/trash/undo icon?) button somewhere on the unexpanded progress window. In neither case the buttons should be placed at the bottom of the dialog, because if you're saving multiple items, that dialog might keep expanding and also eventually gain a scrollbar (hiding the buttons below the fold).

Also this may be a personal opinion, but it's true, that software which moves UI elements under user's direct cursor are committing a cardinal sin of UX and if some macOS dialog does that, then shame on them!

Also we should probably make the buttons blue and red.

yexingsha commented 2 months ago

I think not adding a "Done" button to the unexpanded dialog is fine. And we should make the "Done" button in the expanded dialog blue, but making the "Cancel" button red is probably unnecessary, because it's not really a destructive action that warrants a warning.

As to the cancel button moving around, I think it's okay when it happens as the dialog gets expanded, because that's expected from a user action, and the user's cursor is on the expand/collapse button. The case where multiple items are being saved can indeed cause the cancel button to move away from directly under the user's cursor. However, I'm still inclined to keep the buttons at the bottom of the dialog because that's the standard pattern, and people will expect them to be there. We can do two things to alleviate the issues:

  1. Put the button in a separate div so that when the content overflows, scrolling won't hide the buttons.

  2. When there are multiple items being saved, set the height of the initial dialog to maximum to avoid the buttons moving around.

multiple-bottom-expanded multiple-bottom-collapsed single-bottom-collapsed

If we have to put the buttons somewhere else, the least weird place I can think of is at the top. That does avoid all of the issues, but still I don't really recommend it:

multiple-top-collapsed multiple-top-expanded
abaevbog commented 2 months ago

@adomasven thank you for the context! It actually clarified a few things for me. I've wondered for a while why there is a "done" button that essentially just closes the popup.

@yexingsha from the suggested ideas, the one where we place the buttons at the bottom and the progress items containers becomes scrollable if we used up all available height is the most appealing to me personally. While the buttons being pushed down does feel weird, I suppose anything else would be even weirder, right? Like, buttons at the top is very not-standard, and if we place them below the collection selector but above the progress items like here, it'll also look strange because the "done" and "cancel" button would be in the middle of the popup. And having the popup immediately expand and leave empty space between buttons and the last progress item I think would look as if something broke and is missing there.

So I began looking at putting option 1 together. It does require a bit of stylesheet refactoring that I'm working through but this is the rough demo of this with a large number of items being added. I think it feels quite alright?

https://github.com/user-attachments/assets/3a812108-f612-4578-a8e4-8ea09b850a83

adomasven commented 2 months ago

It's weird that the scrollbar doesn't appear for sections that are scrollable. I guess this is a macOS only thing, but can we force it to appear there? Also, if you accidentally saved something, I think you're going to notice it and try to click the Cancel button early, which means that the button moving away IS going to be annoying, and furthermore you will click on the dialog and dismiss it.

One thing we could do is make the dialog stop expanding if the user's cursor is hovering over it and then allow it to expand again if it leaves or expands the target selector. It might feel a bit strange, but we're going to have to make some sort of compromise here.

Another idea is to make the buttons floating or put them on the side like some weird appendix or something. Or make the dialog wider and put the cancel button on the first line and make it an X next to the down-chevron that expands into a Cancel button on hover.

abaevbog commented 2 months ago

It's weird that the scrollbar doesn't appear for sections that are scrollable. I guess this is a macOS only thing, but can we force it to appear there?

Hm, that comes from macOS system preferences > Appearance > Show scrollbars, and I'm not exactly sure if we can force it to show the scrollbars if the system pref is set otherwise... It's worth noting that selecting to always display a scrollbar looks like this. The extra outermost scrollbar is not meant to be there so it needs to be somehow removed.

Screenshot 2024-09-20 at 12 05 39 AM

Also, if you accidentally saved something, I think you're going to notice it and try to click the Cancel button early, which means that the button moving away IS going to be annoying, and furthermore you will click on the dialog and dismiss it.

That is a great point. You would be "chasing" the cancel button and that is not good ...

One thing we could do is make the dialog stop expanding if the user's cursor is hovering over it and then allow it to expand again if it leaves or expands the target selector. It might feel a bit strange, but we're going to have to make some sort of compromise here.

I'm afraid the risk here would be that if you open a popup, hover over it immediately and do nothing, it might look like something broke because nothing would be happening ... We could add one or two items and then just scroll the container down as more items are added but it goes back to pushing the button initially and seems too complicated.

Another idea is to make the buttons floating or put them on the side like some weird appendix or something.

Alternative positioning for buttons is definitely something to consider. One thing with floating buttons to keep in mind is, per the vpat review issue, one needs to be able to zoom in very closely (400%) into the popup. So if the buttons are floating or sticky, we need to make sure the rest of the content can still be visible and scrollable.

Or make the dialog wider and put the cancel button on the first line and make it an X next to the down-chevron that expands into a Cancel button on hover.

If the dialog becomes winder, maybe we just put "Cancel" button (as an X with a tooltip) and "Done button" together onto the very first line between the collection dropdown and the expand button? It could be somewhat close to what dropbox displays when you upload items:

Screenshot 2024-09-20 at 12 43 56 AM

Last idea I had is to put buttons below the target selector (as I tried first) but make a more distinct separation between the entire popup above and the progress items below. Add more padding, some border, etc. That way, there would be an interactive popup section that ends with "cancel" and "done" buttons and the progress items would be something like a log below. This idea came from looking at a github actions log, so it may or may not be appropriate for most people.

Screenshot 2024-09-20 at 12 24 24 AM
adomasven commented 2 months ago

Last idea I had is to put buttons below the target selector (as I tried first) but make a more distinct separation between the entire popup above and the progress items below. Add more padding, some border, etc. That way, there would be an interactive popup section that ends with "cancel" and "done" buttons and the progress items would be something like a log below. This idea came from looking at a github actions log, so it may or may not be appropriate for most people.

Yeah, and this doesn't work with the collapsed dialog.

abaevbog commented 2 months ago

I was thinking something like below. It actually looks better to me when the dialog is collapsed than when expanded:

https://github.com/user-attachments/assets/37bd1b13-0f22-4026-9be1-12a610c89a07

And if there are too many items, that section would be scrollable

adomasven commented 2 months ago

This could work with more event vertical padding and left-alignment although I still think it's worth exploring an X that expands into the word Cancel on Hover/Focus.

abaevbog commented 2 months ago

This could work with more event vertical padding and left-alignment

Ok, cool! I am really just brainstorming ideas trying to not get too stuck on specifics so all of the suggestions above would very much need to be polished.

I still think it's worth exploring an X that expands into the word Cancel on Hover/Focus.

It is definitely a possibility to place the X into the header. Trying it out, I think we do run into a few small issues. 1. If we place the cancel button to the top, I feel like the "done" button needs to go there too? I think it would look very strange if two buttons that generally go side-by-side are in different parts of the popup. In that case, different widths of done, cancel and expand icons look make it look a bit strange. 2. If we also want the cancel button to expand from X to "cancel", we either have to reserve space for the expanded form, which means "X" will be surrounded by empty space, or it will push other buttons on hover, which is also strange. That is, unless the cancel button is the left-most button but I think the X button always goes to the very right or the very left.

This is a brief recording to demonstrate these points:

https://github.com/user-attachments/assets/212dc42e-821d-4457-8892-1b6625afdc96

As a potential remedy, what if we do not expand the "cancel" button and keep it as an "X" but add a title="Cancel" so that if someone is not sure what it does, they still get a hint when they hover over? Then, we'd have cancel and expand/collapse button as just icons, so maybe we make the "done" button a simple icon as well? Then all 3 icons are somewhat unified and it will look almost similar to typical window control buttons. This is a sketch of what it might look like. I just used unicode characters instead of proper icons to see what it looks like but I think it conveys the idea:

Screenshot 2024-09-20 at 12 03 32 PM
abaevbog commented 2 months ago

It's really quite remarkable how many different ways there are to position two buttons!

yexingsha commented 2 months ago

I like the version where the buttons are at the bottom of the saving section, with the items split off into their own progress section. I think it can work very well if we distinguish the two sections more:

middle-collapsed middle-expanded

The version where the buttons are icons at the end of the first line can be somewhat confusing, since people will expect the x button at the top right corner to function as "Close" rather than "Cancel", and though the "Done" button should logically be placed next to "Cancel", it being at the top is pretty unusual. Also, "Done" is usually placed to the right of "Cancel", which means when the dialog is expanded, the "Cancel" button will shift to the left. A version with the hover interaction can look like this:

https://github.com/user-attachments/assets/9a5ed242-ac46-4dc3-90e3-31de28a83fba

adomasven commented 2 months ago

I quite like the version with the expanding Cancel and Done buttons. @dstillman do you want to weigh in?

abaevbog commented 1 month ago

For reference, I imagine it would look somewhat similar to this? I think we are running into a small issue of new buttons not looking very fitting into the existing design, and I feel like it's best to not implement the entire redesign of the connector in this PR. So I only changed the buttons in the header to look somewhat more uniform with lighter background and new svgs as icons. Otherwise, the chevron and <select> dropdown would look way too different from X and the checkmark.

https://github.com/user-attachments/assets/76a02a98-aac3-4cfb-be31-f08b842fa125

In this recording, I still have the progress items area as a scrollable container left over from the previous iteration. ~On one hand, it feels somewhat nicer to me than having the entire dialog overflow, on the other hand, functionally, it does not add anything if the buttons are at the top. So this part can be reverted, right? I ended up having a lot of changes while trying out different approaches, in case we want to revisit other options before committing.~ edit: actually, if there are many items and the dialog itself is scrollable, as opposed to the itemProgress section being scrollable as in the recording, once you scroll down, the "done" and "cancel" buttons will no longer be visible, which doesn't seem right. So I suppose we do want to keep the scrollable itemProgress section.

yexingsha commented 1 month ago

I think this is really good! The buttons fit in nicely with the current UI, and the scrollable progress item area works well. I agree that we can update the overall styling in a later PR.

adomasven commented 1 month ago

Yeah, this is good. In the video the Done button seems to be bigger height than the other buttons and there's too much empty space where the Cancel button expands, so let's fix that and go with it.

abaevbog commented 1 month ago

Yeah, the border of the button was of the wrong color and I did make the dialog a bit too wide... There were a few other smaller things with spacing that I tweaked. How is it looking now?

https://github.com/user-attachments/assets/a265001d-b6cb-4ff1-b958-22481a68b491

And I just cleaned up earlier unrelated changes and pushed these changes. One question: I added new svg files to use for done, cancel and the disclosure button - it does look like in the build process, there is a step to fetch relevant images from the main repo but the submodule points at a pre-redesign commit that doesn't have all the new svg files, so I assumed just adding new files like this is they way to go for now?

Also, I was doing all the testing on firefox, so let me make sure everything looks right on chrome and safari.

abaevbog commented 1 month ago

Chrome looks good to me now as well. Per https://github.com/zotero/safari-app-extension, do I need to be a part of the team to build and sign the safari extension? Sorry if that's an obvious question, it's been a while since I had to work with xcode.

dstillman commented 1 month ago

Don't bother trying to test on Safari for now — it's too finicky. We'll check and report back.

adomasven commented 1 month ago

One question: I added new svg files to use for done, cancel and the disclosure button - it does look like in the build process, there is a step to fetch relevant images from the main repo but the submodule points at a pre-redesign commit that doesn't have all the new svg files, so I assumed just adding new files like this is they way to go for now?

That's fine. There's a PR pending with updated icons (with the zotero submodule updated) that I am waiting an OK from Dan, but it shouldn't hold this up.

abaevbog commented 1 month ago

Sounds good! Meanwhile, I added some final touches to remove unwanted spacing after buttons and to not show the "done" button unless target selector is expanded per https://github.com/zotero/zotero-connectors/pull/512#issuecomment-2364926136. I unintentionally reverted that change before pushing yesterday. With these final tweaks, I think I can un-draft this.

adomasven commented 1 month ago

@abaevbog You should test the manifestv3 build on chrome. I'll remove the chrome build as it's now fully phased out. EDIT: Actually it's still used by Edge, so keeping it for now.

adomasven commented 1 month ago

@abaevbog One more thing. We should enable this with a Zotero feature check. The response to /ping on Zotero should include supportsSaveCancelling and Connector should only show the cancel button in that case. Also no cancel button if saving to server.

abaevbog commented 1 month ago

You should test the manifestv3 build on chrome.

Gotcha, so from now on, I'll use manifestv3 build folder for chrome but still firefox for firefox.

One more thing. We should enable this with a Zotero feature check.

Oh, that is a great point! Otherwise, one could have an updated connector that would try to hit /connector/cancel that the client might not yet handle.

I pushed to the main repo PR to return supportsSaveCancelling: true together with other prefs when /connector/ping is hit. And here, we just check if it was received and hide the button if it wasn't OR if we're saving to web library. So, for instance, there iw not X button if I run it with the current beta.