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

DevTools panel; Communications re-organisation #204

Merged
merged 31 commits into from Oct 20, 2018

Conversation

1 participant
@matatk
Owner

matatk commented Oct 20, 2018

  • Add a DevTools panel that allows landmarks’ corresponding HTML elements to be inspected.
  • This entailed re-organising the communications in the code to use ports rather than message-passing. Seems better to use a consistent model (ports are required by the DevTools page/panel).

Closes #170; Closes #201.

matatk added some commits Aug 22, 2018

Experiment with creating a DevTools panel
* Create the required DevTools page and script.
* Try loading the sidebar as a Landmarks panel.
* This is on Firefox only for now.
Make a gui.html file as the root for the popup etc.
* 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 landmarks list appear in DevTools
* 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.
Make DevTools panel do something without erroring
* When clicking buttons in the DevTools panel, say which landmark will be highlighted in the console.
Port-based communications; Build tweaks; Commands; Start-up
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.
Content-to-background logic; less verbose; neaten
* 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.
Fix detecting unused messages...
...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 options; remove unused messages; Comments
* 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.
Preparing for landmark inspection
* Refactor GUI makeButton*()s.
* Re-arrange GUI code for clarity.
* Beginnings of inspection buttons.
Make the inspect buttons work
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?)
Wait to set up the mutation observer
This stops it seeing a change immediately and increasing the pause.
Refinement of handling GUIs
* 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).
Attempt to fix background comms and navigation on Firefox
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.
Get around to fixing the tests (mostly)
* Add test data for the selectors.
* This has highlighted some things that need to be addressed
Fix all but one of the tests
* 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").
Neaten the JSON test data
This was needed because some of the one-line expected results no longer
fit due to the inclusion of the "selector" field.
Warn user about DevTools disconnect; messages file
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.

@matatk matatk merged commit 3da6205 into master Oct 20, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@matatk matatk deleted the dev-tools-panel branch Oct 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment