iv-org / invidious

Invidious is an alternative front-end to YouTube
https://invidious.io
GNU Affero General Public License v3.0
16.03k stars 1.76k forks source link

Issues with javascript:void(0) links #769

Open TheOneric opened 4 years ago

TheOneric commented 4 years ago

The links to javascript:void(0); make the interface uncomfortable for keyyboard-centered browsers like vimb. With vimb when going into the "F"-mode and selecting the target, nothing will happen when the href-target is javascript:void(0);. This happens e.g. with the Darkmode-toggle (has no onclick) and the "View Replies"-text (has onclick) in youtube-comments.

For Elements with an onclick-Attribute this can be fixed by just removing the href-attribute. For Elements without an onclick attribute, where the click event was defined in an seperate javscript, this does not work. Imho the cleanest approach would be to use a button element instead of a javascript:void(0); "link". Buttons should always be recognised as a clickable Element. CSS-rules for these buttons could be similar to this:

.astext {
  background:none;
  border:none;
  margin:0;
  padding:0;
  cursor: pointer;
}

.astext:hover {
  color: cyan;
}
TheOneric commented 4 years ago

I was trying to see if I could make the changes myself, but I did run into two problems: There are several a-elemnts with "javascript:void(0)"-href-attributes, where I am not sure, why they have this attribute in the first place. And secondly the docker-compose builds are currently failing, even with previous commits, so it seems to be caused by some dependency. I've also run into problems setting up postgresql on my machine, when not using docker, so I could not actually test my changes so far.

https://github.com/TheOneric/invidious/commit/3aa3be6b0afa5a47480bf8a55155168de75d8be1

stale[bot] commented 3 years ago

This issue has been automatically marked as stale and will be closed in 30 days because it has not had recent activity and is much likely outdated. If you think this issue is still relevant and applicable, please let us know.

TheOneric commented 3 years ago

If you think this issue is still relevant and applicable, please let us know.

Checking the invidious.fdn.fr instance this seems to still be relevant, though not really a high priority issue.

github-actions[bot] commented 2 years ago

This issue has been automatically marked as stale and will be closed in 30 days because it has not had recent activity and is much likely outdated. If you think this issue is still relevant and applicable, you just have to post a comment and it will be unmarked.

TheOneric commented 2 years ago

If you think this issue is still relevant and applicable, you just have to post a comment

I again checked invidious.fdn.fr and the issue still persists, but as before probably not a high priority issue. Though, at least at the surface this seems not too hard to fix but my attempt to produce a patch failed due to lack of familiarity with Crystal.

SamantazFox commented 2 years ago

the fix for that is to put the real URL (like /watch?v=<id>&t=123 for timestamps, or #void for stuff that can only be handled using JS) along with a bunch of data-* attributes, and add JS event handlers based on those.

TheOneric commented 2 years ago

Sorry for the late reply. I'm assuming with “a bunch of data-* attributes” you're referring to GET parameters. While your suggested solution would work, I believe this would be missing the core issue and for e.g. /watch?v=<id>&t=123 this would add a avoidable page reload.

The core issue is, that the primary purpose of <a> links is not “trigger an arbitrary action”, but navigating to a separate page. So when a browser like vim is told to interact with such an element by means other than a mouse, it will (attempt to) navigate to the linked page, and not trigger a click event.

But invidious currently is (ab)using some <a> links as arbitrary action triggers. In my humble opinion, using <button>s for such action triggers is the more idiomatic solution as this is a buttons primary purpose and also avoids the need for superfluous page realoads. At least for the darkmode toggle it has already been verified to work and the original post contains an exemplatory CSS-snippet to allow those buttons to keep their current plaint-text-like appearance. Furthermore, this would also be more straightforward and require less additional bits than adding more handlers and e.g. for comments also ensure the interacted with comment thread is loaded (the expanded thread may be from a different source (YT/reddit) or have been loaded afterwards via Show more).

WaywardHeart commented 2 years ago

Note that, due to Invidious's CSP not allowing inline scripts, javascript:void(0) and attributes like onclick don't even do anything. Telling the browser to not follow links should be done by running event.preventDefault() or return false; in proper event listeners.

TheOneric commented 2 years ago

Telling the browser to not follow links should be done by [...]

The point is, those elements shouldn't be links at all because well, they don't link to anything; they trigger an action. (And unlike when the issue was created, they no longer use the onclick element but a dynamically set up event listener)

SamantazFox commented 2 years ago

I'm assuming with “a bunch of data-* attributes” you're referring to GET parameters.

data-* attributes are not the same as GET parameters. They're HTML element attributes (like src=, href=, aria, etc...) that are used to add metadata inside the HTML that JS scripts can use.

While your suggested solution would work, I believe this would be missing the core issue and for e.g. /watch?v=<id>&t=123 this would add a avoidable page reload.

Invidious is designed to work without JS, so links must have a valid href attribute. In the case where JS is available and enabled, some initialization script would parse the data-* attributes, add event listeners, and remove the URL of all <a> elements. The link would go nowhere and everything would be done in JS.

The core issue is, that the primary purpose of <a> links is not “trigger an arbitrary action”, but navigating to a separate page. So when a browser like vim is told to interact with such an element by means other than a mouse, it will (attempt to) navigate to the linked page, and not trigger a click event.

But invidious currently is (ab)using some <a> links as arbitrary action triggers. In my humble opinion, using <button>s for such action triggers is the more idiomatic solution as this is a buttons primary purpose and also avoids the need for superfluous page realoads. At least for the darkmode toggle it has already been verified to work and the original post contains an exemplatory CSS-snippet to allow those buttons to keep their current plaint-text-like appearance. Furthermore, this would also be more straightforward and require less additional bits than adding more handlers and e.g. for comments also ensure the interacted with comment thread is loaded (the expanded thread may be from a different source (YT/reddit) or have been loaded afterwards via Show more).

As I said above, invidious is designed to work without JS. So the only way to change the player state is to load a different page/URL (requesting the same ressource, with added URL parameter(s)), and hope that the bowser handles the attribute change on the <video> element.

For comments, using a button is probably a better solution, yeah. I'll have to check the accessibility of such a solution.

TheOneric commented 2 years ago

Invidious is designed to work without JS

Right, then using a real link where possible like e.g. the video time is in general better for this goal.


This may be redundant and you already accounted for this, but just in case:

and remove the URL of all elements. The link would go nowhere and everything would be done in JS.

Removing the href= from an <a> element results in it no longer being interactable via vimb’s interaction mode. Adding a href to a button without a click action on the other hand results in the link being followed in vimb though this then would be the reverse of the current situation (button-like element stuffed into a pseudo hyperlink) and I suspect other software would struggle with it like vimb does now. Ideally, the type of elements using a href= for their primary effect should be <a> and those who use click event listeners should use <button>.

So if a startup script just removes the href= attribute but leaves the type as <a> for say the darkmode toggle, then it still wouldn't be possible to interact with it in vimb naturally. (It would however no longer show up in the interaction menu avoiding the false impression that it should work. Actually working would be preferable though). vimb appears to only execute the click action on <a> links for literal clicks, not via its primary means of interaction, the keyboard-driven interaction mode. Imho this makes sense and aligns with <a> denoting hyperlinks. Would it be possible for the initialisation script to also change the type to <button> when the href= attrbute is replaced by an event listener without too much effort?

SamantazFox commented 2 years ago

Removing the href= from an <a> element results in it no longer being interactable via vimb’s interaction mode. Adding a href to a button without a click action on the other hand results in the link being followed in vimb though this then would be the reverse of the current situation (button-like element stuffed into a pseudo hyperlink) and I suspect other software would struggle with it like vimb does now.

I wouldn't remove the href attribute, though. Only the URL. And replace it with #. As for click action, that would be registered as an event handler. Does it specifically need to have an onclick HTML attribute?

Would it be possible for the initialisation script to also change the type to <button> when the href= attrbute is replaced by an event listener without too much effort?

Idk. Buttons already have a specific style (like in the preferences, e.g), so I guess that re-implementing links as buttons will need quite some styling/integration work (links are "transparent" in terms of CSS. They generally don't have their own style and simply "fit" around the child element, and many CSS properties don't work on links).

TheOneric commented 2 years ago

As for click action, that would be registered as an event handler. Does it specifically need to have an onclick HTML attribute?

Whether it's an onclick attribute or an event handler does not matter. It just happened to be an onclick attribute when I originally opened this issue, but now no longer is slightly complicating my testing of the buton solution (editing the type via the dev tools dropped dynamically added event listeners).

I wouldn't remove the href attribute, though. Only the URL. And replace it with #.

Surprisingly for me, this works in vimb. I do not know if it will work as inteded in eg screenreaders. I still believe elements which primarily toggle an action should ideally be a semantic <button> or <input type=button"> element as this is more idiomatic. But if that’s too complicated considering they'll actually be links in the nojs mode, adding the role="button" attribute might perhaps be helpful in js mode. (For vimb, with role="button" the href attribute can also be completly removed while the element is still interactable and working) In any event, thanks for taking time to discuss and consider this :)

github-actions[bot] commented 3 months ago

This issue has been automatically marked as stale and will be closed in 30 days because it has not had recent activity and is much likely outdated. If you think this issue is still relevant and applicable, you just have to post a comment and it will be unmarked.

TheOneric commented 3 months ago

still javascript:void(0) with 2024.04.27-eda7444c