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

m.route.Link doesn't proxy event handler prevention #2767

Closed
barneycarroll opened this issue May 6, 2022 · 4 comments · Fixed by #2768
Closed

m.route.Link doesn't proxy event handler prevention #2767

barneycarroll opened this issue May 6, 2022 · 4 comments · Fixed by #2768
Labels
Type: Bug For bugs and any other unexpected breakage

Comments

@barneycarroll
Copy link
Member

m.route.Link documentation misleadingly states that

You can also prevent navigation by, in an onclick handler, invoking ev.preventDefault() or returning false. This is the same way you block other events, so it's pretty natural.

Neither of those criteria will actually prevent the default behaviour of a Link as they do with a simple a. It's a shame because this is IMO the clearest sentence in this APIs documentation, and it's false. I wrote a little matrix sandbox to try and work out how event handling logic intersected with Link behaviour here, it may be useful for anyone else trying to reconcile expectations.

It took me reading the source & returning to read the docs a few times over to realise they're trying to explain the behaviour of a special-cased magic disabled attribute, which is the only way of preventing an Link from resolving. On that front, they're extremely unclear. Critically, the published docs have a typo stating that the disabled attribute is forwarded to the m.route.set API.

@barneycarroll barneycarroll added the Type: Bug For bugs and any other unexpected breakage label May 6, 2022
@barneycarroll
Copy link
Member Author

As a priority, the docs need fixing to remove the misleading line about event handlers & fix the API signature table's description of disabled. That can close this bug IMO.

@pygy
Copy link
Member

pygy commented May 6, 2022

The router has been completely rewritten since I left, and I'll have to wrap my head around the new code first if we decide something needs fixing

@StephanHoyer
Copy link
Member

Do you think it's a release blocker? I was planning to release this weekend 😉

Can anyone fix the docs for now so we can do bugfix release later on?

@pygy
Copy link
Member

pygy commented May 7, 2022

@barneycarroll can you do it? I've yet to wrap my head around the issue, and I have the kids this weekend. I may have a look at it this evening otherwise.

barneycarroll added a commit that referenced this issue May 10, 2022
* Simpler m.route.Link documentation. Fixes #2767

* Remove redundant HTML encoding from markdown docs

* Warn about m.route.Link immunity to event handler API.

* Integrate @JAForbes review corrections

* Example code typo in docs/route.md

Co-authored-by: Matias Kinnunen <github@mtsknn.fi>

Co-authored-by: Matias Kinnunen <github@mtsknn.fi>
StephanHoyer pushed a commit that referenced this issue May 16, 2022
* Simpler m.route.Link documentation. Fixes #2767

* Remove redundant HTML encoding from markdown docs

* Warn about m.route.Link immunity to event handler API.

* Integrate @JAForbes review corrections

* Example code typo in docs/route.md

Co-authored-by: Matias Kinnunen <github@mtsknn.fi>

Co-authored-by: Matias Kinnunen <github@mtsknn.fi>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug For bugs and any other unexpected breakage
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants