ndungtse / next13-progressbar

A simple Next.js progressbar component using NProgress with support of next.js >=13 app dir.
https://next13-progressbar.vercel.app
MIT License
91 stars 3 forks source link

Progress Bar Persists on External Links with Specified `target` Attribute #25

Open johannbuscail opened 7 months ago

johannbuscail commented 7 months ago

Description

Hello, and first off, thank you for the amazing work on this project! I've encountered a consistent bug related to the behavior of the progress bar when interacting with external links.

Steps to Reproduce

  1. Navigate to a page that contains external links.
  2. Click on any external link where the <a> tag has a specified target attribute. This issue arises not only with target="_blank" but also with custom frame names.

Expected Behavior

The progress bar should complete or disappear after the external link has loaded, indicating that the navigation process has finished.

Actual Behavior

The progress bar appears and remains visible indefinitely, suggesting that the navigation or loading process is still ongoing. This occurs regardless of the target attribute specified in the <a> tag, affecting both standard _blank targets and custom frame names.

Links

https://www.w3schools.com/tags/att_a_target.asp

ndungtse commented 7 months ago

From my local testing links with target _blank doesn't show progress bar because they are ignored. May this can happen in frames because I didn't handle frames implementations. If you have a live example where a with _blank target attribute showing progressbar or code example you can show

johannbuscail commented 7 months ago

Yes my bad, it doesnt show on target=_blank. We use a lot frames on our app. Do you think this can be implemented easily ?

Maybe here, instead of checking if target equals _blank, check if it is not _self, _parent or _top

ndungtse commented 7 months ago

Yeah. I think I should those too. But because I haven’t used frames before I didn't explore other options but yes It can be implemented easily. So I'll do the same as it is on _blank target.

ndungtse commented 7 months ago

checkout latest release it tackles all target attribute values

johannbuscail commented 7 months ago

Thanks for your quick reply !

This doesn't solve the framename issue. Framenames are basically like _blank but with a specific id.

It works in the following:

  1. It opens a new tab the first time the user clicks on the link.
  2. Then, it just switches to the tab if it's already opened.

Any string can be in the target attribute.

ex: <a target="editor">.

So the commit only partially solves the issue.

ndungtse commented 7 months ago

I think you can open another issue about frames compatibility as I think it is a potential feature to be worked on it carefully. And thanks for your continuing contribution.

johannbuscail commented 7 months ago

I don't think it must be another issue, as it is directly related to this one.

johannbuscail commented 7 months ago

@ndungtse ?

ndungtse commented 7 months ago

This feature will be worked on and I'm gonna reopen the issue

johannbuscail commented 7 months ago

I don't think this is quite complicated as it would only require to invert the if statement to return if anchor element is not on same page. Instead of :

https://github.com/ndungtse/next13-progressbar/blob/d7d56831bffc1f46564d2ee649c8a8d7ba321725/src/AppProgressBar.tsx#L116

It would only require:

if (anchorElement.target !== '_self' && anchorElement.target?.trim() !== "")
   return;

I'm not sure if the anchorElement.target?.trim() !== "" is needed. I don't know if not passing a target through html/jsx props automatically gives _self or if you need to handle it.

And for this: https://github.com/ndungtse/next13-progressbar/blob/d7d56831bffc1f46564d2ee649c8a8d7ba321725/src/AppProgressBar.tsx#L142-L145

Replace it to this:

const validAnchorELes = Array.from(anchorElements).filter((anchor) => { 
   if (anchor.href.startsWith('tel:+') || anchor.href.startsWith('mailto:')) return false; 
   if (anchor.target !== '_self' && anchor.target?.trim() !== "") // again not sure about the `anchor.target?.trim() !== ""`
       return false;
   return anchor.href; 
 }); 
johannbuscail commented 6 months ago

@ndungtse Any update ?

ndungtse commented 6 months ago

Well, Sorry for oversight I was somehow busy but I tested it and will be available sonn

johannbuscail commented 6 months ago

Thanks !

MenroMi commented 5 months ago

Hello, I encountered the same problem when I wanted to go to another page using the next link, but in my case the links do not have any specific properties that can affect the progress bar. I noticed that my problem only occurs when I try to go from the page of the new version of the project (was completed in next js 14 where i have app folder) to the page of the old version of the project (with the pages folder - next js 12). It seems that the progress bar sees the old project as an external page.

https://github.com/ndungtse/next13-progressbar/assets/97913944/2bbc28f3-95dc-442d-887c-09a2f5567a29

ndungtse commented 4 months ago

Hello, I encountered the same problem when I wanted to go to another page using the next link, but in my case the links do not have any specific properties that can affect the progress bar. I noticed that my problem only occurs when I try to go from the page of the new version of the project (was completed in next js 14 where i have app folder) to the page of the old version of the project (with the pages folder - next js 12). It seems that the progress bar sees the old project as an external page. ad Screen.Recording.2023-12-11.at.10.37.09.mp4

Well, I tried to go to the same site you developing and the issue didn't appear again. but I also noticed the full reload as you used default html a tag. did you manage to handle it after above comment? cause for me the issue didn't show up

Uploading Quality Engineering Services By Solvd To Meet High Standards - Personal - Microsoft​ Edge 2024-05-11 12-13-15.mp4…

MenroMi commented 4 months ago

Hello, I encountered the same problem when I wanted to go to another page using the next link, but in my case the links do not have any specific properties that can affect the progress bar. I noticed that my problem only occurs when I try to go from the page of the new version of the project (was completed in next js 14 where i have app folder) to the page of the old version of the project (with the pages folder - next js 12). It seems that the progress bar sees the old project as an external page. ad Screen.Recording.2023-12-11.at.10.37.09.mp4

Well, I tried to go to the same site you developing and the issue didn't appear again. but I also noticed the full reload as you used default html a tag. did you manage to handle it after above comment? cause for me the issue didn't show up

record.mp4

Hello again. This problem doesn't appear when you use chrome, but if you check it in firefox / safari or opera, you can see it. At the moment I only have one solution, which is to add the target property to the link tag, but in general this does not solve the problem because the preloader disappears. P.S I don't use the default html a tag, instead I use the next extended link component.

ndungtse commented 4 months ago

Thanks @MenroMi . I'm going to test it on those browsers and ckeck why it behaves like that

johannbuscail commented 1 month ago

Hello @ndungtse, it still doesn't work for me on custom targets. You only added the last part of my message but you haven't added this part on line 116:

I don't think this is quite complicated as it would only require to invert the if statement to return if anchor element is not on same page. Instead of :

https://github.com/ndungtse/next13-progressbar/blob/d7d56831bffc1f46564d2ee649c8a8d7ba321725/src/AppProgressBar.tsx#L116

It would only require:

if (anchorElement.target !== '_self' && anchorElement.target?.trim() !== "")
   return;
ndungtse commented 1 month ago

Hello @ndungtse, it still doesn't work for me on custom targets. You only added the last part of my message but you haven't added this part on line 116:

I don't think this is quite complicated as it would only require to invert the if statement to return if anchor element is not on same page. Instead of : https://github.com/ndungtse/next13-progressbar/blob/d7d56831bffc1f46564d2ee649c8a8d7ba321725/src/AppProgressBar.tsx#L116

It would only require:

if (anchorElement.target !== '_self' && anchorElement.target?.trim() !== "")
   return;

Ooh. I get it right now @johannbuscail . First I thought you need all target values to be evaluated, so as you suggested only target without '_self' and target with some value will not trigger the progressbar. I think that would be easier then 👍🏻. Gonna update it in next release

ndungtse commented 1 month ago

@johannbuscail check out new release I think issue should be now resolved