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

Fix: Tie more elements from DOM snapshot back to original source #2051

Merged
merged 1 commit into from
Mar 13, 2019

Conversation

antross
Copy link
Member

@antross antross commented Mar 11, 2019

Pull request checklist

Make sure you:

For non-trivial changes, please make sure you also:

  • Added/Updated related documentation.
  • Added/Updated related tests.

Short description of the change(s)

Ref #1956

Also fix some cases where use of the snapshot vs. original document were
inconsistent between connectors (except connector-local which needs to
be different).

Notable changes:

  • All connectors now use parser-html to obtain an original source document
    • Should we consider making parser-html fully built-in since HTML handling is so special?
  • Snapshots are created with a reference to the original source document for locations
    • This means we no longer use locations from the snapshot itself (I turned off locations for snapshots to ensure this)
    • A new findOriginalElement helper was added to take an element from the snapshot and build a selector (plus filter function) to identify the same element in the original source (if one exists).
  • If an element was dynamically created, we no longer try to produce a location for it.
    • This avoids producing invalid locations that don't point anywhere in actual source.
    • In future updates some contexts (e.g. the browser extension) may link to the actual element reference instead in these cases.

Marking as "Draft" while I wait for test results which may require some updates.

@antross antross force-pushed the fix/snapshot-locations branch from 0ee15f3 to 74b4be1 Compare March 12, 2019 16:45
Copy link
Member

@molant molant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All connectors now use parser-html to obtain an original source document

I only see import for type definitions and nothing else. Can you please point me to the right place?

packages/connector-jsdom/src/connector.ts Outdated Show resolved Hide resolved
@antross antross marked this pull request as ready for review March 12, 2019 17:57
@antross antross requested a review from sarvaje as a code owner March 12, 2019 17:57
@antross antross force-pushed the fix/snapshot-locations branch 3 times, most recently from f7331a6 to 8c98422 Compare March 12, 2019 20:15
@molant
Copy link
Member

molant commented Mar 12, 2019

@antross do you want to do more changes or is this ready to be merged?

Also fix some cases where use of the snapshot vs. original document were
inconsistent between connectors (except connector-local which needs to
be different).
@antross antross force-pushed the fix/snapshot-locations branch from 8c98422 to 802bc95 Compare March 12, 2019 22:25
@antross
Copy link
Member Author

antross commented Mar 12, 2019

@molant I made one last tweak to fix the issue I discussed earlier where an element with an ID or URL could end up falling back if no match is found and matching (incorrectly) on content. Now this should be good to merge!

@antross antross merged commit 29b2274 into webhintio:master Mar 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants