Skip to content
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.

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

Parsers and state in content #962

Merged
merged 11 commits into from Sep 4, 2018

Conversation

@saulrh
Copy link
Contributor

commented Sep 1, 2018

Substantive changes:

  • Split state.ts into state.ts and content_state.ts, then removed mode from state and moved it to content_state.
  • Moved a couple excmds from background into content since they depended heavily on the mode and should live with the mode
  • Split the controller in half, moving the parser logic to the content script and leaving acceptExCmds in the background version. controller_content forwards background script for executing.
  • Nuked keydown_*, since the parsers are now in the content controller and we can use them directly and decide whether to suppress the event immediately.
  • Nuked hinting_background and finding_background, since we no longer round-trip keyboard events through the background and the content controller can directly invoke the content-script hinting.ts and finding.ts.
  • Nuked msgsafe, since we no longer need to shuttle keyboard events between content and background.

New features:

  • Mode state is now tab-local. You can enter ignore mode in one tab, move to another one, tridactyl around in normal mode on that tab, and then move back to the first tab and you're still in ignore mode.
  • Hinting state is now tab-local. You can start hinting in one tab, get halfway through, move to another tab, tridactyl around in that tab, and then move back to the first tab and finish hitting the hint.
  • Consequently: parsers are all tab-local. This removes a ton of round-tripping of keyboard message events and related jankiness and responsiveness problems.
  • Consequently: key suppression is much less janky because we're using the result of the same parser invocation to both drive excmds and decide whether to suppress, so the decision can't diverge.

Observed regressions:

  • # Error: Could not establish connection. Receiving end does not exist. 4 [object Object] on closing a tab (regardless of how it is closed). The number changes.

  • # Error: Could not establish connection. Receiving end does not exist. 9 [object Object] when switching tabs (with Ctrl-Tab) through an unresponsive / loading tab - e.g. https://goodblock.gladly.io

    I believe that these were some of my debugging code surfacing an existing error which has been fixed in another commit, see #963.

  • # DataCloneError: The object could not be cloned. 5 [object Object] on pressing any key in any hint mode

    This was because I hadn't migrated hinting to the content controller yet. As a result it was trying to send a full keyboard event over messaging and exploding. Migrating hinting into content fixed this.

  • preventDefault etc. seem to have stopped working - ctrl-{f,b,d} all trigger their Firefox default actions (e.g. open find, open bookmarks, bookmark page) as well as the Tridactyl bind.

    This was because I deleted keydown_*.ts before noticing it was actually doing something. It was resolved by moving suppression into the content controller's parsing.

  • I think there may be something wrong with :help and :tutor, maybe they're actually running two copies of tri or the tri content script, but I'm not sure - I haven't looked at either of them for more than a few seconds at a time.

    Just my imagination - half-loaded temporary copy of Tri.

  • autocommands related to document start/end and tab enter/leave may behave materially differently now; I'll need to look at those to make sure the new behavior is reasonable.

  • autocommands will certainly need to be used differently for things like domain-specific ignore mode, so I need to update:

    • :help
    • The sample tridactylrc
    • The blacklist function
    • Possibly :tutor
  • Update docs in :tutor

  • Update docs in :help

  • I probably need to update readme.md, if only to brag about tab-local state meaning we're starting to be even cooler than penta

More features that I want to see if I can get done (these don't need to get done on this PR, they'll probably be spun off into their own issues, but I want a place to list things I run into before we merge this and can start making issues based on it):

  • Right now excmd execution is forwarded by the content controller to the background controller, which executes the background version of the excmd, which for content-mode excmds is a shim that forwards execution to the content script. If I have the content controller instead use the content-mode excmd code, this will be reversed - content-mode excmds will be executed immediately while background-mode excmds will be forwarded to the background script for execution, as intended.
  • See if I can reduce round-tripping related to commandline_*.ts.
    • Originating in commandline_frame.ts
    • Originating in content scripts that stage through the background, because I'm sure there're some of those.
  • Split src into src/content and src/background and push as much stuff as possible into those and lib so it's possible to keep track of which scripts are meant to be used in content, which are meant to be used in background, and which are libraries.
  • See if I can entirely remove MsgSafeKeyboardEvent, since we shouldn't be shuttling those decisions to the background any more.
  • See if I can entirely remove MsgSafeNode, since we shouldn't be shuttling those decisions to the background any more.
  • Survey autocommand functionality to see if there's any low-hanging fruit to be cleaned up by pushing autocommand execution into content.

@bovine3dom bovine3dom added the blocked label Sep 1, 2018

@bovine3dom

This comment has been minimized.

Copy link
Member

commented Sep 1, 2018

Based off the autocontainers code because I was being lazy, which I should probably undo.

Yes please - otherwise it'll be blocked by #708 for no reason.

@bovine3dom

This comment has been minimized.

Copy link
Member

commented Sep 1, 2018

Regressions I've found thus far:

  • # Error: Could not establish connection. Receiving end does not exist. 4 [object Object] on closing a tab (regardless of how it is closed). The number changes.
  • # DataCloneError: The object could not be cloned. 5 [object Object] on pressing any key in any hint mode
  • preventDefault etc. seem to have stopped working - ctrl-{f,b,d} all trigger their Firefox default actions (e.g. open find, open bookmarks, bookmark page) as well as the Tridactyl bind.
  • # Error: Could not establish connection. Receiving end does not exist. 9 [object Object] when switching tabs (with Ctrl-Tab) through an unresponsive / loading tab - e.g. https://goodblock.gladly.io

Stuff that does work:

  • composite
  • all other ex-cmds I've tried
  • having one window in ignore mode and the other window not in ignore mode ❤️
src/content_state.ts Outdated Show resolved Hide resolved

// {{{ Bad key suppression system

// This is all awful and will go away when we move the parsers and stuff to content properly.

This comment has been minimized.

Copy link
@bovine3dom

bovine3dom Sep 1, 2018

Member

I think I'm just beginning to realise how awful this was - did we really have two parsers running at the same time just so we could suppress the keys?

@saulrh saulrh force-pushed the saulrh:parsers-and-state-in-content branch 2 times, most recently from 6372ef1 to 798eded Sep 1, 2018

@saulrh

This comment has been minimized.

Copy link
Contributor Author

commented Sep 1, 2018

Force-pushed a new version that's based on master instead of #953.

@saulrh

This comment has been minimized.

Copy link
Contributor Author

commented Sep 1, 2018

# Error: Could not establish connection. Receiving end does not exist. 9 [object Object] when switching tabs (with Ctrl-Tab) through an unresponsive / loading tab - e.g. https://goodblock.gladly.io

This looks to have not been a regression but actually something I surfaced with some of the debug logging I scattered around. It's fixed in #963 and I've rebased to include that PR.

@cmcaine

This comment has been minimized.

Copy link
Member

commented Sep 1, 2018

@saulrh saulrh force-pushed the saulrh:parsers-and-state-in-content branch from 17f40d9 to d1c9241 Sep 1, 2018

@saulrh

This comment has been minimized.

Copy link
Contributor Author

commented Sep 1, 2018

Hinting is now working again, and with far cleaner code to boot - No more round-trips through the background!

Now to get the same for excmds. ^__^

@saulrh

This comment has been minimized.

Copy link
Contributor Author

commented Sep 2, 2018

I think I have a weird regression where the content script stops responding on one page and then refuses to load on any further tabs or pages - it doesn't even appear to get to "content script loaded, boss!".

@saulrh

This comment has been minimized.

Copy link
Contributor Author

commented Sep 2, 2018

...Nevermind, pretty sure that that "regression" is actually a temp-installed addon not being totally decoupled from its files, so when I accidentally invoke "build an extension" after a failed invocation of the build script I get a half-loaded addon with broken everything.

@cmcaine

This comment has been minimized.

Copy link
Member

commented Sep 2, 2018

Thank you very much for this refactor :) We've known it needed to be done for a while and this will make Tridactyl a lot nicer!

@saulrh

This comment has been minimized.

Copy link
Contributor Author

commented Sep 2, 2018

I think that I managed to nuke msgsafe. In the process the type system detected and forced me to fix a bug that we probably had with our detection of elements with the ARIA role of application, which was being concealed by pick in dom.ts completely bypassing type safety.

@saulrh saulrh force-pushed the saulrh:parsers-and-state-in-content branch from 8c39371 to d16037c Sep 2, 2018

saulrh and others added some commits Sep 1, 2018

Janky proof of concept for per-tab state, incl mode and parsers
I've been programming on pure instinct for like five hours and I'm not
sure what exactly I did any more. I *think* that what I did was
something like:
* Split `state.ts` into `state.ts` and `content_state.ts`, then
  removed mode from state and moved it to content_state.
* Made the types in content_state.ts a hair more powerful
* Fixed all errors resulting from those two changes, in the process
  swapping state out for content_state wherever appropriate.
* Moved a couple excmds from background into content since they
  depended heavily on the mode and should live with the mode
* Split the controller in half, moving the parser logic to the content
  script and leaving acceptExCmds in the background
  version. content_controller forwards excmds to background script
  using messaging.
* Nuked the keydown_* code, since we no longer need to forward keys to
  the background.
* Went around fixing everything resulting from *those* changes, mostly
  the various perversions of keydown listeners
* Various tweaks here and there that probably didn't do anything but I
  was just changing things at random a third of the time and I really
  need to go through this and clean it up.

Things that work:
* excmds are nice now. each tab has its own commandline; they stay
  open when you navigate between tabs and you can come back to them
  after doing stuff in another tab and finish your command line input.
* keybinds that don't don't involve mode switching: `[[`, `]]`, `j`,
  `k`, `r`, etc, all still functional.
* You can turn on hinting mode and, again, navigate to another tab
  which will *not* be in hinting mode, do whatever you need to do
  there including excmds or more hinting, and then come back to finish
  the hint you started in the first tab
* You can exit hint mode by pressing escape. :P

Things that I broke:
* ...Actually quite a bunch of stuff, I think.
* I don't seem to be able to *finish* a hint any more. Sometimes I
  can't even get one key in.
Un-regress hinting and finding
Turned out this was simple - I'd forgotten to switch the controller to
use the content versions of hinting and finding, so the controller was
using the background versions which were messaging-based proxies for
the content versions that weren't running any more. Calling the
content versions directly fixed hinting immediately.
`blacklistadd` now only needs `DocStart` since mode is per-document
Per-document modes might actually leave `TabLeft` completely unused by
most people, so we should be careful to not let that code start to
decay.

Sample tridactylrc turned out to not need updating

It looks like help docs don't reference this functionality anywhere
else, so this should be all the autocommand-related changes.
Nuke msgsafe, since no need to message events any more, and fix ARIA
`pick` in `msgsafe.ts` was not type safe and was concealing an error
where HTMLElements don't have a `pick` property. Then
`MsgSafeNode.pick` would have always been absent, which would have
caused `isTextEditable` to fail to detect elements with the ARIA role
of `application`. Using real types throughout allows us to fix this by
iterating over `Element.attributes`, which appears to do the right
thing.

@saulrh saulrh force-pushed the saulrh:parsers-and-state-in-content branch from d16037c to 58c3ed6 Sep 2, 2018

@saulrh

This comment has been minimized.

Copy link
Contributor Author

commented Sep 4, 2018

I think I've resolved all identified regressions and this PR is good for review. I wouldn't be surprised if it was covered in style guide violations; if you want to get it merged tonight you can go ahead and fix them yourself. The remaining checkboxes are all feature requests that I was hoping to get in before someone merged it, but it looks like the remainder are big enough that they'll want to have their own issue numbers and PRs and I'll move them to the issue tracker when appropriate.

@bovine3dom bovine3dom merged commit 58c3ed6 into tridactyl:master Sep 4, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bovine3dom bovine3dom referenced this pull request Dec 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.