I think it might be better to try to educate the user earlier. Long ago, this was a more common problem, which is why draft inlines now have a dashed outline and a "Not Submitted Yet" in the header; these changes substantially reduce the number of users who hit confusion their first time through.

Was the user just confused about the draft behavior, or did they understand the draft behavior but expect updating to submit their comments?

Right now, we have no UI which actually tells you that you can even leave inline comments, or how to do it. We probably should, and maybe this could also explain that inlines are drafts until submitted. However, most of the approaches I've thought about for this are in the in the vein of "first-time tour" or "clippy the helpful animated character", both of which I'm really hesitant to build (I generally find them annoying, and they'll be hard to get much testing of since no one is going to remember to go through the tours again after making a change).

It's intentional that we don't submit unsubmitted inline comments when updating, and I hesitate to change this since I think the behavior is already correct for sophisticated users (really, users whose mental model includes the "draft" phase). I'd like to try to find a solution which improves this for new users without making it any worse for experienced users.

◀ Merged tasks: T4382.

The description here is more narrow than T4382, but generally there are several issues:

• There is no UI or other affordance which even tells you that it's possible to leave inlines in Differential.
• New users are sometimes confused by "draft" / "not submitted yet".
• Users of all experience levels sometimes forget to submit inlines.

I think the submit-on-comment model is enormously better than the submit-immediately model, so I'm very hesitant to change that. However, I suspect we can do a better job with the three issues above.

This isn't a great solution, but would likely make most happy:

• Default to a JS popup when you navigate away asking you to 'publish comments' or 'save as draft'
• Preference to not have JS popup in Settings.

Pholio at least could be solved differently, since it's inline comments stack on the right. There we could simply have a "Submit Inline Comments" button under the stack of dashed boxes. But doing something different just in Pholio isn't my first choice.

Just want to +1 that the "send immediately" model is bad. It would make me look like a fool in Differential in particular. I often have an inline like "have you considered X?" and then later see X was indeed considered somewhere else in the code.

I think Chad's solution handles 2 and 3 from Evan's breakdown. Not sure how to handle 1.

A downside to the JS popup is that it breaks the browser "bfcache", so hitting the back button in your browser to return to a revision will be slower. This is pretty minor, though.

A less severe approach would be to show some kind of floating "You have unsubmitted stuff!" icon on the page, but I'd guess users would still miss that sometimes.

I don't really see any other reasonable approaches.

Overall, the JS thing seems like the best first step here to me, too.

Hmm -- in Firefox, we can't put any text in the onbeforeunload dialog. It always looks like this:

See the relevant issue. This dialog seems nearly useless for reminding users to submit inline comments.

what does Facebook do?

Facebook uses onbeforeunload, but the message makes more sense there because the warning is really about unsaved form data (for example, here I've started chatting with you, but not submitted the text):

I think this message will be mostly useless for (3) and completely useless for (2), since it's pretty misleading (the interrupt has nothing to do with unsaved form data).

From that thread, two workarounds are:

• Show an alert() first in Firefox, so the user gets an alert ("You are about to get a dialog which is really confusing, it really means you haven't submitted inline comments. If you want to submit inlines, hit "OK" on this dialog, then "Stay on Page" on the next one").
• Blank the entire page and replace it with a message, so the vague dialog appears on top of a page with detailed instructions.

Those are both pretty sketchy.

I think onbeforeunload + a info box at the top of the page (added in onbeforeunload, and kept for future loads of the page) saying there are draft inline comments would cover it and fit in with the UI.

UX-wise, onbeforeunload would probably work fine on it's own, if you only see it when trying to navigate away after writing inline comments, you're going to associate it yourself pretty quickly.

If I can weigh in with my own opinion: the workflow is good as it is -- it's just not obvious what that workflow is. I posit that the blue "ready" button serves no purpose except to confuse. I don't know why you'd need to "ready" a comment. You've typed it in, and it's ready. It doesn't help that there are big blue buttons everywhere in Phabricator with whimsical titles. I like the whimsy, and in all cases but this one it's fine, because these buttons mean one thing: "do the thing". But "ready" doesn't do a thing -- all it does is animate the box to make it look like the thing was done. No one bothers to read "Not Yet Submitted", because they are already convinced, by having clicked the big blue button, and having seen the animation, that the thing they wanted to do has been done.

Just get rid of the "ready" button. Then, people will be looking for the big blue button to "do the thing" when they are done writing comments. Everyone understands on a webpage that a textbox with editable text needs something to be done before it's submitted. It could be good to have something in the inline comment form which helps them find the blue button (clowncopterize) by scrolling to the bottom of the page for them. This shows them the form at the bottom, which is probably the last place they look.

Getting rid of the "ready" button is better than an onbeforeunload, or "Not Yet Submitted", because it eliminates the source of confusion, rather than trying harder to explain it.

"Append" as a quick language fix? Not sold on Ready either.

"Attach" also works. Basically something more obvious that you're not submitting. Another alternative:

[ Cancel ] [ Attach and Submit ] [ Attach ]

@chad, why do you need a button at all? Not-inline comments have just two states:

• not yet submitted
• submitted

Why do inline comments need:

• not yet submitted
• not yet submitted, but now you have to click an "edit" button if you want to edit it more
• submitted

If there's some technical reason getting rid of inline comment buttons is hard, then I propose that the button says "preview", and that it's not blue. I think that's still confusing, but at least people will be confused immediately and directly, rather than thinking everything went fine then confused when other people can't see their comments.

Our overall goal here it to foster and encourage well formed and well thought out code reviews. Providing some friction and ability re-read the comment is part of that user-experience. Any change to the UX needs to make sure this philosophy isn't degraded.

My goal with this task is:

• Provide additional clarity around the interaction
• Provide additional value to the interaction
• Not disrupt the current userbase's workflow (or, if we do, ensure the value added is significant)

My concern with the auto-save model is:

• Does not provide additional clarity (users still need to learn to scroll to submit)
• Value provided is arguable (more form consistency, less ability to review comments)
• Disrupts current workflow

The alternative suggestion:

• Provides additional clarify (better wording, two actions)
• Provides additional value (lightweight is a user chosen option)
• Does not disrupt current workflows

@bitglue - I think a big difference is "not" inline comments have a giant preview area below them; inline comments have no such preview area. This preview is pretty critical if you include remarkup in your comments e.g. a code sample on how to better write something

I think unifying these concepts best then would be to change the inline commenting UI to ajax in a preview right below. There should then be a "cancel" and a "go weigh in" button. The latter should take you down to the main comment box where you actually submit the thing.

For 1 - helping people figure out inline comments exist - perhaps we could render a "pro tip" about leaving inline comments in the preview area for the main comment? As in, if no inline comments there is some inline-comment like UI down in that preview area with instructions / tips on how to leave inlines?

@chad, I'm not proposing auto-save. I'm saying leave everything as it is, and get rid of the button that does nothing.

To illustrate, go to a differential revision and enter some stuff in the non-inline comment box. Now go somewhere else. Now come back. Your comments are saved, as a draft. This is a bit of a hidden feature, but it's not confusing. I suspect other aspects of the UI work this way too. Adding a "draft automatically saved" near the preview could address this nicely.

Why does the inline comment box need to be different? Everywhere else in Phabricator, when you type in a textbox, the preview just automatically updates, and I presume, a draft is automatically saved. Other sites work the same way, stackexchange for example. This is a behavior that people increasingly expect of a web application.

Why is the inline comment form so different? Why doesn't it update the preview immediately?

There's another confusion that could be addressed here: when you make an inline comment, it shows up at the top of the revision, where people will see it in the context of other comments that have been made. So far as I can tell, it's redundant to also say in the comment box at the bottom, "see inline comments". Yet, people here are doing just that.

Presumably they don't realize that the inline comment will also be displayed at the top. They think people will have to scroll down through a potentially long diff to see that they've made a comment, adding a redundant comment at the top.

If the preview looked exactly like all the comments would look after they are submitted, then they wouldn't do this. The workflow is the same, but the interface to do it would be consistent with the rest of the Phabricator UI and generally just more of what people expect.

I really doubt that anyone would think that if someone begins an inline comment, they are presented with a <textarea>, and they haven't clicked a blue button, and it's still a <textarea>, that they will think their comment has been submitted. They will want to find a blue button, and you just have to help them find it. You will have to do this exactly once, because when they scroll down to the bottom, they will find the blue button, and see the preview, and maybe also the thing that says "draft automatically saved", and at this point they have correctly learned how the interface worked without any surprises.

All you have to do is give them a gentle hint, one for which they will be looking, to scroll to the bottom of the page. I bet some smart web 2.0 programmer can even make a link which when clicked, does the scrolling for them, with a neat animation. Compare this to the current "hint", which is some dim grey text which no one is looking for because up to the point where their coworkers say "where are your comments?!" they thought everything was working well. People don't see the hints on how to use the interface because they don't think anything is wrong. Make something wrong, give the proper hint. Problem solved.

When I said "auto-save" I meant "automatically attach it to the comment form below without clicking ready", at least I think I'm fully understanding the suggestion. :)

Generally, I think users who are still confused by this are often coming into the workflow with a strong, preexisting model of how comments work, e.g. from GitHub or some other system where they work differently. The workflow already provides fairly strong visual cues about how it works if you're coming in as a blank slate, but no hard stops which say "Hey! Your model of how inlines work is wrong! Stop thinking like that!".

I posit that the blue "ready" button serves no purpose except to confuse.

The ready button allows editing to be modal, which provides these advantages:

• You can cancel an edit to get back to a previous state. I do this fairly often while composing inlines, and removing it would make the workflow materially worse for me.
• It makes it clear when a comment has been saved to the server and will persist if the page is closed.
• It makes it easy to keep track of which comment you're working on if you scroll away to check something and have other inline comments.
• It allows you to preview the comment inline in a natural way.

We could solve these in other ways, and these features aren't necessarily perfect. However: they're clear, they're unambiguous, they work properly today, and everyone is happy with them.

But "ready" doesn't do a thing -- all it does is animate the box to make it look like the thing was done.

Ready exits the editing mode and persists the comment on the server, rendering a preview in the client.

"Append" as a quick language fix? Not sold on Ready either.
If there's some technical reason getting rid of inline comment buttons is hard, then I propose that the button says "preview", and that it's not blue.

"Ready" is already a language fix, see D1796.

You can reasonably assert that you'd find "Preview" or "Append" or "Attach" or some other word more clear than "Ready", but vrana chose "Ready" to implement this exact fix, so that's at least some evidence that whatever other word you might want to use isn't universally more clear. These are all very soft hints, and the UI already has a lot of soft hints.

Another alternative: [ Cancel ] [ Attach and Submit ] [ Attach ]

I don't think anyone wants this feature on its own (that is, everyone who has an accurate model of the workflow is happy with it), and I'd be frustrated if I hit this button by accident and submitted some half-finished junk. Even if this fixed the problem completely, I suspect the cure would be worse than the disease because of accidental clicks.

My proposal is to pop this once, the first time someone makes an inline in any application:

+---------------------------------------------------------------------------+
| +--------+  **Other users can't see your inline comment yet!**            |
| |  CUTE  |                                                                |
| |  PANDA |  In Phabricator, inline comments work a little differently     |
| +--------+  than they do in some other software. Your inline comment is   |
|             a **draft** until you submit feedback using the form at the   |
|             bottom of the page, and other users can't see it yet.         |
|                                                                           |
|             This gives you more opportunity to revise comments before     |
|             sharing them, and means Phabricator sends fewer, more useful  |
|             emails.                                                       |
|                                                                           |
|             You can tell which comments are drafts by looking at the      |
|             border. Draft comments have a dashed border.                  |
|                                                                           |
|             When you're ready to share your comments, scroll down to the  |
|             bottom of the page and submit feedback. This will publish     |
|             your draft comments and make them visible to other users.     |
|                                                                           |
+---------------------------------------------------------------------------+
|                    [ Keep Reminding Me Over and Over Again ] [ Got It! ]  |
+---------------------------------------------------------------------------+

Anyone hate that?

I'm not super hot on a big dialog/education. It solves the problem, yes, but in a fairly brute force kind of way. Though I do think we need some sort of NUX system for, well, everything.

In this case I'd like to have Design at least take a pass at the problem without changing the workflow concerns.

Do you have a specific proposal other than adding a third button, or do you mean you want to think about it some more?

I don't like the dialog either, although I think it's much better than a third button, changing the text, or removing the button.

I also don't think this is urgent at all, and am happy to let it sit indefinitely. T1460 will change this UI in material ways anyway.

I do but I'll just build it and we can keep the task around if/when it come up again. At least, a design/language pass is worth the low cost. (I've had a revised inline comments table in a branch for a week or so).

I'm shooting for brown.

hahaha

◀ Merged tasks: T3639.

Here is similar "Audit" application problem described: T3639: Don't eat unsaved inline comments

Explanations: https://secure.phabricator.com/T3639#9

◀ Merged tasks: T5667.

Has anyone considered implementing this similar to how Reviewboard does it: https://www.reviewboard.org/docs/manual/dev/users/reviews/draft-banner/ ? The draft banner is really handy and intuitive (it's always at the top of the pane that the review is open in).

We already note that you have a pending draft on revisions. It could probably be more obvious.

I've got to rebump this issue. I've started a new job since I last commented, and the current review process needs some work. Currently we are using GitHub PRs for review and I don't like it. I've installed Phabricator for evaluation, and so far both of two people who have tried it have made this mistake.

Usually when they figure that out, the next problem they have is that they don't realize they should accept or reject the diff. They just leave the action at "comment". I suspect this is because it's how GitHub works -- people don't even think to consider other actions. It's also a thing I like dislike most about GitHub's review workflow.

If I could rank this issue, I'd put it at the top of the list. Phabricator is otherwise the most awesome piece of software ever, but I think everyone I have ever introduced to Phabricator (about a dozen people, now) has hit this issue, in an interface that otherwise is super intuitive, unsurprising, and makes a great first impression.

@bitglue our expectation right now is companies will do their individual onboarding. T4778 covers how we currently prioritize tasks.

One possible approach is to overkill the draft-ness so hard that no reasonable user being can possibly miss it:

That's pretty awful, not sure how awful. It gets worse in the preview:

<blink>Unsubmitted</blink>

Yeah, basically. Presumably at some level of <blink /> tag, no first-time user will get the model wrong, but then every user pays the price for it forever. That mock feels like too high a price to pay, although maybe there's a 1/2 as blinky version that's more obvious than "Not Submitted Yet" + dashed border.

We could also try T1591 again, but it basically caused an endless array of problems.

I was planning to look into this with T7503

Any direction you prefer?

I think the real problem is right here:

The issue is, like it or not, that it looks just like this, which everyone has used (probably a lot):

Of course, these two things have extremely different semantics.

I'm not saying you should adopt the GitHub model, I actually do like that inline comments aren't submitted one-by one. I appreciate that Phabricator doesn't bombard me with fragments of a half-completed review.

I think anything you can do to make Phabricator not look like GitHub is good. Most importantly, make "Ready" not a button, and not blue. Both of these things scream, "when you click me, you will have made a visible (to everyone else) change to the application's state".

Everything is being re-built and re-designed as far as inline commenting goes, but I don't have mocks yet for the the submission form yet.

I like (3) on its own, but they all feel too heavy to actually ship to me -- (2) and (3) moreso than (1). Like, the terrible one-time clippy popup seems less bad to me than making the element that much heavier for experienced users. My thinking that we can make headway here by making the "draft" state more obvious may ultimately be misguided, since these are pretty tame but still feel too heavy to me.

Yeah, it might be too heavy. Mostly I'm curious if we value added the callout (Finalize Review - scrolls to comment box), if that'd make it more OK. I have some less obnoxious ideas, but wanted to start here.

Non-heavy version.

Ah, nice. That feels a lot more palatable.

I love that last one.

If the issue here is that you want to send all of the diff comments in a single email, why not just send those on a timed basis? E.g.: I have submitted a diff, and someone comes to review it. If there's a new comment within 5 minutes (maybe less or more, not sure about the exact time), it doesn't get mailed. If we don't have any new comments for these 5 minutes, all new comments get collected into a single email. No need for comment drafts at all, only maybe you'd have to add a timer under every draft saying that it will get saved in 5 mins, no need to click the Submit button under an empty textarea.

The worst UX problem in Diff is that in order to save all of my comments, I have to click a submit button under a text field. If first paragraph isn't something you could implement, then maybe it's worth adding a position: fixed; bar on top of the window that would have a Submit button and a draft comment counter?