Summary:
Ref T2086. Ref T7014. With the persistent column, there is significant value in retaining chrome state through navigation events, because the user may have a lot of state in the chat window (scroll position, text selection, room juggling, partially entered text, etc). We can do this by capturing navigation events and faking them with Javascript.
(This can also improve performance, albeit slightly, and I believe there are better approaches to tackle performance any problems which exist with the chrome in many cases).
At Facebook, this system was "Photostream" in photos and then "Quickling" in general, and the technical cost of the system was //staggering//. I am loathe to pursue it again. However:
- Browsers are less junky now, and we target a smaller set of browsers. A large part of the technical cost of Quickling was the high complexity of emulating nagivation events in IE, where we needed to navigate a hidden iframe to make history entries. All desktop browsers which we might want to use this system on support the History API (although this prototype does not yet implement it).
- Javelin and Phabricator's architecture are much cleaner than Facebook's was. A large part of the technical cost of Quickling was inconsistency, inlined `onclick` handlers, and general lack of coordination and abstraction. We will have //some// of this, but "correctly written" behaviors are mostly immune to it by design, and many of Javelin's architectural decisions were influenced by desire to avoid issues we encountered building this stuff for Facebook.
- Some of the primitives which Quickling required (like loading resources over Ajax) have existed in a stable state in our codebase for a year or more, and adoption of these primitives was trivial and uneventful (vs a huge production at Facebook).
- My hubris is bolstered by recent success with WebSockets and JX.Scrollbar, both of which I would have assessed as infeasibly complex to develop in this project a few years ago.
To these points, the developer cost to prototype Photostream was several weeks; the developer cost to prototype this was a bit less than an hour. It is plausible to me that implementing and maintaining this system really will be hundreds of times less complex than it was at Facebook.
Test Plan:
My plan for this and D11497 is:
- Get them in master.
- Some secret key / relatively-hidden preference activates the column.
- Quicksand activates //only// when the column is open.
- We can use column + quicksand for a long period of time (i.e., over the course of Conpherence v2 development) and hammer out the long tail of issues.
- When it derps up, you just hide the column and you're good to go.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T2086, T7014
Differential Revision: https://secure.phabricator.com/D11507
Summary:
Fixes T7054. Fixes T7049.
- Stop scrolling Differential reticles when the page scrolls.
- Make dialogs aware of multi-panel UI.
Test Plan:
- Dialogs pop up in the right place.
- Inline + scroll now longer leaves the inline in a fixed position.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T7049, T7054
Differential Revision: https://secure.phabricator.com/D11523
Summary:
When JX.Scrollbar activates, the page needs to be clicked before scrolling keys work.
Instead, set focus into the content after we set the page frame (if something else isn't already focused).
Also fixes T7042.
Test Plan: In Safari, Chrome and Firefox, scrolling with key commands is now immediately active.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T7042
Differential Revision: https://secure.phabricator.com/D11508
Summary: See rPea67a8ab8e58. This was typed wrong and all the things I tested called scrollToPosition() directly.
Test Plan: Quoted a comment.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley, krzysztof.ciebiera
Differential Revision: https://secure.phabricator.com/D11505
Summary: Fixes T7033. When we've reframed the main page content we need to scroll relative to the containing frame, not relative to the window.
Test Plan:
In Safari, Chrome and Firefox, used j/k/J/K keys to navigate diff content.
Tried some other scroll-based beahviors, like jump-to-anchors.
(It looks like the highlighting reticle got slightly derped a while ago, but it's still functional, so I didn't mess with it.)
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T7033
Differential Revision: https://secure.phabricator.com/D11490
Summary:
Fixes two issues:
- In Firefox, dragging outside the window and releasing the mouse button would miss the `mouseup` event. This would leave the bar dragging, even though the user had released the mouse button.
- In all browsers, dragging the handle and then holding your cursor in one place for more than a second would hide the handle. Instead, never hide the handle during a drag.
Test Plan:
- In Firefox, dragged handle right (outside of window) and released mouse button. Waved cursor over window; no more "sticky" scroll.
- In FF/Chrome/Safari, dragged handle and held cursor in same position for several seconds. No more handle hide.
- Waved cursor over window and made sure normal hiding still works.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11487
Summary: See D11472. I eyeballed the "140" number by screenshotting / measuring in Paint.
Test Plan: Made the snapback thing return `true` and got snapback on OSX.
Reviewers: chad
Reviewed By: chad
Subscribers: avivey, epriestley
Differential Revision: https://secure.phabricator.com/D11485
Summary: In Safari, Firefox and Chrome drags outside the window will work if we do this. Safari didn't work before, not sure about the other two.
Test Plan: Clicked the scroll handle, then dragged my mouse to the right (outside the window) and down. Page scrolled in Safari, Firefox, and Chrome.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11484
Summary:
See <rPc40bc0c8bf75#4050>. Repro steps:
- Scroll partway down the page.
- Click and drag the scroll handle.
Prior to this diff, the handle incorrectly jumps back to the top of the page. This is because we didn't store the handle's original position. (In testing, I always dragged from near the top of the page, and I don't normally drag scrollbars, so I didn't notice this.)
Test Plan: Clicking and dragging a partially scrolled handle now works correctly.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11483
Summary:
See discussion on rPc40bc0c8bf75. Fixes a couple of glitchy things:
- Things were generally not nice on iOS.
- On OSX, with no mouse, the OS scrollbar and our fake scrollbar would both draw.
- Bar z-index was not set quite correctly.
Specifically, check if we need these bars. If we don't, just exit immediately and use the OS bars.
Test Plan:
- Tested Safari, Firefox, Chrome with and without a mouse.
- Tested iOS Simualtor.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D11480
Summary:
Ref T7014. With a mouse plugged in, multi-panel UIs are pretty hideous on OSX. This is somewhat offputting for me in Conpherence, and really jumps out at me with the new column mocks in T7014.
Sites like Twitch and Facebook approach this by emulating the touchpad scrollbar to achieve a more aesthetic UI. Use a similar approach.
This:
- Replaces the main scrollbar with a prettier fake one.
- This prepares the standard page frame for a persistent chat column.
Test Plan:
- Seems to work properly on OSX, Chrome and Firefox. Haven't tested on IE; my Windows setup is pretty iffy at the moment.
- Tried Conpherence.
- Tried Workboards.
Reviewers: btrahan, chad
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7014
Differential Revision: https://secure.phabricator.com/D11472
Summary: This variable is used before it is defined.
Test Plan: `arc lint`
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11434
Summary: Enable strict mode for Javelin when running in NodeJS.
Test Plan: Made sure Aphlict still worked.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11432
Summary:
This was broken in D11383. Basically, I had the `ws` module installed globally whilst testing, but the changes made do not work if the `ws` module is installed locally (i.e. in the `./support/aphlict/server/node_modules` directory). After poking around, it seems that this is due to the sandboxing that is done by `JX.require`.
A quick fix is to just //not// use `JX.require`, although you may have a better idea?
The error that is occurring is as follows:
```
<<< UNCAUGHT EXCEPTION! >>>
Error: Cannot find module 'ws'
at Function.Module._resolveFilename (module.js:338:15)
at Function.Module._load (module.js:280:25)
at Module.require (module.js:364:17)
at require (module.js:380:17)
at extra.require (/usr/src/phabricator/webroot/rsrc/externals/javelin/core/init_node.js:48:16)
at /usr/src/phabricator/support/aphlict/server/lib/AphlictClientServer.js:10:17
at Script.(anonymous function) [as runInNewContext] (vm.js:41:22)
at Object.JX.require (/usr/src/phabricator/webroot/rsrc/externals/javelin/core/init_node.js:58:6)
at Object.<anonymous> (/usr/src/phabricator/support/aphlict/server/aphlict_server.js:102:4)
at Module._compile (module.js:456:26)
>>> Server exited!
```
Test Plan: Now able to start the Aphlict server.
Reviewers: joshuaspence
Reviewed By: joshuaspence
Subscribers: Korvin, epriestley
Maniphest Tasks: T6987
Differential Revision: https://secure.phabricator.com/D11425
Summary: What do you think this is, PHP?
Test Plan: Eyeball it.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11361
Summary:
Fixes T4656. Helps users with this naming convention, which is probably not super duper rare.
Users will need to make an edit to a project -or- run bin/search index "#project-tag" to make this actually work.
Test Plan: made a project "[T4656test]". Typed "t4" and project showed up!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T4656
Differential Revision: https://secure.phabricator.com/D11302
Summary:
Ref T6559. Adds a "JX.Leader" primitive to replace the synchronization over Flash. This does a couple of things:
- Offers an "onBecomeLeader" event, which we can use to open the WebSocket.
- Offers an "onReceiveMessage" event, which we can use to dispatch notifications and handle requests to play sounds and send desktop notifications.
- Offers a "broadcast()" method, which we can use to send desktop notification and sound requests.
Test Plan:
Added some code like this:
```
if (!statics.leader) {
statics.leader = true;
JX.Leader.listen('onBecomeLeader', function() {
JX.log("This tab is now the leader.");
});
JX.Leader.listen('onReceiveBroadcast', function(message, is_leader) {
JX.log('[' + (is_leader ? 'As Leader' : 'Not Leader') + '] ' + message);
});
JX.Leader.start();
}
```
Then:
- Saw first tab open become leader reliably in Safari, Chrome, Firefox.
- Saw new tab become leader reliably when the first tab was closed.
- Saw broadcast() work as documented and deliver messages with correct leadership-flag and uniqueness.
Reviewers: joshuaspence, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6559
Differential Revision: https://secure.phabricator.com/D11235
Summary: Ref T6559. Wraps WebSocket in a reasonable driver class which does event dispatch, some state management, and handles automatic reconnect.
Test Plan: In Safari, Firefox and Chrome, connected to a websocket server and sent messages back and forth. Terminated and restarted server, saw automatic reconnects successfully reestablish a connection on all browsers.
Reviewers: btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T6559
Differential Revision: https://secure.phabricator.com/D11252
Summary:
Fixes T5251. We don't recompute tokenizer metrics accurately after a paste event.
Listen for paste events and redraw the input.
Test Plan: Pasted long text into a tokenizer in Safari, Firefox and Chrome.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5251
Differential Revision: https://secure.phabricator.com/D10442
Summary:
Fixes T5727. Updates the regexes to split on '-'. Also changes the editor such that tokens are updated by the larger search process. (Note this means we update this data more often then we need to - for every project transaction.)
Users will need to make an edit to a project -or- run `bin/search index "#project-tag"` to make this actually work.
Test Plan: Made "Frontend-Engineering", "Engineering", and "Backend-Enginering". They all showed up in the typeahead!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5727
Differential Revision: https://secure.phabricator.com/D10247
Summary:
Ref T5240. This fixes dragging stuff and using the mousewheel or keyboard to scroll the window during the drag.
(It does not fix "dragging near the edge of the container should scroll it" yet.)
Test Plan: Dragged stuff around on task list and workboards in Safari, Firefox and Chrome. Used mousewheel and shift + mousewheel to scroll the document and containers during drag. Object remained under the cursor.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5240
Differential Revision: https://secure.phabricator.com/D10186
Summary:
Ref T5366. Currently, enabling `notification.debug` fails to produce any messages unless developer mode is also enabled.
There's no reason we need to disable JX.log in production, and the byte size of the method is very small. Always provide a real JX.log implementation.
Test Plan: Saw notification debugging without needing to enable developer mode.
Reviewers: btrahan, joshuaspence, hach-que
Reviewed By: hach-que
Subscribers: epriestley
Maniphest Tasks: T5366
Differential Revision: https://secure.phabricator.com/D10104
Summary:
Ref T4420. Fixes T5473. Currently, when typeahead results get redrawn, you can lose your cursor position. A simple way to reproduce this is type "dif", select "Differential" using the arrow keys, then type "f". The selection will be lost.
Instead: store the old selection, then look for an item with the same name in the new set and select it. In effect, this preserves any focus selection.
Test Plan:
- Typed "dif".
- Typed "down arrow key" to select "Differential".
- Typed "f".
- "Differential" remained selected.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5473, T4420
Differential Revision: https://secure.phabricator.com/D9900
Summary: I'm pretty sure that `@group` annotations are useless now... see D9855. Also fixed various other minor issues.
Test Plan: Eye-ball it.
Reviewers: #blessed_reviewers, epriestley, chad
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin, hach-que
Differential Revision: https://secure.phabricator.com/D9859
Summary:
Fixes T5273. Two issues:
- If a JX.Resource callback throws, we keep running it. Instead, make sure it gets cleaned up before raising an exception.
- The countdown timer script doesn't recover gracefully if the node has been removed from the document by the time it runs. Instead, just bail if we can't find the countdown.
Test Plan: Dumped `{Cxyz}` into a preview and got a countdown.
Reviewers: yungsters
Reviewed By: yungsters
Subscribers: epriestley
Maniphest Tasks: T5273
Differential Revision: https://secure.phabricator.com/D9399
Summary:
A few fixes for IE that makes this work in IE9+ and at least you can drag things and have it fail horribly in IE8 now. Ref T5023.
- try / catch around getBoundingClientRect because IE throws if the object is not attached to the DOM. The correct value can be inferred in these cases as 0,0
- turns out IE has different values for button here (1,2,4) than other browsers. special case this by using the other IE fact that 'which' will be null
Test Plan: dragged and dropped tasks successfully in IE9, IE10, and IE11 on Windows 7. dragged and dropped tasks unsuccessfully in IE8 on Windows 7 - they always "drop" to the 2nd position or so as I think the ondrag code isn't firing properly or something
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5023
Differential Revision: https://secure.phabricator.com/D9118
Summary:
Allows you to quickly search for files within a repository. Roughly:
- We build a big tree of everything and ship it to the client.
- The client implements a bunch of Sublime-ish magic to find paths.
Test Plan: {F154007}
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley, zeeg
Differential Revision: https://secure.phabricator.com/D9087
Summary:
Fixes T430. Fixes T4834. Obsoletes D7641. Currently, we do some things less-well than we could:
- We just let the browser queue and prioritize requests, so if you load a revision with 50 changes and then click "Award Token", the action blocks until the changes load in most/all browsers. It would be better to prioritize this action and queue it immediately.
- Similarly, changes tend to load in order, even if the user has clicked to a specific file. When the user expresses a preference for a specific file, we should prioritize it.
- We show a spinning GIF when waiting on requests. This is appropriate for some types of reuqests, but distracting for others.
To fix this:
- Queue all (or, at least, most) requests into a new queue in JX.Router.
- JX.Router handles prioritizing the requests. Principally:
- You can submit a request with a specific priority (500 = general content loading, 1000 = default, 2000 = explicit user action) and JX.Router will get the higher stuff fired off sooner.
- You can name requests and then adjust their prorities later, if the user expresses an interest in specific results.
- Only use the spinner gif for "workflow" requests, which is bascially when the user clicked something and we're waiting on the server. I think it's useful and not-annoying in this case.
- Don't show any status for draft requests.
- For content requests, show a subtle hipster-style top loading bar.
Test Plan:
- Viewed a diff with 93 changes, and clicked award token.
- Prior to this patch, the action took many many seconds to resolve.
- After this patch, it resolves quickly.
- Viewed a diff with 93 changes and saw a pleasant subtle hipster-style loading bar.
- Viewed a diff with 93 changes and typed some draft text. Previews populated fairly quickly and there was no spinner.
- Viewed a diff with 93 changes and clicked something with workflow, saw a spinner after a moment.
- Viewed a diff with 93 changes and clicked a file in the table of contents near the end of the list.
- Prior to this patch, it took a long time to show up.
- After this patch, it loads directly.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T430, T4834
Differential Revision: https://secure.phabricator.com/D8979
Summary:
Dropdown menus are entirely dynamic right now and use custom CSS. Begin rebuilding them to use ActionList CSS.
This introduces PHUIX components which are basically JS copy/pastes of the PHP PHUI components, just implemented in JS.
We have two other dropdowns: policy controls and one in Conpherence. I'll convert those, then implement D8966.
Test Plan: {F150418}
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D8973
Summary: We were correctly invoked a didSyntheticSubmit event on the form, but nothing was listening to it. Re-jigger the workflow submit code a tad so an onsyntheticsubmit event handler can be written to fill this gap. Fixes T4669.
Test Plan: edited comments, submitting both via clicking the button and apple + enter LIKE A BOSS
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4669
Differential Revision: https://secure.phabricator.com/D8961
Summary: Ref T4714, tooltips in fullscreen mode need special math, due to fixed position throwing off position of tooltips.
Test Plan: Create work board, create several tasks, create several columns, drag tasks among columns and within columns. Create a paste, check tooltips in comment box show and are positioned correctly. Fullscreen comment box. Verify tooltips still show and position correctly.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T4714
Differential Revision: https://secure.phabricator.com/D8762
Summary: Bad news @cpojer @tomo. IE8 doesn't like you.
Test Plan: Load a diff in IE8; see changes and don't get JS errors.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, tomo, Korvin, cpojer
Differential Revision: https://secure.phabricator.com/D8728
Summary: Being bold. I think it is fine to do this in 2011.
Test Plan: Run the new specs.
Reviewers: mroch, tomo, epriestley
Reviewed By: epriestley
Subscribers: chad, epriestley, aran
Differential Revision: https://secure.phabricator.com/D835
Summary: Ref T4587. Add an option to automatically generate a keypair, associate the public key, and save the private key.
Test Plan: Generated some keypairs. Hit error conditions, etc.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: aran, epriestley
Maniphest Tasks: T4587
Differential Revision: https://secure.phabricator.com/D8513
Summary:
Ref T4324.
- Create `Listener` to represent listening clients.
- Create `ListenerList` to represent the current list of clients.
- Create `Logfile` to handle logging.
Test Plan: Clicked "Send Test Notification", verified logs, status and notifications all work correctly.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4324
Differential Revision: https://secure.phabricator.com/D8256
Summary:
Ref T4324. The server code is probably going to get a fair amount more complicated, so allow it to load Javelin classes in a mostly-reasonable way.
This integration has a few warts, but should be good enough to let us manage complexity through the next iteration of the server.
(Mostly I just want the concicse Javelin mechanism for defining new classes.)
Version bump is just so I can figure stuff out if this creates any issues for users based on which version of things they're running.
Test Plan: Started server, posted some messages through it.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4324
Differential Revision: https://secure.phabricator.com/D8253
Summary: Ref T4441. In D8250 I added code to drop results if they don't match
the current typeahead state, but with the OnDemand source we were incorrectly
comparing normalized values to denormalized values. Instead, pass and compare
raw values.
Auditors: btrahan, chad
Summary: Ref T4420. I think the border is good enough without this spinny thing, and it needs a lot of code.
Test Plan: Used typeahead.
Reviewers: chad, btrahan
Reviewed By: chad
CC: chad, aran
Maniphest Tasks: T4420
Differential Revision: https://secure.phabricator.com/D8249
Summary:
Ref T4420. After the changes to the tokenizer, I sometimes do this:
- Type something like "diff" into a project typeahead.
- Select "differential".
- A fraction of a second later, the typeahead pops back open.
This is because I selected the result from a partial query (like "diff" running against the "di" results) and then the full results of the "diff" query came back to the browser.
Instead, when showing results, require that the current state match the state that the results are for: don't show "dog" results if the tokenizer now reads "cat", for whatever reason.
Test Plan: Added a 1s delay to results, typed "a", then typed "m" and selected a result in less than a second. Prior to the patch, the tokenizer would pop back open with "am" results afterward. Now, it doesn't.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T4420
Differential Revision: https://secure.phabricator.com/D8250
Summary:
Ref T4420. When a result list contains both open and closed results, hide the closed results. I think this has a good chance of almost always working, and feeling very intuitive. It has a small chance of being a weird mess. It feels reasonable to me so far
The one bad case I can come up with here is that if you have results which shadow each other, like "Apples" (a closed project) and "Apples and Bananas" (an open project), it is impossible to get "Apples" in the result list, because "Apples and Bananas" will always shadow it. Let's wait for someone to hit this before we figure out how to deal with it.
Test Plan: Typed through open stuff to hit closed stuff.
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T4420
Differential Revision: https://secure.phabricator.com/D8238
Summary:
Ref T4420. Fixes T3309. Two major UX issues here:
- When the user extends a query ("alin" -> "alinc"), we currently hide all the results, then show them again when the new results arrive. This makes the typeahead feel a bit flickery. Instead, show matching results, then add more results when everything arrives.
- When loading more results from ondemand sources, we currently do not give you any indication that things are loading. Instead:
- Show a loading GIF (this might need #design help, @chad).
- Slightly lighten the control border.
- I didn't want to do anything like actually add "loading" text because it would cause UI flicker in the 'extend a query' case and some other cases, but otherwise this design is totally made up.
Test Plan: Typed into tokenizers and extended queries, got a better-feeling UI.
Reviewers: chad, btrahan
Reviewed By: btrahan
CC: chad, aran
Maniphest Tasks: T3309, T4420
Differential Revision: https://secure.phabricator.com/D8233
