next13-progressbar icon indicating copy to clipboard operation
next13-progressbar copied to clipboard

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

Open johannbuscail opened this issue 2 years ago • 20 comments

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

johannbuscail avatar Feb 21 '24 21:02 johannbuscail

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

ndungtse avatar Feb 22 '24 06:02 ndungtse

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

johannbuscail avatar Feb 22 '24 13:02 johannbuscail

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 avatar Feb 22 '24 18:02 ndungtse

checkout latest release it tackles all target attribute values

ndungtse avatar Feb 22 '24 18:02 ndungtse

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.

johannbuscail avatar Feb 22 '24 20:02 johannbuscail

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.

ndungtse avatar Feb 23 '24 06:02 ndungtse

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

johannbuscail avatar Feb 23 '24 09:02 johannbuscail

@ndungtse ?

johannbuscail avatar Feb 28 '24 17:02 johannbuscail

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

ndungtse avatar Feb 28 '24 18:02 ndungtse

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 avatar Mar 08 '24 19:03 johannbuscail

@ndungtse Any update ?

johannbuscail avatar Mar 16 '24 17:03 johannbuscail

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

ndungtse avatar Mar 16 '24 18:03 ndungtse

Thanks !

johannbuscail avatar Mar 16 '24 20:03 johannbuscail

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

MenroMi avatar May 10 '24 13:05 MenroMi

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…

ndungtse avatar May 11 '24 10:05 ndungtse

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.

MenroMi avatar May 12 '24 17:05 MenroMi

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

ndungtse avatar May 12 '24 18:05 ndungtse

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;

johannbuscail avatar Aug 14 '24 16:08 johannbuscail

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 avatar Aug 14 '24 21:08 ndungtse

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

ndungtse avatar Aug 18 '24 08:08 ndungtse