Page MenuHomePhabricator

Prevent Send on Enter in Fullscreen Remarkup Mode
ClosedPublic

Authored by chad on Mar 31 2017, 2:33 AM.
Tags
None
Referenced Files
F14083279: D17590.id42445.diff
Sat, Nov 23, 3:37 AM
Unknown Object (File)
Thu, Nov 21, 7:44 PM
Unknown Object (File)
Wed, Nov 20, 1:32 AM
Unknown Object (File)
Tue, Nov 19, 8:45 PM
Unknown Object (File)
Sat, Nov 16, 8:47 AM
Unknown Object (File)
Wed, Nov 13, 9:18 PM
Unknown Object (File)
Tue, Nov 12, 2:22 PM
Unknown Object (File)
Mon, Nov 11, 6:09 AM
Subscribers

Details

Summary

Fixes T12138. Test for the presence of being in fullscreen mode, and disable send on enter if present. Side note, I'd love a first class "hasClass" type Javelin function.

Test Plan
  • Go to Conpherence
  • Type some smack, see it send on enter
  • Go fullscreen like a boss
  • Let the words flow
  • Close fullscreen, then send on enter.
  • (might be nice someday to add a "submit" button to fullscreen editor)

Diff Detail

Repository
rP Phabricator
Branch
arcpatch-D17590 (branched from master)
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 16272
Build 21625: Run Core Tests
Build 21624: arc lint + arc unit

Event Timeline

Side note, I'd love a first class "hasClass" type Javelin function.

Not having this method is an explicit design decision, and I don't ever plan to provide this method. See here for some discussion, under "Sigils":

https://secure.phabricator.com/book/javelin/article/sigils_metadata/

Briefly:

  • At Facebook, an innocent designer rounded the corners of a dialog and broke the dialog behavior. This was the custom privacy dialog, and the dialog no longer saved changes when submitted, so you'd set your post to "just me" and save, but the setting would stay "public".
  • This was because the Javascript relied on some CSS class like dialog-no-rounded-corners to dictate logical behavior (the class name wasn't quite this ridiculous, but not far off).
  • Not having hasClass() means that it is much harder for change which makes design adjustments to also change behavior, because we know that (except in rare, extreme cases) no Javascript depends on whether a class is present or not, since we don't have a method for even testing this in the library. So we can be more confident that making CSS and class adjustments is almost always safe, and almost always only affects display behavior.

In this case, imagine we move remarkup-fullscreen-mode from body to a container div. That seems totally innocent and no one would think twice about writing or reviewing it, and it would survive testing since neither the author nor reviewer would be likely to test fullscreen + send on enter, but would break this behavior silently.

This kind of code (using CSS class names for both display behavior and logical behavior) is inherently fragile. Both engineers and designers reasonably assume they can make minor changes to CSS, class names, and the DOM without breaking how the application works, and that these kinds of changes will generally only affect the UI.

A generalization of this issue is the idea of "using the DOM as a datastore". Broadly, it's almost always bad for the answer to any logical question to every rely on the state of the DOM.

Instead, your application code should store the state itself (in classes and objects), and should publish that state to the DOM as necessary, but should not read state from the DOM which could be stored in the application and read from the application instead.

These functions are generally bad:

function is_thing_enabled() {
  return has_class(document.body, 'thing');
}

function set_thing_enabled(enable) {
  if (enable) {
    add_class(document.body, 'thing');
  } else {
    remove_class(document.body, 'thing');
  }
}

These functions are generally a better (primarily: more robust) way to build this:

function is_thing_enabled() {
  return this.isEnabled;
}

function set_thing_enabled(enable) {
  // Set the state record in the application, not the DOM.
  this.isEnabled = enable;

  // Now publish the state to the DOM, which is just a view of the application.
  JX.DOM.alterClass(document.body, this.isEnabled);
}

We don't have hasClass() because 99% of the time it's used to write the bad version of is_thing_enabled().


All that said, we don't currently have a great place to store this state, since none of the other remarkup behaviors depend on global state. We probably need some kind of new state-storing thing to deal with this, plus T8200, plus T11171, plus "Sheets" (e.g., when you click a task on a workboard, load a "new browser tab" into the same page using Javascript) if we're going to pursue those. I don't really have a clear picture of what that should look like yet, and the answer probably mostly depends on what "Sheets" are going to look like.

An alternative -- and perhaps better -- approach would be to move this behavior into the remarkup area itself. That is:

  • When creating a remarkup input in PHP, have a method like setSendOnEnter(true).
  • That passes some flag to the phabricator-remarkup-assist behavior.
  • When that behavior sees a "return" key, checks the state of the control and submits the form if the "return" is valid (for example: not in fullscreen mode).

This could potentially let us fix stuff like the :3<return> bug in a more graceful way, too -- that's a minor case, but it's a situation where the behavior of the "return" key depends on a different, complicated piece of remarkup state. If we add more stuff like that in the future, it would probably be better for the control to handle send-on-enter than for Conpherence to ask the control a bunch of questions about whether send-on-enter is OK or not.

A simpler change would be to implement half of this: let PHP specify setSendOnEnter(true), have that just disable the "fullscreen" option with no other effects, and leave the rest of the code in place until we can deal with this more properly.

This revision now requires changes to proceed.Mar 31 2017, 1:53 PM

That was a top notch test plan though. I was very proud.

I think I still need conpherence-pontificate to intercept the form:submit and send it to the ThreadManager ?

  • I guess everything works as expected, awaiting final judgement from engineering.

Cool, I think this approach is a lot better. Thanks!

src/applications/conpherence/controller/ConpherenceViewController.php
181

Should be unnecessary, see below.

187

Consider "Sendon" -> "SendOn" here and elsewhere unless coining an odd neologism.

webroot/rsrc/js/core/behavior-phabricator-remarkup-assist.js
23

Consider renaming the 'fa-arrows-alt' mode (which I think is just named that for legacy reasons) into 'fullscreen', then testing edit_mode == 'fullscreen' instead of using a separate flag which stores the same state information.

390–392

You should be able to JX.DOM.listen(area, 'keydown', null, ...) without needing to put remarkup-send-on-enter on the form.

418–420

This has no effect.

This revision is now accepted and ready to land.Apr 2 2017, 2:34 PM
webroot/rsrc/js/core/behavior-phabricator-remarkup-assist.js
418–420

I think I had a form.submit() that's supposed to go after this. The main problem I was working on was the conpherence listener wasn't getting executed before form.submit() and the page would reload vs. ajax a message.

This revision now requires review to proceed.Apr 3 2017, 6:46 AM

This revision now requires review to proceed.

Hrrm, looks like we've still got a bug here somewhere.


I expect that didSyntheticSubmit should be sufficient on its own, without calling form.submit(). What issue are you seeing without form.submit()?

This revision is now accepted and ready to land.Apr 9 2017, 10:22 AM

oh wait hold on

I expect that didSyntheticSubmit should be sufficient on its own, without calling form.submit(). What issue are you seeing without form.submit()?

This revision now requires changes to proceed.Apr 9 2017, 10:22 AM

Not sure I understand what you're looking for. Without form.submit(), the form doesn't submit (it does in the Conpherence case, since it has another listener). My expectation is you want the form to submit on any config.sendOnEnter configuration. Or do you want to always need a secondary listener to handle that functionality.

Forms with workflow should also listen for didSyntheticSubmit already, so I'd expect that this works without form.submit() in (modern) comment areas if we enable sendOnEnter there for some reason.

It wouldn't work on, like, "Edit Task", but I don't think send on enter ever makes sense in that kind of form anyway.

chad edited edge metadata.
  • comments

lurvely

webroot/rsrc/js/core/behavior-phabricator-remarkup-assist.js
416–418

This has no effect again :3

This revision is now accepted and ready to land.Apr 10 2017, 9:36 PM
This revision was automatically updated to reflect the committed changes.

I guess I don't need var r = either