zHaytam / SmartBreadcrumbs

A utility library for ASP.NET Core (both MVC and Razor Pages) websites to easily add and customize breadcrumbs.
https://blog.zhaytam.com/2018/06/24/asp-net-core-using-smartbreadcrumbs/
MIT License
161 stars 77 forks source link

Index breadcrumb always shows, even if user did not access that page #49

Closed bryancass closed 4 years ago

bryancass commented 4 years ago

My DefaultBreadcrumb is "Home". So let's say on my MVC home page, the user can navigate to two different links.

<a href="/Contact/Index">Index</a>
<a href="/Contact/Edit?contactId=##">Edit</a>

One goes to controller/action Contact/Index and one goes to Contact/Edit. I have a breadcrumb attribute for both in the ContactController. When the user navigates to Contact/Index, the breadcrumbs correctly looks like:

Home / Index

When the user navigates to Contact/Edit, the breadcrumbs incorrectly looks like:

Home / Index / Edit

The user should not be able to navigate back to Index on this breadcrumb, only to Home.
It should look like this:

Home / Edit

Is this a bug?

Thanks! Bryan

zHaytam commented 4 years ago

Hello, sorry I didn't see this issue. Can you show me your actions?

bryancass commented 4 years ago

Hi, no problem.  Here's the actions in CompanyController.cs:

        // GET: Company/Index         [Breadcrumb("Company List")]         public async Task Index(string message, int? pageNumber, int pageSize, string sortOrder, string currentFilter, string searchString)   { [...]            return View(model);       }

        // GET: Company/Edit         [Breadcrumb("Edit Company")]          public async Task Edit(int? id, string returnUrl = "/", string prevReturnUrl = "/") { [...]            return View(model);         }

Here is the link on the home page to the edit page: My Company

When you click that link, the breadcrumb becomes:

Home / Company List / Edit Company

... when it should be:

Home / Edit Company

bryancass commented 4 years ago

Also, if a controller has no Index page, SmartBreadcrumbs does not work. I get this error:

SmartBreadcrumbs.SmartBreadcrumbsException: 'xxxxController doesn't contain a valid action named Index.'

If I add a dummy Index action to the controller with a [Breadcrumb("Index")] attribute so that SmartBreadcrumbs doesn't crash

        [Breadcrumb("Index")]
        public IActionResult Index()
        {
            return View();
        }

...it then adds the dummy Index automatically to the breadcrumb as I mentioned above, but then the Index link causes an error because there is no Index.cshtml page.

Home / Index / Edit

zHaytam commented 4 years ago

Can you try to specify the FromController and FromAction to your Home page and see if it still shows the dummy index? I'm failing to understand where the problem is coming from without a project to try it on :/

bryancass commented 4 years ago

No problem. I created a new web app that illustrates the problem. Please unzip the attached app and run in VS 2019. I attached some screen shots to show how it inserts the invalid "Index" link into the breadcrumb. Please note that /Test/Details can be accessed from other pages in the app, so I cannot assume only one FromController and FromAction for that page.

WebApplication1.zip Untitled Untitled1

zHaytam commented 4 years ago

Can you try this with the new version (3.1) ?

bryancass commented 4 years ago

Hi Zanid. I am sorry, but it still does not work with v3.1. You should be able to test it by downloading and running the WebApplication1 I attached above. Untitled

timstokman commented 4 years ago

Same issue here. It seems when the SmartBreadcrumbs library went from 2.0 to 2.1 it started getting this issue. It also occurs in all of the later versions. For some reason, if no FromAction is defined, it seems to assume "Index" is the FromAction. It also crashes when there's no "Index" in the controller. This is impeding an upgrade from asp core 2 to 3. Going to debug this myself, see where it's coming from.

timstokman commented 4 years ago

The issue appears here: https://github.com/zHaytam/SmartBreadcrumbs/commit/330656834a8f98fd38b7305efd0f5b63b749718f

The issue is BreadcrumbOptions.DefaultAction . It's set to "Index", and it's readonly.. which means you can't change this behaviour easily. It used to be the case that if there's no FromAction, it'd be a "top-level" node, now everything gets redirected to "Index".

I'm using this work-around for now:

services.AddBreadcrumbs(
    GetType().Assembly,
    options =>
    {
        // .. setting other options
        // .. and then setting the DefaultAction property through the backing field & reflection
        FieldInfo defaultActionBackingField = options.GetType().GetRuntimeFields().Where(a => Regex.IsMatch(a.Name, $"\\A<{nameof(BreadcrumbOptions.DefaultAction)}>k__BackingField\\Z")).First();
        defaultActionBackingField.SetValue(options, null);
  });

Very ugly, sets the backing field of the read-only property directly. I'm not sure if it'll have any side-effects. I'm going to test this, might be a temporary work-around until this is fixed. Might work for you as well @bryancass.

bryancass commented 4 years ago

Thanks, Tim. I just wanted to confirm that the way this should work is that any consecutive page you link to gets added to the breadcrumb 'trail' as long as it has the [Breadcrumb...] annotation on the controller's action method, correct? And then when clicking on any of the breadcrumb page links, it should go back to that page as if you did a javascript:history.back() command, meaning the URL for that page was preserved, correct?

timstokman commented 4 years ago

@bryancass If I get what you're saying, yes. What changed since version 2.1 of this library is that it links to "Index" as a previous action in the breadcrumb trail (FromAction in [Breadcrumb..) if there's no previous action. My workaround (I hope) will restore the previous behaviour (end of the trail if there's no FromAction), until there's an actual fix.

bryancass commented 4 years ago

Your fix seems to work OK in my little WebApplication1 I attached. If it works OK in your testing, I can try it in my production web app.


using System.Reflection;
using System.Text.RegularExpressions;
using SmartBreadcrumbs;
using SmartBreadcrumbs.Extensions;
[...]
            services.AddBreadcrumbs(GetType().Assembly, options =>
            {
                options.TagName = "nav";
                options.TagClasses = "";
                options.OlClasses = "breadcrumb";
                options.LiClasses = "breadcrumb-item";
                options.ActiveLiClasses = "breadcrumb-item active";
                FieldInfo defaultActionBackingField = options.GetType().GetRuntimeFields().Where(a => Regex.IsMatch(a.Name, $"\\A<{nameof(BreadcrumbOptions.DefaultAction)}>k__BackingField\\Z")).First();
                defaultActionBackingField.SetValue(options, null);
                //options.SeparatorElement = "<li class=\"separator\">/</li>";  // This adds an extra separator between each level
            });
timstokman commented 4 years ago

@bryancass Might take a bit before I'm able to test everything, the upgrade I'm doing is part of a bigger change (upgrading a project with a lot of dependencies). Might be faster if you test yourself.

It seems to work for me (roughly), but I did notice a few other minor things that have changed from 2.0 to 3.1.

bryancass commented 4 years ago

I tried it on my production web app and it doesn't work correctly. All you get is the default breadcrumb and the page you are on. It's missing all the pages in between the Home page and how you got to the page you're on.

bryancass commented 4 years ago

Any progress on this one, guys?

timstokman commented 4 years ago

The fix worked for me, but I discovered another issue. For now I reverted the library, seems the older version works with even the newest asp core version. I'll probably be replacing or removing this in the future.

bryancass commented 4 years ago

Cool, maybe I'll try that. Which version did you install? Did you just do that within Visual Studio and NuGet to install an earlier version, and it replaces the newer version?

timstokman commented 4 years ago

2.0.0. But I suggest you just revert to whatever it was before for your project.

bryancass commented 4 years ago

I installed v3.0 initially, so I have nothing to revert to. Sorry -- I see where you can install an older version in NuGet. I'll try 2.1.0.

Just tried it and nope, still doesn't work. It inserts an invalid breadcrumb for "Index" that does not exist as a view. When I click on the breadcrumb link:

Home > Index > Select Recipients

InvalidOperationException: The view 'Index' was not found. The following locations were searched: /Views/Job/Index.cshtml /Views/Shared/Index.cshtml /Pages/Shared/Index.cshtml

zHaytam commented 4 years ago

Hello, really sorry for the lack of communication, my time has been very limited these weeks. I will try to fix this issue asap. @timstokman mind telling me what other issues did you find?

bryancass commented 4 years ago

OK, thank you Zanid. If you want, you can send me your updates and I will beta-test for you before you commit changes. I'd be glad to help out.

timstokman commented 4 years ago

@zHaytam In version we were able to do this:

[DefaultBreadcrumb("", IconClasses = "fas fa-home")]

Which hid the breadcrumbs on the default node (I only need the crumbs if there's something to navigate). In the newest version, if title is "" or null it gets set to some default value and you're no longer able to do this.

@bryancass

You can revert the version by editting the csproj file (just fill in the version), or selecting it in visual studio like this:

afbeelding

zHaytam commented 4 years ago

Good news @bryancass @timstokman, I fixed both your issues!
Indeed the problem was that commit you pointed at, which added some "forced" functionnality.

In order to fix the issues, I added two options:

You will need to set these options to false so that you get the behavior you want.
The NuGet version with this fix is 3.1.1.

timstokman commented 4 years ago

Right, that works for me. Thanks.

bryancass commented 4 years ago

Awesome, thank you! I am away from home this week, but I will do some testing next week and let you know.  Bryan

Sent from Yahoo Mail on Android

On Mon, Feb 24, 2020 at 7:57 AM, Haytam Zanidnotifications@github.com wrote:
Good news @bryancass @timstokman, I fixed both your issues! Indeed the problem was that commit you pointed at, which added some "forced" functionnality.

In order to fix the issues, I added two options:

You will need to set these options to false so that you get the behavior you want. The NuGet version with this fix is 3.1.1.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

bryancass commented 4 years ago

You resolved one issue of the invalid "Index" breadcrumb. But a secondary issue still exists. With these new options set to False, you always only get one breadcrumb, for example Home / Index, or Home / Details, or Home / Edit. The complete chain from Home to the page you are on is lost. Please unzip and run the attached WebApplication1. I added comments to the view pages to illustrate the issue.

WebApplication1.zip

zHaytam commented 4 years ago

Where exactly is the problem? All of your controllers contain breadcrumbs with one level. They don't specify FromX, thus they will only get linked to the default breadcrumb, which is your Home.Index action. What's the behavior you want?

bryancass commented 4 years ago

Maybe I am misunderstanding what a "breadcrumb" is? I thought that SmartBreadcrumbs displays a breadcrumb trail of all of the pages you visit, from Home to the page you are on.

My web pages are hierarchical. So if you start at the Home page, then visit the Company Listing page, then from that page go to Company Details page, then from there go to Company Edit page, your breadcrumb trail would look like this:

Home / Company Listing / Company Details / Company Edit

But with v3.1.1, the breadcrumb trail looks like this:

Home / Company Edit

How do I navigate back to Company Listing or Company Details when they do not appear in the breadcrumb trail? See this Wiki page for how breadcrumbs should work:

https://en.wikipedia.org/wiki/Breadcrumb_navigation

Breadcrumbs typically appear horizontally across the top of a Web page, often below title bars or headers. They provide links back to each previous page the user navigated through to get to the current page or—in hierarchical site structures—the parent pages of the current one. Breadcrumbs provide a trail for the user to follow back to the starting or entry point.[1] A greater-than sign (>) often serves as hierarchy separator, although designers may use other glyphs (such as » or ›), as well as various graphical icons.

Thanks!

zHaytam commented 4 years ago

SmartBreadcrumbs can't create it alone, it doesn't have the necessary information I'm afraid. If your pages are static, then you can use FromAction or FromPage to link pages between them. But seing your example, I guess Company Details needs an Id, if I'm right, you'll need to create manual breadcrumbs. SmartBreadcrumbs is mainly here to facilitate this AND to draw the UI side.