In-course links in Notify content disable scrolling

[moloko]

If you include a hyperlink to another part of the course inside a Notify popup, the link works but scrolling is subsequently completely disabled.

This is presumably down to notifyView disabling scrolling when a Notify popup gets opened - but is not being given the chance to re-enable it again because clicking the link will cause Adapt to route to the new location without calling closeNotify.

Looking at the code I suspect this will also have an impact on accessibility, though I haven't tested that.

It seems reasonable to me that someone would want to add a link to another part of the course to a Notify popup - in this particular case it was to direct the user to a part of the course they needed to revisit because they'd answered a question incorrectly - so it would be great to get this working.

[danielstorey]

Would something as simple as this work?

danielstorey@f1047f6

[oliverfoster]

you just need to check the link target, a link going to a new window doesn't need to close the popup.

[oliverfoster]

or re-enable scrolling on navigation?

[oliverfoster]

or drop the disable scrolling behaviour? it is a bit rubbish (i know cos i wrote it).

[danielstorey]

Lol. Returning to the same place does make sense. Like if you scroll when notify is open you could lose your place.

Close notify ensures that the notify class is removed from html as well as scrolling re-enabled.

See #1679

[tomgreenfield]

I'd tend to agree with Ollie aiming more towards a re-enable-on-navigation-style approach.

Hardcoding an <a> tag event handler seems limited in scope, and also risky if you use an anchor for anything else within Notify.

You may have a button which performs Adapt.navigateToElement() on click, for example.

[danielstorey]

Sounds fair Tom but all the handler does is calls closeNotify, which makes sure that notify closes, class is removed from html and scroll is re-enabled, on the condition that target !== "_blank".

I can't think of a use case where this behaviour would not be required. Notice that I have not called event.preventDefault meaning that as per your example Adapt.navigateToElement would still get called. This handler just ensures the other essential stuff happens as well :)

[moloko]

The only problem I can really think of with @danielstorey 's solution is that you might have an <a> in there (that doesn't have a target of _blank) where you wanted the Notify popup to be left open when clicked. struggling to think of a real-world example though!

If this is a concern then a possible alternative would be to require links or buttons to have some kind of attribute that flags that notify should be closed. I'd normally plump for a data attribute but I don't think those are easy to add via the AT.

Equally I'm happy to go with the solution as-is - and if we find it's not working out we can sort it out then.

[danielstorey]

Bit hacky but could use classes instead of data attributes? Didn't know data attrs were difficult to add...

[moloko]

yeah you have to edit source to add data attrs which may be a bit much to ask

this is what you can add via the AT editor:

[tomgreenfield]

A few possible use cases for anchors:

• mailto links
• links with download attribute
• links which are used (hackily) to do an onclick action

To be honest, I was more worried about the bigger picture – if any navigation happens while in a Notify, you'll get the same frozen scrolling bug – <a> elements are just one way of triggering this. Try popping in a different ID in the URL while you're in a popup to test.

[danielstorey]

Ok it's now sounding like the most sensible thing to do would be to call closeNotify when the route changes.

Something like this.listenTo(Adapt, "router:location", this.closeNotify);

How does that sound?

[oliverfoster]

It's just a question of where you put it, where you remove the event listener and ensuring it only happens once. But yea, totally a better solution.

[danielstorey]

Well thanks to Backbone listeners get removed with the view. I think this cracks it though.

b420a36