DevTools panel; Communications re-organisation #204
Merged
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
* Create the required DevTools page and script. * Try loading the sidebar as a Landmarks panel. * This is on Firefox only for now.
* gui.html is the root that is used to create the popup, sidebar, devtools... * gui.css is used by the various pop-ups/panels. It is a static file. * Due to the addition of the devtools panel, the sidebar and devtools panel HTML file names are prefixed for clarity. * A few DRY tweaks to the build script.
* Make background script unknown message handling more robust. * Use ports (as apparently this is the only possible way - thanks https://stackoverflow.com/a/41983437/1485308 ) to communicate between DevTools and the content script, via the background script. Note: the buttons do not work due to using forbidden APIs. Plan is to make them work and/or make them inspect their respective landmarks.
* When clicking buttons in the DevTools panel, say which landmark will be highlighted in the console.
Partly addresses #201 * Catch Rollup errors in build script. * Neaten up bundle specifying in build script. * Build DevTools panel bits for, and load them on, Chrome and Opera. * Add port-based communications from content script to background script; remove blanket message-sending. * Add port-based communications from DevTools scripts to background script; remove blanket message-sending. * Add port-based communications from pop-up and sidebar scripts to background script; remove blanket message-sending. * Remove sendToActiveTab.js. * Add a library function to check for disconnecting port errors. * Sort out calling of the keyboard settings window cross-browser; fixes #200; thanks openstyles/stylus#52 (comment) for the URL. * Inject the content script when the extension background page loads on Chrome/Opera - previously it was only injected when the extension was installed, so would not work as expected when the extension was disabled and then re-enabled. * Re-jig the Logger to not require the init() function. This was done to make it possible to use the Logger to log messages relating to the new port-based communication. However that is not presently being used and, now the odd Firefox issues with content-to-background comms have been addressed, it may not be (and the Logger could be removed in future if the DevTools panel(s) get(s) good. Due to the unified messaging system, the DevTools panel can now focus landmarks just like the other GUIs. The inspection of landmarks can be done on a separate UI elment that's only added to the DevTools panel. This should make the DevTools panel more intuitive.
* Fix the logic around connections from the content script to the background script, and dealing with orphaned. * Remove various log messages from background script now it seems to be working fine. * Simplify the function to set the badge text. * Logger queues up messages until it knows if the user wants to display them or not. When the browser returns the user's preference, the queued messages are either displayed or dropped. * Make the scanning-related messages softer (less shouty case). Partly addresses #201 - what remains is to make the error messages about not having a connection more user-friendly, and complete the FIXMEs.
...and then fix the unused messages :-). In fact one /was/ used, but the algorithm was not able to detect it, so I changed the approach to something simpler and more declarative. Also tidy build script log output slightly.
* Fix an error where '.selectedOptions' was '.selectedOption' * Remove more unused messages. * Remove obsolete comments. * Neaten up the translation in options.js. * Improve GUI/sidebar comments.
* Refactor GUI makeButton*()s. * Re-arrange GUI code for clarity. * Beginnings of inspection buttons.
This probably is not 100% robust, but has worked everywhere I have tried it so far. It occurred to me that it may be easier than I first thought to generate a reliable selector because it can be entirely specific. One way might have been to use nth-child() a lot, but that would lead to long strings. This way is simple and works well, though one area it may fall over is that it does not use nth-child at all. Need some test cases, but it feels promising. Minor code reorganisation, such as for button-making, based on these changes. The inspect button now also gives context in its ARIA label, and has a title (thus tooltip and accessible description) that shows the computed selector. Still to do: update comments and user docs. Note: this was committed without tests because there is now an extra field added to the element info objects returned by the LandmarksFinder. Need to think about whether to add the selectors to the existing tests (seems most proper) or whether to only test, or request, the selectors when needed (may improve speed, but premature optimisation?)
This stops it seeing a change immediately and increasing the pause.
* When moving to a page on which content scripts can't run, send a message of null landmarks to any open GUIs. * When a tab is activated, if there's a DevTools page open, send the landmarks to it (this was already being done for the other GUIs).
As 62 is now stable.
Note: there are still problems with the workaround for content script non-reinjection on Firefox, so whilst the solution below seems good, it doesn't actually work yet; still getting "WebExtension context not found!" errors: <https://bugzilla.mozilla.org/show_bug.cgi?id=1390715#c6>. * Change the reconnection logic to use @lydell's method for coping with the background script not already being loaded on Firefox - https://bugzilla.mozilla.org/show_bug.cgi?id=1474727#c3 - thanks; I had been using an approach whereby it would try to reconnect n times in quick succession, but timing-based solutions are a bit yucky, whereas your method is elegant :-). Addresses #201. * At least temporarily reverse the changes in 6ea4e56 to make the code simpler. * Use @rpl's method for supporting the content script not being re-injected when navigating back to a previous page on Firefox - https://bugzilla.mozilla.org/show_bug.cgi?id=1390715#c4 - also thanks; I was really stuck with this and would certainly not arrived at the same conclusion and clean approach to the workaround :-). Ref #202. * Reinstate some of the background script logging from b766e49 to help with the content script re-injection/context errors ongoing investigation. * Add a FIXME to sort regarding DevTools being persistent across reloads on Chrome-like browsers.
* Add test data for the selectors. * This has highlighted some things that need to be addressed
* This involved fixing some of the test data to use correct selectors. * The selector-creating code was re-written to include nth-child stuff where appropriate. It could do with some performance tuning and it would be nice if it could be written more declaratively again (i.e. not needing "let").
This was needed because some of the one-line expected results no longer fit due to the inclusion of the "selector" field.
On Chrome and Opera, the DevTools page is not reloaded when the rest of the extension is. I tried a few ways to mitigate this (trying to reconnect to the background script; trying to reload the DevTools page) but the code was both not neat and actually did not work, so have resorted to simply warning the user for now, which is probably a good idea as it will not happen often. This also fixes some missing blank lines in the messages file.
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.
Closes #170; Closes #201.