[MGNLUI-3539] Favorites: After opening a bookmark in favorites, most clicks don't react anymore Created: 31/Aug/15  Updated: 25/May/18  Resolved: 29/Dec/15

Status: Closed
Project: Magnolia UI
Component/s: favorites
Affects Version/s: 5.4, 5.4.1
Fix Version/s: 5.4.4

Type: Bug Priority: Critical
Reporter: Christian Ringele Assignee: Federico Grilli
Resolution: Fixed Votes: 1
Labels: None
Remaining Estimate: 0d
Time Spent: 6h 10m
Original Estimate: Not Specified

Issue Links:
Relates
relates to MGNLUI-3717 BrowserLocation should treat null 'pa... Closed
relates to MGNLUI-4088 Favorites: only works when the app is... Closed
duplicate
is duplicated by MGNLUI-3667 Favorites: After opening a bookmark i... Closed
relation
is related to MGNLCE-14 Provide new UI test for favourites Closed
Template:
Acceptance criteria:
Empty
Task DoD:
[ ]* Doc/release notes changes? Comment present?
[ ]* Downstream builds green?
[ ]* Solution information and context easily available?
[ ]* Tests
[ ]* FixVersion filled and not yet released
[ ]  Architecture Decision Record (ADR)
Bug DoR:
[ ]* Steps to reproduce, expected, and actual results filled
[ ]* Affected version filled
Date of First Response:
Sprint: Basel 19, Basel 24
Story Points: 5

 Description   

Reproduce:

Create a link in Favorites.
Try to use it (sometimes works the first time).

Then try to use it again, or close an app.
depended on the OS and the browser sometimes all clicks are hooked, or just certain.
But what appears in all situations not to work is:

  • Favorites can't be clicked again
  • Apps cant be closes
  • Most apps don't open, stay on loading
    All this until you refresh admin-central and click on a favorite again.


 Comments   
Comment by Sven Bach [ 24/Sep/15 ]

Any news on this?

Seems not to be a browser issue.
Testet Chrome / FireFox / IE on different Windows Versions.

Not affects 5.3.6 / 5.3.8
Affects 5.4.2.
(try https://demoauthor.magnolia-cms.com)

Figured out that the LocationChangedEvent of AdmincentralEventBus is not fired.
(set a breakpoint in info.magnolia.ui.contentapp.browser.BrowserSubApp constructor)

Feel free to ask for more information.

Comment by Philip Mundt [ 21/Dec/15 ]

The changes on the ctor of info.magnolia.ui.admincentral.shellapp.favorites.FavoritesPresenter break binary compatibility.

Comment by Mikaël Geljić [ 21/Dec/15 ]

While QAing MGNLUI-3717, I found out the preloader zoom-in transition is gone with this change :# Shall I reopen as well?

Comment by Federico Grilli [ 21/Dec/15 ]

Hmm, the only change I made was to use LocationController instead of replacing the location with Vaadin's page object. How could such a despicable thing happen because of that change? :/

Comment by Mikaël Geljić [ 22/Dec/15 ]

Reopening anyhow, it's at least worth investigating, although unfortunate. :/ If it takes longer to hunt down, I will agree to proceed and fix later, since this bug here was far more critical anyway.

Comment by Mikaël Geljić [ 22/Dec/15 ]

The more I think about it, the more I doubt using LocationController is a good fit for now—preloader has to be kicked in by client-side, *before* sending request to sync location back with server.
That's probably the reason why we were doing it with the Vaadin Page, that is:

  1. server response updates client location;
  2. client listens and sends request to sync location with server;
  3. server updates LocationController

Right, a bit convoluted, but was working till 5.4.
Ideally, Shell apps should be smarter on the client-side, so we would change the location from there, rather than forking that via `FavoritesPresenter`+`Page`; but it would be interesting nonetheless to identify why the listening part (point #2 above) stopped working.

Comment by Federico Grilli [ 29/Dec/15 ]

mgeljic indeed it looks like this stopped working due to this change in Vaadin's Page https://dev.vaadin.com/ticket/12925. That occurred in version 7.2, which would explain why the issue doesn't show up in Magnolia 5.3.x (using Vaadin 7.1.x). One way to have the transition back would be to use the implementation below where we use directly Page.open(..) and pass null instead of "_self" as windowName parameter. This will "somehow" trigger again the FragmentChangedEvent which is handled at info.magnolia.ui.api.location.new FragmentChangedHandler() and which in turn apparently triggers the transition magic

 @Override
    public void goToLocation(final String location) {
        final String completeLocation = getCompleteURIFromFragment(location);
        Page.getCurrent().open(completeLocation, null, false);
    }
Comment by Mikaël Geljić [ 29/Dec/15 ]

Thanks fgrilli! Sounds good to me, as long as we leave a comment (since it's not well documented on Page#open).
Alternatively, I also saw:

  • Page#setUriFragment(String, boolean) sounds like a good fit, at first glance
    • maybe with fireEvents = false, since we listen on client with History handler, in MagnoliaShellConnector
  • or Page#updateLocation(String, boolean), although advertised "For internal use only" (but not more internal that relying on passing null)
Comment by Mikaël Geljić [ 29/Dec/15 ]

Well, I ended up trying it out briefly:

  • #setUriFragment() doesn't work fully, #updateLocation work even less :/
  • #open(loc, null) works all fine, at least similar to 5.3.x
    • there is a recurring issue that page first reloads when there are value-less query params, but that's been here for ages

I'm happy to go with this work-around for now, although I have the suspicion that we do not handle fragment changes correctly—#setUriFragment should work. Let's save this for later.

Comment by Federico Grilli [ 29/Dec/15 ]

Hmm okay, I actually used #setUriFragment() and seemed to work fine...

Comment by Mikaël Geljić [ 29/Dec/15 ]

iirc it doesn't re-sync selection properly

Comment by Nils Breunese [ 11/Jan/16 ]

Is MGNLUI-2981 related to this?

Comment by Federico Grilli [ 11/Jan/16 ]

Seems not to be related to this issue. I could reproduce MGNLUI-2981 on latest SNAPSHOT bundle where this is already integrated.

Comment by Nils Breunese [ 11/Jan/16 ]

Hm, sadly favorites still won't be very useful for editors then, but thanks for checking.

Comment by Federico Grilli [ 12/Jan/16 ]

Np, that's unfortunate indeed and hopefully will be tackled asap.

Generated at Mon Feb 12 09:07:37 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.