Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

amp-lightbox not working after browser back, requires page reload #1463

Open
88kbbq opened this issue Jan 17, 2016 · 14 comments
Open

amp-lightbox not working after browser back, requires page reload #1463

88kbbq opened this issue Jan 17, 2016 · 14 comments

Comments

@88kbbq
Copy link

88kbbq commented Jan 17, 2016

I'm testing the use of amp-lightbox to display a full-viewport navigation menu on mobile devices. Links in the amp-lightbox navigate to pages as expected, but when testing, the <button on="tap:my-lightbox">Open lightbox</button> does not function as expected after returning to the page using the browser back button. The amp-lightbox button simply doesn't open the amp-lightbox. After the page is refreshed, the amp-lightbox will open as expected.

@dvoytenko
Copy link
Contributor

@sriramkrish85 could you take a look?

@camelburrito camelburrito added INTENT TO IMPLEMENT Proposes implementation of a significant new feature. https://bit.ly/amp-contribute-code and removed INTENT TO IMPLEMENT Proposes implementation of a significant new feature. https://bit.ly/amp-contribute-code labels Jan 22, 2016
@camelburrito
Copy link
Contributor

@88kbbq is there a page i can look at? I tried this in the some manual test files seems to be working.

@88kbbq
Copy link
Author

88kbbq commented Jan 23, 2016

Just uploaded to pos.88k.com.tw. I'll add some other pages, but it seems if the page doesn't have the lightbox.js then the lightbox won't open/close unless the page is reloaded. For testing, the catering.html has the lighbox.js but the restaurant.html does not. Also, the lightbox doesn't close if the user navigates to another page without closing the lighbox. Not sure how to implement the hamburger-style menu dropdown with AMPHTML, but if there's a better way just point me to those resources.

@88kbbq
Copy link
Author

88kbbq commented Jan 23, 2016

Maybe there could be a `on="tap:my-lightbox.reset"' that resets lightbox to default state after tapping a link

@88kbbq
Copy link
Author

88kbbq commented Jan 23, 2016

working around these AMP HTML restrictions is just a nightmare. How about you allow 50,000 bytes of inline JS like the 50,000 bytes amp-custom CSS? Restrict those functions that conflict with the v0.js and you could solve half the problems on Github in one fell swoop without a signification impact on page load speed. Consider? I mean, even GPRS gets 0.1Mb/s, so what's a split second really when an image would take just as much, if not longer? You could restrict it to before the </body> tag.

@rudygalfi rudygalfi added this to the M1 milestone Feb 4, 2016
@rudygalfi
Copy link
Contributor

@sriramkrish85 Can you test back button scenarios in top window and iframe? Close this out if no further action is needed.

The rest covered here will be addressed by menu work.

@rudygalfi rudygalfi modified the milestones: Feb 13, M1 Feb 4, 2016
@88kbbq
Copy link
Author

88kbbq commented Feb 5, 2016

Tested a bit, so far so good. Closing the amp-lightbox problem, but hopefully a menu option will be available soon.

@88kbbq 88kbbq closed this as completed Feb 5, 2016
@johndsci
Copy link

btw, this is still present in 2021.
amp-lightbox still not working on browser back.
everything works on desktop.
now working on mobile (tested using safari 14.4, chrome 87 app)

the only workaround is to use on="tap:AMP.navigateTo(url='some_url')" instead of href

@dvoytenko
Copy link
Contributor

@johndsci I've tried the examples listed here: https://amp.dev/documentation/examples/components/amp-lightbox/?format=websites and they all appear to work correctly with the back button. Is the situation you're describing somehow different?

@johndsci
Copy link

johndsci commented Apr 3, 2021

@dvoytenko I used amp-lighbox for a much cleaner alternative to sidebar nav.

users will navigate the pages by clicking some hamburger icon on the top which opens the lightbox with links inside. clicking the links would navigate to another page.

when on the next page, the browser's back option would bring you back to the previous page with the lighbox already opened and all buttons and links not working. you need to refresh the page again for everything to work.

everything's fine on desktop.
mobile browsers broken.

@dvoytenko
Copy link
Contributor

/cc @caroqliu this is an interesting case. There's a special navigation handler inside the sidebar that resolves history conflicts on navigation to avoid back button issues there. Perhaps we could make it available to all other overlaying components? And that also brings up the fact that we should probably also consider it for Bento?

/cc @ampproject/wg-components

@caroqliu
Copy link
Contributor

caroqliu commented Apr 15, 2021

There's a special navigation handler inside the sidebar that resolves history conflicts on navigation to avoid back button issues there.

@dvoytenko Are you referring to the following introduced due to #6585?:

if (target && target.href) {
const tgtLoc = Services.urlForDoc(element).parse(target.href);
const currentHref = this.getAmpDoc().getUrl();
// Important: Only close sidebar (and hence pop sidebar history entry)
// when navigating locally, Chrome might cancel navigation request
// due to after-navigation history manipulation inside a timer callback.
// See this issue for more details:
// https://github.com/ampproject/amphtml/issues/6585
if (removeFragment(target.href) != removeFragment(currentHref)) {
return;
}
if (tgtLoc.hash) {
// Click gesture is high trust.
this.close_(ActionTrust.HIGH);
}

Trying to think if there are other overlaying components to consider here... amp-consent, and maybe amp-sticky-ad? Do @ampproject/wg-stories folks have any overlaying components that manipulate history more directly? Would be good to know what others do if so.

@gmajoulet
Copy link
Contributor

We do for Stories, and do it a different way:

  1. When opening the overlaying component, we push a new history entry, and:
    a. add some data to the history entry so we remember it's open in case of page reload.
    b. register a method to close it when that entry is popped

const currentHistoryState = /** @type {!Object} */ (getState(
this.win.history
));
const historyState = {
...currentHistoryState,
[HistoryState.ATTACHMENT_PAGE_ID]: this.storeService_.get(
StateProperty.CURRENT_PAGE_ID
),
};
this.historyService_.push(() => this.closeInternal_(), historyState);

  1. The overlaying component can be closed by:
    a. a user navigating back, which will pop the history entry and call the callback we provided above
    b. a user interaction on the page, in which case we call history.back() manually, to pop the history entry and trigger its callback

this.historyService_.goBack();

@dvoytenko
Copy link
Contributor

@gmajoulet it sounds like you're trying to keep the overlay UI on refresh of the page? This doesn't always work well with all types of overlaying UI. Though it might be ok for us here.

@caroqliu, yes, that's the one I mean. We could always do it differently, of course, e.g. closer to what @gmajoulet, which avoids popping history on navigation. I think that could be ok as well for this use case. But then you have a choice of:

  1. Either restoring the lightbox when a user returns to the page.
  2. Not restoring the lightbox and just simply "living with" a dead history state.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants