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 button not being inject #33

Closed
wants to merge 2 commits into from

Conversation

1natsu172
Copy link
Collaborator

Issue: #31
Button was not injected in the video with creator-heart-action in the comment.

Compared to other video pages this video type is slow to load comments.
Moreover, when navigating from this video type to another video, it turned out that the rewriting of the parse target node was delayed.

newYouTube.injectCommentsCount(e)
}, 500)
});

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Case of issue: heart-action typed video watched then next another video.
The next video's parse target comments node is rewritten late.

} else {
clearInterval(tryRewrite);
}
}, 100);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fetch of the number of comments may end before the button's injection. So wait for the button.

@dideler
Copy link
Owner

dideler commented Feb 5, 2018

Checked out the branch and ran it locally but I'm still seeing the issue on this video of the button not being injecting. I can try having a better look tomorrow night.

@1natsu172
Copy link
Collaborator Author

1natsu172 commented Feb 6, 2018

I thought "really?" And watched it on the UK version of YouTube but it certainly doesn't work... 😨
It's working fine on YouTube in Japanese version…
I will investigate a little more.

No, this was my mistake, I was in the master branch locally (Master doesn't contain this fix).
I noticed it and tried with this(#31) branch, so it still works on the UK version of YouTube ...

@dideler
Copy link
Owner

dideler commented Feb 7, 2018

Here are the steps I'm using to reproduce the bug:

  1. Check out branch fix#31/support-heart-action-video with commit 11e7a33
  2. Reload local extension in Chrome
  3. Hard navigate to https://www.youtube.com/watch?v=7a9lsGtVziM

If dynamic navigation is used for the last step, for example clicking on the video from another video, then the button will be injected as expected. Otherwise with static navigation, the button isn't being injected.

I'll be able to help with the debugging later this week.

@1natsu172
Copy link
Collaborator Author

Well...apparently it seems that whether or not we are logged into YouTube affects the behavior. I use a separate Chrome account for development, and I haven't logged into YouTube over there. As I tried with YouTube logged in then static navigation to video, so didn't work as you said.

The specific cause is yet to be understood. Please help me if I can't solve it by the weekend…! 🙏

@dideler
Copy link
Owner

dideler commented Feb 7, 2018

Well...apparently it seems that whether or not we are logged into YouTube affects the behavior.

😱

That was a tricky one to find, good spot.

I think it's time to introduce some automated tests (#9). There are a lot of cases we have to cover now and doing it manually is becoming too time consuming. I'll try starting that this weekend. 🤞

(
e.target.tagName === 'YTD-ITEM-SECTION-RENDERER' || // this mean: rendered childNode of comments'DOM(<ytd-comments>)
e.target.tagName === 'YTD-CREATOR-HEART-RENDERER' // this case: video type with heart action to user comments by creator.
)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fix problem the user logged in YouTube and watching video with heart-action-comment page.

@1natsu172
Copy link
Collaborator Author

1natsu172 commented Feb 11, 2018

@dideler, I fixed an issue where the logged in user and the un-logged in user behaved differently.

My local is working well in either state, on the issue video page! Please check this again. 🤞

@1natsu172 1natsu172 force-pushed the fix#31/support-heart-action-video branch from c0e45d6 to ade568a Compare February 11, 2018 04:04
Button was not injected in the video with creator-heart-action in the comment. #31
…rt-action-comment

Fixed that button is not rendered under this condition. #31
@1natsu172 1natsu172 force-pushed the fix#31/support-heart-action-video branch from ade568a to 8384575 Compare February 11, 2018 04:28
@dideler
Copy link
Owner

dideler commented Feb 12, 2018

Hey @1natsu172, sorry for the slow reply and thanks for your patience!

I tried the fix with the latest changes and it worked on video 7a9lsGtVziM 🙌
But I was wondering if there was a simpler way without having to introduce timers.

I listened to every YT event I could find

image

const events = [
  'yt-action',
  'yt-add-element-to-app',
  'yt-autonav-pause-blur',
  'yt-autonav-pause-focus',
  'yt-autonav-pause-guide-closed',
  'yt-autonav-pause-guide-open',
  'yt-autonav-pause-player',
  'yt-autonav-pause-player-ended',
  'yt-autonav-pause-player-scroll',
  'yt-focus-searchbox',
  'yt-guide-close',
  'yt-guide-toggle',
  'yt-history-load',
  'yt-history-pop',
  'yt-masthead-height-changed',
  'yt-navigate',
  'yt-navigate-finish',
  'yt-navigate-set-page-offset',
  'yt-navigate-start',
  'yt-page-type-changed',
  'yt-page-data-updated',
  'yt-preconnect-urls',
  'yt-register-action',
  'yt-report-form-closed',
  'yt-report-form-opened',
  'yt-request-panel-mode-change',
  'yt-retrieve-location',
  'yt-service-request-completed',
  'yt-set-theater-mode-enabled',
  'yt-swatch-changed',
  'yt-theater-mode-allowed',
  'yt-unregister-action',
  'yt-update-title',
  'yt-update-unseen-notification-count',
  'yt-visibility-refresh',
];

events.forEach(eventName => {
  console.log(`listening for ${eventName}`);
  document.addEventListener(eventName, e => {
    console.log(e);
  });
});

And I noticed that the event yt-register-action with tag YTD-COMMENTS was firing right before comments would be loaded.

See the result in #35 and give it a try. It also benefits from not showing the loading animation by the comments which would appear with the timer strategy, which could confuse users as it looks like comments are about to show and then they are hidden.

@1natsu172
Copy link
Collaborator Author

Thanks for checking.
I don't think the timer strategy is a good solution. yt-visibility-refresh had no choice.
But I think your solution(#35) is perfect. Let my PR abolish and adopt your #35.


As an aside, I had seen a lot of "yt-*" events when I implemented the number of comments, but it seems I didn't have yt-register-action with tag YTD-COMMENTS before, or did I miss it? 🤔

@dideler
Copy link
Owner

dideler commented Feb 13, 2018

but it seems I didn't have yt-register-action with tag YTD-COMMENTS before, or did I miss it?

Interestingly, the event yt-register-action was poorly being used by the extension to support New YT (ebb3290) but was later removed in favour of yt-visibility-refresh (0dc8bfb).

I'm not sure if yt-register-action always had YTD-COMMENTS as a tag, it could be something new.

@dideler dideler closed this Feb 13, 2018
@dideler dideler deleted the fix#31/support-heart-action-video branch February 13, 2018 20:53
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.

2 participants