martok / palefill

Inject Polyfills for various web technologies into pages requiring them
https://martok.github.io/palefill/
Mozilla Public License 2.0
79 stars 9 forks source link

Reaction tooltips missing in GitHub #22

Closed Vangelis66 closed 2 years ago

Vangelis66 commented 2 years ago

Many thanks for this most valuable extension 👍 . I'm currently in the process of migrating from gh-wc-polyfill to palefill, but stumbled across a bug/irritation:

STR Version of extension: 1.11

  1. Load, e.g., https://github.com/martok/palefill/issues/8#issuecomment-1146410975
  2. Hover over the reaction emoji, to see the identity of the GH-member that reacted
  3. Expected behaviour: A tooltip pops-up with the GH-username (among other things):

rt1

This is currently working as expected with gh-wc-polyfill-v1.2.19 😉

  1. Actual behaviour: No tooltip pops up when the reaction emoji is hovered over... 😞

I can also reproduce with latest palefill ALL the problems reported in upstream issue #54: https://github.com/JustOff/github-wc-polyfill/issues/54

problems fixed by https://github.com/JustOff/github-wc-polyfill/commit/c0f5ff34009e1288a8138b2be2d235fec13f6206 and https://github.com/JustOff/github-wc-polyfill/commit/74179c247f23cb02a6e7dc5780de09599855c485

Thanks in advance for any eventual resolution... 😄

Vangelis66 commented 2 years ago

"Vielen Dank" for your prompt action! 👍 I compiled from the master branch, source snapshot martok-palefill-v1.11-3-g58a1a29, and it's working splendidly:

pf2

martok commented 2 years ago

That's actually Github screwing it up. The elements get the dropdown-menu-w class (which usually means "west"), and that is excactly where they show up. They should set dropdown-menu-e (for "east").

Vangelis66 commented 2 years ago

That's actually Github screwing it up.

Yes :angry: ... I can easily reproduce with a recent-ish Chromium-based browser, too :disappointed: ... In the meantime, could Microsoft's blunder be mitigated somehow via a userstyle or userscript? Currently, the only way to react (but NOT on Releases) is via the "emoji" button in the comment's header (which also spawns a pop-up to its (downwards) left) ...

GH

Vangelis66 commented 2 years ago

How typical of Microsoft... 😠 😡

The misplaced "reaction emoji selection panel" reported by @NEO-SPECTRUMAN appears to have been now fixed:

  1. reacting on a comment: REACTION1
  2. reacting on a release: REACTION

... however, this original issue has now come back with a vengeance :rage:, i.e.: hovering over already cast reaction emojis (under comments/releases) doesn't spawn a pop-up anymore with the usernames of the GH members that reacted, much like in my OP here... This time GH did something else, because the old upstream extension, gh-wc-polyfill-v1.2.19, no longer works in that regard...

This issue should be re-opened and revisited... FWIW, Serpent v52.9.0 (2022-07-30) (32-bit) and latest stable palefill (v1.18); @martok, please, kindly to the rescue! :heart:

Later EDIT: I also forgot to note that ALL GH tooltips are now seemingly affected (i.e. have vanished), including GH Editor's toolbar buttons (like when typing/editing this very comment):

missingtooltips

SeaHOH commented 2 years ago

Patches below fixed the new issue.

https://github.com/martok/palefill/blob/2d141c10522ab465df0906be740d32777d7704f3/lib/polyfills.js#L333-L350

                  return !context ? !selectors ? tagName :
                      \`\${tagName}:-moz-any(\${selectors})\` :
                      \`:-moz-any(\${selectors}) \${tagName}\`;
                }) + style;
          }
        }
      }
    };
    const observer = new MutationObserver(function(mutations) {
            for (const {target, attributeName, oldValue} of mutations) {
              target.attributeChangedCallback(attributeName, oldValue, target.getAttribute(attributeName));
            }
          }),
          asNames = new Set(["article", "aside", "blockquote", "body", "div",
                             "footer", "h1", "h2", "h3", "h4", "h5", "h6",
                             "header", "main", "nav", "p", "section", "span"]);
    Element.prototype.attachShadow = function attachShadow(init) {
      // customElements observe Shadow failed, so re-start observe here
      const {observedAttributes} = this.__CE_definition || {};
      if (observedAttributes) {
        observer.observe(this, {
          attributes: true,
          attributeOldValue: true,
          attributeFilter: observedAttributes
        });
      }
      if (this.shadowRoot !== undefined)
martok commented 2 years ago

Makes sense, that fits with how far I managed to narrow the problem down.

Thank you, will apply later!

Vangelis66 commented 2 years ago

@SeaHOH

Between 202208072241Z and 202208090651Z, you have published, in total, nine (9) different iterations of #22 patches, so many thanks, indeed 👍 ...

The date/time while writing this comment is ca. 202208091700Z; during the last hour or so, I have been applying each and every one of these 9 different "patches" of yours, and then compiling them locally into XPI files, then testing each resultant XPI extension file...

Do you know what the current outcome is? Each and every one of these 9 different patches, if applied on top of source snapshot martok-palefill-v1.18-2-git-20220803-g2d141c1, produces, in the end, an .XPI file which, when installed in the browser [St52 v52.9.0 (2022-08-05) (32-bit)], BREAKS GitHub in my UXP-based browser 😞 ...

I have been very careful at properly applying the patches onto file polyfills.js of snapshot 2d141c1, so, hopefully, it isn't something caused by me messing up things (but I can never rule it out 😜 ...).

Latest palefill release (v1.18) as well as latest master HEAD (palefill-v1.18-2-git-20220803-g2d141c1) when compiled to an XPI DO NOT break GitHub, but, alas, don't have a fix for this issue...

TL;DR: As of this time, proposed "fixes" do not remedy this issue... 😭

SeaHOH commented 2 years ago

One line has been missed, it's OK now.

Vangelis66 commented 2 years ago

... Thanks for your prompt reply 😄 ! I have applied the most up-to-date version of your patch,

--- a.polyfills.js      2022-08-04 00:32:21.000000000 +0300
+++ b.polyfills.js      2022-08-09 21:14:20.477000000 +0300
@@ -330,23 +330,32 @@
                 // flag "s" is broken in SeaMonkey
                 /:host(-context)?(?:\\(([\\s\\S]+?)\\))?/g,
                 function ($, context, selectors) {
-                  if (context === undefined && selectors === undefined)
-                    return tagName;
-                  if (context)
-                    return \`:-moz-any(\${selectors}) \${tagName}\`;
-                  let res = [];
-                  for (let selector of selectors.split(","))
-                    res.push(tagName + selector);
-                  return \`:-moz-any(\${res.join(", ")})\`;
+                  return !context ? !selectors ? tagName :
+                      \`\${tagName}:-moz-any(\${selectors})\` :
+                      \`:-moz-any(\${selectors}) \${tagName}\`;
                 }) + style;
           }
         }
       }
     };
-    const asNames = new Set(["article", "aside", "blockquote", "body", "div",
+    const observer = new MutationObserver(function(mutations) {
+            for (const {target, attributeName, oldValue} of mutations) {
+              target.attributeChangedCallback(attributeName, oldValue, target.getAttribute(attributeName));
+            }
+          }),
+          asNames = new Set(["article", "aside", "blockquote", "body", "div",
                              "footer", "h1", "h2", "h3", "h4", "h5", "h6",
                              "header", "main", "nav", "p", "section", "span"]);
     Element.prototype.attachShadow = function attachShadow(init) {
+      // customElements observe Shadow failed, so re-start observe here
+      const {observedAttributes} = this.__CE_definition || {};
+      if (observedAttributes) {
+        observer.observe(this, {
+          attributes: true,
+          attributeOldValue: true,
+          attributeFilter: observedAttributes
+        });
+      }
       if (this.shadowRoot !== undefined)
         throw new DOMException(
             \`The <\${this.tagName}> element has be tried to attach to is already a shadow host.\`,

compiled locally source to .XPI, installed and can verify now things are OK:

proof

martok commented 2 years ago

Thank you two! I've moved the observer attachment down below the input validation, otherwise applied unchanged.

Vangelis66 commented 2 years ago

reaction popup panel appears on left corner of button and goes out of screen

That's actually Github screwing it up.

The misplaced "reaction emoji selection panel" reported by @NEO-SPECTRUMAN appears to have been now fixed:

😡 ; just a short while ago, I bumped into one more such instance which hasn't been fixed; it's "Conversations" as part of PRs...

STR

  1. Navigate to https://github.com/ytdl-org/youtube-dl/pull/31170 (the "Conversations" tab should have been selected by default)
  2. Try to react to the opening post by clicking on the "emoji" button at the bottom of that post/comment; result:

GH1

Really? How incompetent are they at Microsoft currently? 👎 😞 Anyhow, only way to react is by using the "emoji" button in the post's header...

dirkf commented 2 years ago

UserCSS:

.dropdown-menu-reactions.dropdown-menu-w {
    float: left;
    left: 0px;
        top: -180%;
    width: -moz-fit-content; /* or 1000% */
        /* uncomment for specific bg colour
        background-color: black;
        */
}

For browsers that we don't care about: https://stackoverflow.com/questions/7839164/is-there-a-css-cross-browser-value-for-width-moz-fit-content.

Vangelis66 commented 2 years ago

Thanks for that userstyle 😄 ; I've tried it, and while it does now allow for (un-)reacting, it appears as an "ugly" kludge (which is still better than nothing 😉 ) in my case:

ghreact1

(among other things, the panel's background is transparent, etc.).

Additionally, I'd need someone to kindly offer the regex needed for the userstyle to be applied selectively on https://github.com/*/*/pull/* URIs, as other "emoji" buttons (under issue comments and releases) are not affected by MS's blunder 😠 ... As you might've guessed by now, neither CSS nor RegExp is my forte... 😞

dirkf commented 2 years ago

I suspect this depends on your theme in GH. Mine is "Light high contrast". Insert a line with background-color: black; in the {...} to get one.

See the updated CSS above for a more specific fix that doesn't affect the other reaction menus. When applied with Stylem the CSS properties need the !important attribute: eg float: left !important;.

Vangelis66 commented 2 years ago

See the updated CSS above for a more specific fix that doesn't affect the other reaction menus.

Your altruistic help is being highly valued here! ❤️ Your edited/updated code works brilliantly for me now:

dirkf-fix1

I just tweaked somewhat the top: parameter to a -225% value, so it more closely resembles the behaviour of the other "emoji-buttons" (makes the panel display slightly further above).

When applied with Stylem

In my case, my current browser (latest Serpent 52.9.0) has kept Fx-52esr-level support for Web Extensions, so I'm using Stylus-v1.4.23 as a userstyle manager - contrary to Stylem, Stylus has additional support for *.user.css userstyles (the type that can be further configured by the user post-install); I've been waiting/hoping for that support to arrive in Stylem, too, but I fear that ship has sailed (and sunk?).

I suspect this depends on your theme in GH. Mine is "Light high contrast"

I, OTOH, use Dark default, together with the excellent Dark GitHub userstyle (and yes, it's of the *.user.css format 😉 ).

Kindest regards 😄

Vangelis66 commented 2 years ago

... And... UPDATE (this is a bottomless pit 😡 ) !

The praise-worthy solution by dirkf apparently breaks another "emoji-button": the one appearing in comments inside actual commit code (there's a proper term for these comments, probably, but I hope you get my drift...). Visit:

https://github.com/ytdl-org/youtube-dl/commit/d231b56717c73ee597d2e077d11b69ed48a1b02d#r81169570

; if you click on the "emoji-button" to react to pukkandan's comment, the emoji selection popup is displayed to the right, but truncated 😞 :

dirkf-fix2

Fortunately, temporarily disabling dirkf's userstyle in Stylus will allow for that (marginal) reaction case to function; I have currently the userstyle enabled on domain github.com, but, as I said previously, some fine-tuning via RegExp to restrict it to PR Conversations is, apparently, still needed...

Later addition: It would appear the below RegExp filter for CSS does it for me:

https://github\.com/.*/.*/pull/.*

Got help from this and that 👍 ...

SeaHOH commented 2 years ago

just adjust the selector as .comment-reactions .dropdown-menu-reactions