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

GitHub: "Load diff" misplaced and non-clickable on many commits #46

Closed
Vangelis66 opened this issue Feb 19, 2022 · 29 comments
Closed

GitHub: "Load diff" misplaced and non-clickable on many commits #46

Vangelis66 opened this issue Feb 19, 2022 · 29 comments
Labels
bug Something isn't working fixed

Comments

@Vangelis66
Copy link

Browser: Serpent v52.9.0 (2022-01-19) (32-bit)
[it's platform should be on par with last official Basilisk 52.9.2022.01.27 (64-bit)]
Extension version: github-wc-polyfill-v1.12.13

STR:

Load, e.g. , 3c2384b:

st-ld

Load diff is misplaced (but this is something already pointed out here) 😉 , but is also overlaid with the message:

Some generated files are not rendered by default. Learn more.

Sadly, it appears that now you can only click on either "L" or "o" (of the whole Load diff string) to display the actual commit content, but those two letters are superimposed by the word "not" 😞 ...
The screenshot is from a dirty profile, but I have replicated the issue on a fresh St52 profile, with only said extension installed...

Toying a little with uBlock Origin 1.16.4.30, I removed the overlay with
github.com##.width-full.height-full.left-0.top-0.position-absolute.flex-justify-center.flex-items-center.d-flex and I thus got:

st-ld-2

which now allows me to generate the diff by clicking on the "Lo" area, but after a tab reload, Load diff appears greyed-out and non-clickable:

st-ld-3

I have to disable uBO on github.com, reload the tab a second time to get a non-clickable "Lo" area (due to the overlay), then re-enable uBO and reload the tab a third time to get a clickable "Lo" area (without the overlay); this is impractical ...

My instinct tells me this issue is caused by limitations in UXP's CSS/JS parsers (can't handle well the CSS code served by GitHub), so I'm not certain it could be addressed in the extension itself, but it sure is a nuisance... 😠

Any tips to mitigate the issue (e.g., some userstyle installable in Stylem/Stylus?) would be highly appreciated!

FWIW, this is how things look on a Chromium-78-based browser:

ch78-ld

... but this is no surprise, the modern web is tailored to perfectly suit only Chromium (I consider current Firefox to be just another Chromium fork, give or take...).

As always, utmost gratitude extended to the maintainer of this extension for his unwavering efforts, especially considering current RL tensions in his neck of the woods ❤️ ...

@SeaHOH
Copy link
Collaborator

SeaHOH commented Feb 19, 2022

Your rule is not suitable (big range), MUST only use the unique class name.

github.com##.hidden-diff-reason

@JustOff JustOff added bug Something isn't working help wanted Extra attention is needed labels Feb 19, 2022
@JustOff
Copy link
Owner

JustOff commented Feb 19, 2022

Try 1.2.14b1.

@SeaHOH
Copy link
Collaborator

SeaHOH commented Feb 19, 2022

The button (.load-diff-button) got an atttribute disabled when reload pages after be clicked. Reload pages using skip cache method is OK.
disabled-button

@Vangelis66
Copy link
Author

Apologies for the tardy replies... 😉

Your rule is not suitable (big range), MUST only use the unique class name.

github.com##.hidden-diff-reason

... As you'd probably imagine, the uBO rule I used and mentioned in my original post was not hand-written, but rather the result of using the built-in element picker...
BTW, I did try your code above, thanks, but arrived in the same results as the ones detailed in my original post (with regards to reloading the tab) ...

Try 1.2.14b1.

Many thanks for trying to address this 👍 ; I just finished a set of tests (fresh St52 profile with only 1.2.14b1 installed) and...

  1. The misplacement issue is correctly mitigated 👍 , see below:

P1

  1. However, you can now load the diff by even clicking on the "blue-lined" area (header?), which means the collapse (top left) and ellipsis (top right) buttons can't be accessed without loading diff...

  2. Loading the diff works fine, as do now the two previously mentioned buttons:

P2

  1. Sadly, once you reload the tab/page, the Load diff string becomes greyed-out and non-clickable, and the latter becomes also true for the "header"/buttons area (red-lined) 😢 :

P3

This last "status" is only resolved by fully restarting the browser, as mere tab-reloads/ browser-cache-purging tried here to no avail... 😞

Hopefully, we're close to a definitive solution!
Best regards 😄

@SeaHOH
Copy link
Collaborator

SeaHOH commented Feb 20, 2022

This last "status" is only resolved by fully restarting the browser, as mere tab-reloads/ browser-cache-purging tried here to no avail... disappointed

Reload skip cache be activated with Ctrl + Shift + R.

@rofl0r
Copy link

rofl0r commented Feb 20, 2022

funny, i'm seeing this too in palemoon 29.4.4 with gh polyfill 1.2.13 on this link ryanmcgrath/svgalib-1@e698d8c

@SeaHOH
Copy link
Collaborator

SeaHOH commented Feb 20, 2022

It also happen with 1.2.12. Maybe the bug(?)/feature of page status keeping.

@JustOff
Copy link
Owner

JustOff commented Feb 20, 2022

the collapse (top left) and ellipsis (top right) buttons can't be accessed without loading diff...

Try 1.2.14b2.

once you reload the tab/page, the Load diff string becomes greyed-out and non-clickable

I have no idea how to address this, fortunately, as mentioned above, Ctrl + Shift + R and Ctrl + F5 work.

@SeaHOH
Copy link
Collaborator

SeaHOH commented Feb 20, 2022

I have tested the old style frameworks-56f7ce131f05e8b45ad6787fd4b122c3, no issues except the disabled button.

@THEtomaso
Copy link

Does this uBO rule fix the issue?:
github.com##.js-diff-entry-loader:style(position: inherit !important;)

@garoto
Copy link

garoto commented Feb 20, 2022

Does this uBO rule fix the issue?: github.com##.js-diff-entry-loader:style(position: inherit !important;)

It does for me while using v1.2.13 from the releases page.

@Vangelis66
Copy link
Author

Vangelis66 commented Feb 20, 2022

funny, i'm seeing this too in palemoon 29.4.4 with gh polyfill 1.2.13

Why do you find it "funny"? 😃 Pale Moon (that's how its authors like it to be spelt 😜) is a UXP-based browser, Serpent 52 (a forked Basilisk) is built on a forked UXP which, to the best of my knowledge, bears no differences to upstream UXP as far as JS/CSS parsers are involved...

Does this uBO rule fix the issue?:

github.com##.js-diff-entry-loader:style(position: inherit !important;)

It surely does fix the misplacement 👍, with or without github-wc-polyfill-1.2.13 installed (but without this extension, clicking on Load diff does nothing, of course...).

the collapse (top left) and ellipsis (top right) buttons can't be accessed without loading diff...

Try 1.2.14b2.

Thanks! 👍 ; that solves it! 🎉

I have no idea how to address this, fortunately, as mentioned above, Ctrl + Shift + R and Ctrl + F5 work

... Sad to hear 😢 ... I'll leave this issue open for a tad longer, in the hope some bright idea 💡 comes up to someone, barring that, keyboard shortcut(s) it will be... 😉

Best wishes!

@THEtomaso
Copy link

It should be possible to add a fix, similar to the uBO rule which I posted, to the extension itself.

@JustOff
Copy link
Owner

JustOff commented Feb 20, 2022

In fact, your rule is pretty much the same as the one contained in 1.2.14b1:

.js-diff-load-container .js-diff-entry-loader {
  position: inherit !important;
}

and here is an additional fix from 1.2.14b2:

.js-skip-tagsearch > .load-diff-button, .js-skip-tagsearch > div {
  height: 75% !important;
  top: 25% !important;
}

@THEtomaso
Copy link

THEtomaso commented Feb 20, 2022

your rule is pretty much the same as the one contained in 1.2.14b1

Oh, so the remaining issue is that the fix included in your extension breaks upon page refresh?
Is that what you've now addressed in v1.2.14b2?

@SeaHOH
Copy link
Collaborator

SeaHOH commented Feb 20, 2022

Is that what you've now addressed in v1.2.14b2?

Big button overlay the commit box's header. #46 (comment)

@SeaHOH
Copy link
Collaborator

SeaHOH commented Feb 21, 2022

This injected script is working.

(function () {
  // Remove "disabled" attribute of "Load diff" buttons
  document.addEventListener('DOMContentLoaded', function () {
    for (let button of document.getElementsByClassName('load-diff-button'))
      button.removeAttribute("disabled");
  }, {once: true});
}());

@misteuk
Copy link

misteuk commented Feb 21, 2022

I fix in #48

@Vangelis66
Copy link
Author

This injected script is working.

Many thanks, but where exactly inside 1.2.14b2's bootstrap.js file (i.e. between which lines) should this be inserted? Can you submit a proper PR here?

I have already tried the very latest commit of your fork and it addresses fully all aspects of this issue 👍 , but your fork has diverged significantly from this ("upstream") to let a non-coder such as I easily apply your posted fix...
Hopefully JustOff will accommodate, when his RL permits... 😃

@Vangelis66
Copy link
Author

Vangelis66 commented Feb 22, 2022

I have already tried the very latest commit of your fork and it addresses fully all aspects of this issue 👍

@SeaHOH : I just stumbled on a very minor bug of SeaHOH-gh-wc-polyfill-1.2.14, which is selectively manifested in this very issue page, i.e.

  1. Load in a tab
    GitHub: "Load diff" misplaced and non-clickable on many commits #46
  2. With the END button (or with the scrollbar) go to the very end of the page
  3. Reload the tab
  4. Once the page has fully reloaded, you now find yourself not at the bottom of the page (expected behaviour), but somewhere in the middle of first (mine) post:

bug

NB: It ONLY happens when I am logged-in !

This small "bug" isn't there with both (upstream) 1.2.14b2 or 1.2.13+PR#48 ... 😉

@SeaHOH
Copy link
Collaborator

SeaHOH commented Feb 22, 2022

I just stumbled on a very minor bug of SeaHOH-gh-wc-polyfill-1.2.14, which is selectively manifested in this very issue page, i.e.

Sorry, I have no idea, I can't reproduce it with my browser.

@JustOff
Copy link
Owner

JustOff commented Feb 27, 2022

when his RL permits...

Unfortunately, it's highly unlikely this will ever happen :-(

@Vangelis66
Copy link
Author

Vangelis66 commented Feb 27, 2022

I can only speak for myself 😉 , since this extension has a very varied user base, but what is of paramount importance now is the safety/well-being of you, your family, your close-ones, etc,, under war conditions...

I'm afraid this isn't the right place to discuss international politics (and I believe GH doesn't allow this, in any case), so I won't, but you've done so much over the course of time 👍 with this extension, in essence providing crutches to browser platforms that can't be put up to the challenges imposed by GitHub by the browser maintainers themselves 😞 ...

Hopefully, when all dust has settled, you'll be able to revisit/address the 'disabled attribute of the "Load diff" button'; keep safe, many thanks, again, for all your work thus far! 🥇 ❤️

@Vangelis66
Copy link
Author

Vangelis66 commented Feb 27, 2022

This injected script is working.

where exactly inside 1.2.14b2's bootstrap.js file (i.e. between which lines) should this be inserted?

With no clue myself, I ended up transforming that into a GM/VM userscript,

// ==UserScript==
// @name        Fix disabled "Load diff" button
// @namespace   https://github.com/SeaHOH
// @description Restores disabled "Load diff" area, after it has been clicked once and page reloaded (see "https://github.com/JustOff/github-wc-polyfill/issues/46#issuecomment-1046129969", item [4.] ...).
// @author      SeaHOH
// @include     /^https:\/\/github\.com\/.*\/.*\/commit\/.*$/
// @include     /^https:\/\/github\.com\/.*\/.*\/pull\/.*$/
// @run-at      document-start
// @grant       none
// @version     0.1
// @icon        https://github.githubassets.com/pinned-octocat.svg
// ==/UserScript==

(function() {
  // Remove "disabled" attribute of "Load diff" buttons
  document.addEventListener('DOMContentLoaded', function() {
    for (let button of document.getElementsByClassName('load-diff-button'))
      button.removeAttribute("disabled");
  }, {
    once: true
  });
}());

... which I'm using together with "github-wc-polyfill-v1.2.14b2" in latest Serpent 52.9.0;
works OK; thanks @SeaHOH 👍

@rofl0r
Copy link

rofl0r commented Feb 27, 2022

someone with some skills can take @JustOff's last release (1.2.14b2), unzip it and apply the above patch to it and make it available for folks here please ?

@THEtomaso
Copy link

THEtomaso commented Feb 27, 2022

Stay safe, JustOff.

@AroKol78
Copy link

(and I believe GH doesn't allow this, in any case)

https://github.com/github/feedback/discussions/12042

Stay safe, JustOff.

and everyone 🇺🇦

@Vangelis66
Copy link
Author

Vangelis66 commented Feb 28, 2022

(and I believe GH doesn't allow this, in any case)

community/community#12042

FWIW,
https://docs.github.com/en/github/site-policy/github-community-guidelines#what-is-not-allowed
😕

Later edit: As expected, that heated discussion has been now closed/locked by GH staff... 😉
community/community#12042 (comment)
https://github.com/github/feedback/blob/main/CODE_OF_CONDUCT.md

@AroKol78
Copy link

AroKol78 commented Mar 3, 2022

SeaHOH added a commit that referenced this issue Mar 5, 2022
and disabled "Load diff" buttons #46

and tweak gitlab JS replace
SeaHOH added a commit that referenced this issue Mar 5, 2022
and disabled "Load diff" buttons #46

and non-response "Copy the full SHA" buttons #49

and tweak gitlab JS replace
@SeaHOH SeaHOH added fixed and removed help wanted Extra attention is needed labels Mar 6, 2022
@SeaHOH SeaHOH closed this as completed Mar 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working fixed
Projects
None yet
Development

No branches or pull requests

8 participants