feat(toggle): Show all landmarks keyboard shortcut #245
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* Handle Enter keydown events. * Use tabindex=0 rather than href=# so that when the user returns to the page, they are not moved, and the install/update status is not lost. * Tidy up some FIXMEs (can address later; it is generally tidier now than it was).
* Change the reset settings message to indicate it resets to defaults. * Handle show all command in content script (does not toggle yet). * Avoid spurious note creation if GUI setting does not really change. * Reset settings to defualts rather than clearing entire storage area. * Add a BorderManager to handle drawing of borders on element(s). * ElementFocuser concnetrates on focusing one element. * Add a function to enumerate all landmarks to LandmarksFinder. * Update ID of reset button in options to make it clearer. * Do not draw a border around an element that already has one. Several FIXMEs to address - notably need to ensure that toggled-on borders remain present when the user has momentary borders and moves between elements, support toggling off, and work out if ElementFocuser should continue to act as a pass-through, but making good progress.
* Keep it clean by only "new"ing things in the content script. * Ensure that uses of "window" and "document" are "escaped". * Clarify variable names in the content script.
* Move things to BorderManager that it should be doing. * Update calls from content script. * Tweak some use of defining public APIs. Need to address potentially doing some stuff simpler using the "that = this" technique.
Use this.removeBorderOnCurrentlyFocusedElement() for both public and private calls.
Use this.addBorder() for public and private API.
* Improve function names. * Reflect latest responsibilities split. * Add some comments to address.
...to make it clear that after a change to the document, nothing would automatically be added, but the current state reflected as far as possible (i.e. removing border on focused element if it has gone; updating all borders). Currently it does not remove borders on elements that no longer exist when it is trying to resize the borders. This will lead to errors, but there is already a FIXME about it.
When all landmarks are showing, the borders are not added, nor removed, when moving between landmarks. BorderType preference changes also have no effect. Other border prefrences do, though.
This fixes ElementFocuser to properly handle the change of borderType. Now the problem I thought I should find in the previous commit, but could not reproduce, is here :-).
* Clearer API names. * More logical public/private split. * Fix issue of borders that can disappear due to settings change. * Remove need for "that = this".
Originally this seemed like a good idea because it keeps the state apart from the ElementFocuser, but there were several things in the ElementFocuser that needed to keep track (because the prefs code could run at any time), so in the end it made sense to manage the state there.
* Clear up logic and function names for dealing with removed elements when refreshing in BorderDrawer. * Fix typo that was breaking the adding of borders to newly-added landmarks.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Naughty! This really should’ve been split up into smaller PRs, though I think I am starting to get into this new commit style :-). Anyway, this does a couple of things:
In support of this, it required splitting the ElementFocuser into an ElementFocuser and a BorderDrawer. A lot of API naming cleanup and code tidying was done too.
A further PR will add UI to the pop-up that mouse users can use to access this feature.