search

refined-github / refined-github

:octocat: Browser extension that simplifies the GitHub interface and adds useful features
MIT License
24.23k stars 1.47k forks source link

Back/forth navigation is delayed #6028

Closed kaste closed 1 year ago

kaste commented 1 year ago

Description

See detailed below:

How to replicate the issue

  1. Starting on https://github.com/refined-github/refined-github/issues?q=is%3Aissue+is%3Aopen+sort%3Aupdated-desc
  2. Navigate to the issue 6000 https://github.com/refined-github/refined-github/issues/6000
  3. Now "Go back" in the browser (you feel a delay here)
  4. Now "Go forth" (you feel the same delay)

I count roughly to 2 (seconds).

The performance tab shows nada, just wait time

image

The starting task (on the right) after the presumed waiting is a timer

image

pointing to

image

which has this code

image

So it looks like we timeout after 2000 and that's also the delay I have. What's interesting is that we wait for a styleelement here.

image

Without refined enabled, I don't run this code here. T.i. with Refined this code here starts with a on history.pop event ("Going forth" navigation). Without Refined there is some flag, some this.enabled somewhere I forgot, which is false so we never reach much further in the code.

I'm actually no expert here, this is hooray let's start a debugger I've not used for years, so everything can really be wrong here.

Extension version

22.9.25

Browser(s) used

brave

kaste commented 1 year ago

It was this enabled check just one step in the stack

image

So just from reading "turbo disabled" it looks like Refined has turbo for such a navigation enabled, and GitHub itself (without Refined) has it disabled. But that's also wild on my side of course.

fregante commented 1 year ago

Thanks for the debugging. I don't see anything that Refined GitHub could interact with, we don't deal with navigation. The only thing we do occasionally is mark our own links with an old "ajaxed" marker, but you clicked a native link so that's not covered.

If you can replicate it consistently, would you mind using "Find feature..." in Refines GitHub's options page?

fregante commented 1 year ago

Do you have other extensions installed? Like Octotree

kaste commented 1 year ago

Holy shit you prepared here. bisect gives https://github.com/refined-github/refined-github/blob/main/source/features/linkify-user-location.tsx (I had all extensions disabled, I don't have many and not Octotree anyway.) But disabling this feature then has no effect. A second bisect had the same result.

kaste commented 1 year ago

If I start the bisect with linkify-user-location disabled it flags https://github.com/refined-github/refined-github/blob/main/source/features/parse-backticks.tsx And then with that also disabled it flags https://github.com/refined-github/refined-github/blob/main/source/features/set-default-repositories-type-to-sources.tsx as the culprit

With all three disabled the bug is gone.

kaste commented 1 year ago

It waits for a style element here which has the following content

"@keyframes rgh-selector-observer {}\n\t\t:where(.BorderGrid--spacious .f4.my-3,.js-commits-list-item pre,.js-commit-group .pr-1 code,.js-commit-group pre,.release-header,.Box-row .mb-1 a,#pull-requests a.Link--primary,[id^=\"check_suite\"] a.Link--primary,.js-socket-channel[data-url*=\"/header_partial\"] h3,.js-wiki-sidebar-toggle-display a,#wiki-wrapper .gh-header-title,.issues_labeled .color-fg-default > a,#user-repositories-list [itemprop=\"description\"],.js-hovercard-content > .Popover-message .Link--primary,.js-discussions-title-container h1 > .js-issue-title,a[data-hovercard-type=\"discussion\"]):not(.rgh-seen-11509184296) {\n\t\t\tanimation: 1ms rgh-selector-observer;\n\t\t}\n\t\n\t\t:where(.comment-body a[href]:not(.rgh-linkified-code)):not(.rgh-seen-2855538752) {\n\t\t\tanimation: 1ms rgh-selector-observer;\n\t\t}\n\t\n\t\t:where(.BorderGrid--spacious .f4.my-3,.js-commits-list-item pre,.js-commit-group .pr-1 code,.js-commit-group pre,.release-header,.Box-row .mb-1 a,#pull-requests a.Link--primary,[id^=\"check_suite\"] a.Link--primary,.js-socket-channel[data-url*=\"/header_partial\"] h3,.js-wiki-sidebar-toggle-display a,#wiki-wrapper .gh-header-title,.issues_labeled .color-fg-default > a,#user-repositories-list [itemprop=\"description\"],.js-hovercard-content > .Popover-message .Link--primary,.js-discussions-title-container h1 > .js-issue-title,a[data-hovercard-type=\"discussion\"]):not(.rgh-seen-11509184296) {\n\t\t\tanimation: 1ms rgh-selector-observer;\n\t\t}\n\t\n\t\t:where(.BorderGrid--spacious .f4.my-3,.js-commits-list-item pre,.js-commit-group .pr-1 code,.js-commit-group pre,.release-header,.Box-row .mb-1 a,#pull-requests a.Link--primary,[id^=\"check_suite\"] a.Link--primary,.js-socket-channel[data-url*=\"/header_partial\"] h3,.js-wiki-sidebar-toggle-display a,#wiki-wrapper .gh-header-title,.issues_labeled .color-fg-default > a,#user-repositories-list [itemprop=\"description\"],.js-hovercard-content > .Popover-message .Link--primary,.js-discussions-title-container h1 > .js-issue-title,a[data-hovercard-type=\"discussion\"]):not(.rgh-seen-11509184296) {\n\t\t\tanimation: 1ms rgh-selector-observer;\n\t\t}\n\t\n\t\t:where(.BorderGrid--spacious .f4.my-3,.js-commits-list-item pre,.js-commit-group .pr-1 code,.js-commit-group pre,.release-header,.Box-row .mb-1 a,#pull-requests a.Link--primary,[id^=\"check_suite\"] a.Link--primary,.js-socket-channel[data-url*=\"/header_partial\"] h3,.js-wiki-sidebar-toggle-display a,#wiki-wrapper .gh-header-title,.issues_labeled .color-fg-default > a,#user-repositories-list [itemprop=\"description\"],.js-hovercard-content > .Popover-message .Link--primary,.js-discussions-title-container h1 > .js-issue-title,a[data-hovercard-type=\"discussion\"]):not(.rgh-seen-11509184296) {\n\t\t\tanimation: 1ms rgh-selector-observer;\n\t\t}\n\t\n\t\t:where(.BorderGrid--spacious .f4.my-3,.js-commits-list-item pre,.js-commit-group .pr-1 code,.js-commit-group pre,.release-header,.Box-row .mb-1 a,#pull-requests a.Link--primary,[id^=\"check_suite\"] a.Link--primary,.js-socket-channel[data-url*=\"/header_partial\"] h3,.js-wiki-sidebar-toggle-display a,#wiki-wrapper .gh-header-title,.issues_labeled .color-fg-default > a,#user-repositories-list [itemprop=\"description\"],.js-hovercard-content > .Popover-message .Link--primary,.js-discussions-title-container h1 > .js-issue-title,a[data-hovercard-type=\"discussion\"]):not(.rgh-seen-11509184296) {\n\t\t\tanimation: 1ms rgh-selector-observer;\n\t\t}\n\t"

rgh-selector-observer is probably coming from you

fregante commented 1 year ago

It's unclear why GitHub would pick our style tag and await it, but this issue could be solved by using adoptedStylesheets: https://web.dev/constructable-stylesheets/#using-constructed-stylesheets

Unfortunately it's not available in Safari yet, so we'd have to "polyfill" it https://developer.mozilla.org/en-US/docs/Web/API/Document/adoptedStyleSheets#browser_compatibility

kaste commented 1 year ago

turbo automatically does that. Just from the method names it probably diffs the dom, the <head>, see newHeadStylesheetElements. So just from the name it should not bother with stylesheets outside of <head>.

But the @keyframes thing I posted is rather long. It has more than 3 where clauses. Can I assume each feature adds 1 where clause. So my thinking was: why do all the other observers work fine but the ones (the selectors) from these 3 feature do not?

  1. Is there a timing thing that these 3 feature have a different timing than the other features?
  2. Is there a simple error in the selectors. (Naturally: why does adding the style element never fire load nor errorso that it timeouts. It does fire with the 3 feature disabled!)
  3. Could this added to the <body> and turbo will just ignore everything.
fregante commented 1 year ago

The content of style is irrelevant, the load event fires when the stylesheet is "applied" to the document, regardless of its rules, unless there are a billion of them.

2. It does fire with the 3 feature disabled!)

I think it's just a coincidence or a bug of the browser

But the @keyframes thing I posted is rather long

I do see an "issue" in it, the selectors appear to be duplicated somehow, I'd need to look into it (specifically into why the seen ID is different for each), (Edit: not really an issue) but again it shouldn't be related to it.

This is in a readable form:

@keyframes rgh-selector-observer {}
        :where(.BorderGrid--spacious .f4.my-3,.js-commits-list-item pre,.js-commit-group .pr-1 code,.js-commit-group pre,.release-header,.Box-row .mb-1 a,#pull-requests a.Link--primary,[id^="check_suite"] a.Link--primary,.js-socket-channel[data-url*="/header_partial"] h3,.js-wiki-sidebar-toggle-display a,#wiki-wrapper .gh-header-title,.issues_labeled .color-fg-default > a,#user-repositories-list [itemprop="description"],.js-hovercard-content > .Popover-message .Link--primary,.js-discussions-title-container h1 > .js-issue-title,a[data-hovercard-type="discussion"]):not(.rgh-seen-11509184296) {
            animation: 1ms rgh-selector-observer;
        }

        :where(.comment-body a[href]:not(.rgh-linkified-code)):not(.rgh-seen-2855538752) {
            animation: 1ms rgh-selector-observer;
        }

        :where(.BorderGrid--spacious .f4.my-3,.js-commits-list-item pre,.js-commit-group .pr-1 code,.js-commit-group pre,.release-header,.Box-row .mb-1 a,#pull-requests a.Link--primary,[id^="check_suite"] a.Link--primary,.js-socket-channel[data-url*="/header_partial"] h3,.js-wiki-sidebar-toggle-display a,#wiki-wrapper .gh-header-title,.issues_labeled .color-fg-default > a,#user-repositories-list [itemprop="description"],.js-hovercard-content > .Popover-message .Link--primary,.js-discussions-title-container h1 > .js-issue-title,a[data-hovercard-type="discussion"]):not(.rgh-seen-11509184296) {
            animation: 1ms rgh-selector-observer;
        }

        :where(.BorderGrid--spacious .f4.my-3,.js-commits-list-item pre,.js-commit-group .pr-1 code,.js-commit-group pre,.release-header,.Box-row .mb-1 a,#pull-requests a.Link--primary,[id^="check_suite"] a.Link--primary,.js-socket-channel[data-url*="/header_partial"] h3,.js-wiki-sidebar-toggle-display a,#wiki-wrapper .gh-header-title,.issues_labeled .color-fg-default > a,#user-repositories-list [itemprop="description"],.js-hovercard-content > .Popover-message .Link--primary,.js-discussions-title-container h1 > .js-issue-title,a[data-hovercard-type="discussion"]):not(.rgh-seen-11509184296) {
            animation: 1ms rgh-selector-observer;
        }

        :where(.BorderGrid--spacious .f4.my-3,.js-commits-list-item pre,.js-commit-group .pr-1 code,.js-commit-group pre,.release-header,.Box-row .mb-1 a,#pull-requests a.Link--primary,[id^="check_suite"] a.Link--primary,.js-socket-channel[data-url*="/header_partial"] h3,.js-wiki-sidebar-toggle-display a,#wiki-wrapper .gh-header-title,.issues_labeled .color-fg-default > a,#user-repositories-list [itemprop="description"],.js-hovercard-content > .Popover-message .Link--primary,.js-discussions-title-container h1 > .js-issue-title,a[data-hovercard-type="discussion"]):not(.rgh-seen-11509184296) {
            animation: 1ms rgh-selector-observer;
        }

        :where(.BorderGrid--spacious .f4.my-3,.js-commits-list-item pre,.js-commit-group .pr-1 code,.js-commit-group pre,.release-header,.Box-row .mb-1 a,#pull-requests a.Link--primary,[id^="check_suite"] a.Link--primary,.js-socket-channel[data-url*="/header_partial"] h3,.js-wiki-sidebar-toggle-display a,#wiki-wrapper .gh-header-title,.issues_labeled .color-fg-default > a,#user-repositories-list [itemprop="description"],.js-hovercard-content > .Popover-message .Link--primary,.js-discussions-title-container h1 > .js-issue-title,a[data-hovercard-type="discussion"]):not(.rgh-seen-11509184296) {
            animation: 1ms rgh-selector-observer;
        }

3. Could this added to the <body> and turbo will just ignore everything.

Unfortunately it seems that some elements are removed by turbo, see https://github.com/refined-github/refined-github/issues/5857

Does Turbo has an easy way to exclude our style tag from their waitlist?

fregante commented 1 year ago

Judging by the code in https://github.com/hotwired/turbo/blob/4e8ba783419cc01c0aa42d01cbca313e28b35016/src/core/drive/head_snapshot.ts#L47, style elements shouldn't even be covered as it's looking for <link type=stylesheet> elements specifically

kaste commented 1 year ago

elementIsStylesheet includes <style> tags. The type of the function getElementsMatchingTypeNotInSnapshot<HTMLLinkElement> is confusing (a lie?) because stylesheet here refers to the "type" turbo gave the element.

kaste commented 1 year ago

Do these events fire if you add the same element a second time to head? (They don't deduplicate?) (noobish, I know)

"error" should be triggered when there is an error in the selector for example I guess. So "error" is; "I've seen this but I (the browser) cannot apply it; It is not in effect now."

fregante commented 1 year ago

Even if it doesn't cover every browser, it'd be good to have a helper like:



function attachStyleSheet(css: string): CSSStyleSheet {
    if ('adoptedStylesheets' in document) {
        const sheet = new CSSStyleSheet(css);
        document.adoptedStylesheets = [sheet]
        return sheet;
    }

    const sheet = document.createElement('style')
    sheet.textContent = css;
    document.head.append(sheet)
    return sheet;
}
probablykasper commented 1 year ago

In case it wasn't fully confirmed, this is definitely caused by parse-backticks for me

LOuroboros commented 1 year ago

I don't want to open a new issue ticket if it's not needed, so allow me to ask; could this be the same issue that is causing files to load slower, at least on Firefox, while Refined GitHub is enabled?

I noticed that if I open a .c file with a considerable amount of lines such as 8k to 10k on GitHub.com with Refined GitHub enabled, it takes an extra 20 to 30 seconds to load compared to what it takes to load the same file with Refined GitHub disabled.

For reference, a file with 8266 lines loads up in ~5 seconds without Refined GitHub on my end, where as it takes around 22 while it is enabled.