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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Autocomplete: Fix menu staying open on BSS and mobiles #4326

Merged
merged 3 commits into from Apr 3, 2022

Conversation

mckaragoz
Copy link
Member

Description

Fixes #4303.

First of all this is the not final solution about this issue. There is really a deeper bug when occures complex things happened.

I believe this bug is about timing. So we can fix it with control event timing.

So;

  1. Added ontouchend event both list item and overlay in autocomplete. I don't know the exact reason, but ontouchend fires before onclick and helps the timing problem.
  2. You can ask why we didn't use ontouchstart. Because if we use touchstart, it fires first and closes the popover in autocomplete. And then click event starts and if there is something behind the popover, clicks it. We don't want this.
  3. The only problem about touchend is calling same method twice with click event. So we checked the touch event and if user touches the component, the code understand it and skips click event.
  4. This look like fix server side touch problem, but have some unwanted side effects on WASM-touch. The current WASM-touch is already working properly, so we skipped this touch events if runtime location is WASM.
    This PR also supports direct click between autocompletes without touch again.

How Has This Been Tested?

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Autocomplete server side-touch

20220401_201705.mp4

Autocomplete direct touch (in mobile no need to touch somewhere to close overlay and touch other component again, now user can directly touch and interact another component)

20220401_201811.mp4

Checklist:

  • The PR is submitted to the correct branch (dev).
  • My code follows the code style of this project.
  • I've added relevant tests. (Test it if you can 馃檪 )

@codecov
Copy link

codecov bot commented Apr 1, 2022

Codecov Report

Merging #4326 (cb2773f) into dev (5241e8d) will decrease coverage by 0.05%.
The diff coverage is 36.36%.

@@            Coverage Diff             @@
##              dev    #4326      +/-   ##
==========================================
- Coverage   91.22%   91.17%   -0.06%     
==========================================
  Files         361      361              
  Lines       12450    12458       +8     
==========================================
+ Hits        11357    11358       +1     
- Misses       1093     1100       +7     
Impacted Files Coverage 螖
...c/MudBlazor/Components/Popover/MudPopover.razor.cs 84.21% <酶> (-0.79%) 猬囷笍
...r/Components/Autocomplete/MudAutocomplete.razor.cs 91.16% <30.00%> (-2.99%) 猬囷笍
...azor/Components/Autocomplete/MudAutocomplete.razor 93.75% <100.00%> (酶)

Continue to review full report at Codecov.

Legend - Click here to learn more
螖 = absolute <relative> (impact), 酶 = not affected, ? = missing data
Powered by Codecov. Last update 5241e8d...cb2773f. Read the comment docs.

@henon
Copy link
Collaborator

henon commented Apr 1, 2022

I looked at the code and commented. I think the most important is for those who have reported the bug to test it out and confirm that it fixes the bug reliably.

@mblichowski
Copy link

I looked at the code and commented. I think the most important is for those who have reported the bug to test it out and confirm that it fixes the bug reliably.

Worked like a charm in my applications. However I didn't test WASM, only Server hosting model apps.

@negator13
Copy link

What would be the best way to test this in my application?

Pull down this branch, build a local nuget package for it, import it into my app and deploy it to my published app to test it?

@henon
Copy link
Collaborator

henon commented Apr 2, 2022

What would be the best way to test this in my application?

Pull down this branch, build a local nuget package for it, import it into my app and deploy it to my published app to test it?

yes

@mblichowski
Copy link

Pull down this branch, build a local nuget package for it, import it into my app and deploy it to my published app to test it?

That's exactly what I did.
If you override your local nuget packages cache %userprofile%\.nuget\packages\mudblazor\6.0.9\lib\net6.0 and redeploy your app, it should be ok - like I said, it worked for me.

Another advantage we gain with this fix is that it's no longer needed to have those delays in search function. Probably since the refactored click/touch callback handlers are now being awaited.

@negator13
Copy link

negator13 commented Apr 2, 2022

I built this feature branch and created a new nuget package for it (6.0.800), and deployed my application targeting that package, and unfortunately I am still easily getting the issue to reproduce:

(I took this screenshot via Snipping Tool, so you can tell that even though I had clicked off the dropdown and even off the page entirely it still was stuck open).

I am not seeing this behavior occur on the older 6.0.5 version.

image

@henon
Copy link
Collaborator

henon commented Apr 2, 2022

@negator13 just negated this fix :D. You sure you didn't accidentally still deploy 6.0.9 i.e. because you maybe didn't do a clean or something? Maybe print the version out somewhere on your app so you can be sure.

@bugproof
Copy link

bugproof commented Apr 2, 2022

What I did was referencing the project instead of creating a local nuget package. Much faster to setup. This PR fixed the issue for me.

@negator13
Copy link

I have now referenced the project (pulled from this PR branch) and deployed to my test instance (updated the app version to 0.5.3 as suggested to be sure it is the latest build). I am still instantly getting the issue behavior (SS below).

The test instance currently has this branch of MudBlazor referenced and can be seen here: https://eldenring-test.azurewebsites.net/calculator

I greatly appreciate the effort that is being put into this fix and I'm sorry to report the continued issue, I am doing what I can to make sure I am giving the best info I can.

image

@bugproof
Copy link

bugproof commented Apr 2, 2022

The test instance currently has this branch of MudBlazor referenced and can be seen here: eldenring-test.azurewebsites.net/calculator

Just tested and no touch issues on my end.

@henon
Copy link
Collaborator

henon commented Apr 2, 2022

I can still repro the issue with @negator13 's link. But only for the Weapons autocomplete. The others seem to work fine. What is different with that one?

autocomplete issue

@negator13
Copy link

I can still repro the issue with @negator13 's link. But only for the Weapons autocomplete. The others seem to work fine. What is different with that one?

The Weapons is a MudAutocomplete, the others are MudSelect. I posted a snippet in the original thread in the original Issue: https://try.mudblazor.com/snippet/wEQQEdHbSXsmUPUo

It doesn't compile, but it should hopefully illustrate how I am using the component (maybe incorrectly?)

@henon
Copy link
Collaborator

henon commented Apr 2, 2022

I can still repro the issue with @negator13 's link. But only for the Weapons autocomplete. The others seem to work fine. What is different with that one?

The Weapons is a MudAutocomplete, the others are MudSelect. I posted a snippet in the original thread in the original Issue: https://try.mudblazor.com/snippet/wEQQEdHbSXsmUPUo

It doesn't compile, but it should hopefully illustrate how I am using the component (maybe incorrectly?)

I don't think you are doing anything wrong if it worked with an older version of MudBlazor.

We are still shooting in the dark here because we don't know which commit caused the change in behavior in MudAutocomplete. I suggested finding the offending commit with git bisect in the issue, it would require you to reference the mudblazor source project directly in your app and do several publish steps until git bisect converges on the culprit. It would probably take less than an hour to do it. Just follow the instructions of the bisect command line command.

@henon
Copy link
Collaborator

henon commented Apr 2, 2022

@negator13 If you can't manage to bisect I can do it with your code base assuming that the error happens when I publish it to a free azure instance.

@negator13
Copy link

@henon I have added you as a collaborator in the repo for my app (private) - I'm not sure I will have time to try the git bisect this weekend, if you are able to assist that would be great, I appreciate it

@bugproof
Copy link

bugproof commented Apr 3, 2022

I can still repro the issue with @negator13 's link. But only for the Weapons autocomplete. The others seem to work fine. What is different with that one?

Weird I can't reproduce it. It works for me even with Weapons autocomplete. Might be a browser difference? I tested it on both Edge and Chrome latest versions (100)

@henon
Copy link
Collaborator

henon commented Apr 3, 2022

Guys, I have identified the commit which caused this issue. Bisecting is no big deal, took me about 30 minutes with 7 publish steps to Azure.

6ea9967 AutoComplete: Fix initial open with server-side and non-async search (#3776)

And here is the bisect log for those who are interested:

henon@MONSTER MINGW64 /d/MudBlazor.git (dev)
$ git bisect start

henon@MONSTER MINGW64 /d/MudBlazor.git (dev|BISECTING)
$ git bisect bad

henon@MONSTER MINGW64 /d/MudBlazor.git (dev|BISECTING)
$ git bisect good v6.0.5
Bisecting: a merge base must be tested
[23b9dab3b20df6c97be31273fcbc1b363d9d8834] Docs: Cache bust 6.0.5

henon@MONSTER MINGW64 /d/MudBlazor.git ((23b9dab3b...)|BISECTING)
$ git bisect good
Bisecting: 100 revisions left to test after this (roughly 7 steps)
[f5b91c980b37ecee9813824cdc7ac6543d54626f] Docs: Added Example Width component

henon@MONSTER MINGW64 /d/MudBlazor.git ((f5b91c980...)|BISECTING)
$ git bisect bad
Bisecting: 50 revisions left to test after this (roughly 6 steps)
[d820d15af79c0babfd18c44801c4c817db2426c6] Build(deps): Update Microsoft.AspNetCore.Components requirement in /src (#3948)

henon@MONSTER MINGW64 /d/MudBlazor.git ((d820d15af...)|BISECTING)
$ git bisect good
Bisecting: 25 revisions left to test after this (roughly 5 steps)
[c4e8b04ba39bcfbb50bf4b8e5b634f18f29e4c16] Update ci-cd.yml

henon@MONSTER MINGW64 /d/MudBlazor.git ((c4e8b04ba...)|BISECTING)
$ git bisect good
Bisecting: 12 revisions left to test after this (roughly 4 steps)
[0770750179fd57b98ba05a8f38ad03d8e18b3352] Docs: Fixed broken image link on landing page

henon@MONSTER MINGW64 /d/MudBlazor.git ((077075017...)|BISECTING)
$ git bisect good
Bisecting: 6 revisions left to test after this (roughly 3 steps)
[eddb9a958bc41fcc5708ebd1a863d5f5ce85f4e4] MudMask: Fix cursor jumping in table inline edit mode. (#4037)

henon@MONSTER MINGW64 /d/MudBlazor.git ((eddb9a958...)|BISECTING)
$ git bisect good
Bisecting: 3 revisions left to test after this (roughly 2 steps)
[b0cf3b399390ef9be0bd62f523a4ee6ab9d70c4e] MudFormComponent : Fix field validation when triggered before a required cascading parameter (#3943)

henon@MONSTER MINGW64 /d/MudBlazor.git ((b0cf3b399...)|BISECTING)
$ git bisect good
Bisecting: 1 revision left to test after this (roughly 1 step)
[6ea9967c9f4b43ed7cce60eb5070890186b850f9] AutoComplete: Fix initial open with server-side and non-async search (#3776)

henon@MONSTER MINGW64 /d/MudBlazor.git ((6ea9967c9...)|BISECTING)
$ git bisect bad
Bisecting: 0 revisions left to test after this (roughly 0 steps)
[a64cfada4fc1794b3ee1898d9af77f76af988ca7] Autocomplete: renamed DisableSelectOnClick to SelectOnClick

henon@MONSTER MINGW64 /d/MudBlazor.git ((a64cfada4...)|BISECTING)
$ git bisect good
6ea9967c9f4b43ed7cce60eb5070890186b850f9 is the first bad commit
commit 6ea9967c9f4b43ed7cce60eb5070890186b850f9
Author: Anders Storhaug <storhaug.3@gmail.com>
Date:   Thu Feb 24 14:51:03 2022 -0500

    AutoComplete: Fix initial open with server-side and non-async search (#3776)

 .../Autocomplete/AutocompleteSyncTest.razor        | 22 +++++++++++++++++
 .../Components/AutocompleteTests.cs                | 26 +++++++++++++++++++-
 .../Autocomplete/MudAutocomplete.razor.cs          | 28 ++++++++++------------
 .../Components/Popover/MudPopover.razor.cs         |  6 +++++
 4 files changed, 66 insertions(+), 16 deletions(-)
 create mode 100644 src/MudBlazor.UnitTests.Viewer/TestComponents/Autocomplete/AutocompleteSyncTest.razor

henon@MONSTER MINGW64 /d/MudBlazor.git ((a64cfada4...)|BISECTING)
$ git bisect reset
Previous HEAD position was a64cfada4 Autocomplete: renamed DisableSelectOnClick to SelectOnClick
Switched to branch 'dev'
Your branch is up to date with '_MudBlazor/dev'.

henon@MONSTER MINGW64 /d/MudBlazor.git (dev)

@mckaragoz
Copy link
Member Author

Are you sure, i can reproduce the autocomplete thing after 5.2.0

@henon
Copy link
Collaborator

henon commented Apr 3, 2022

Yes I am sure that it caused @negator13 's problem. Not sure if we are talking about the same thing. Anyhow, I identified the code that is the culprit:

#3776 added this in Popover:
image

Commenting it out makes the issue go away in the eldenring AR calculator.

@mckaragoz
Copy link
Member Author

I can confirm here is no problem with open and close

@henon
Copy link
Collaborator

henon commented Apr 3, 2022

Thanks @mckaragoz and everyone who tested.

@negator13
Copy link

What version will this fix be a part of?

@mckaragoz
Copy link
Member Author

Thanks @mckaragoz and everyone who tested.

And thanks you especially

@henon
Copy link
Collaborator

henon commented Apr 3, 2022

What version will this fix be a part of?

v6.0.10

@Garderoben
Copy link
Member

@henon why was this merged? (The revert of 6ea9967 is totally fine and i see that you guys have confirmed its working after that but this PR also adds new stuff that was not changed in the reverted PR)

I requested that Benno would review it. It also has missing tests, can it not be done with this?https://bunit.dev/api/Bunit.TouchEventDispatchExtensions.html

Select / Autocomplete etc have gotten a lot of "fixes" like this lately where it hasn't been tested manually correctly or written tests for it. It's become a bit flip floppy where we patch one thing and break another and that's not good.

@mckaragoz
Copy link
Member Author

It also has missing tests

Actually i request about test because most of the untested code works only in server side and if im not wrong test project works on client side. So this one should test manually.

Maybe we turn back this with list rework? Let's not spend too much time on something that may change soon

@henon
Copy link
Collaborator

henon commented Apr 4, 2022

@Garderoben, I merged because I reviewed the changes and found them OK and also the change was tested by me and multiple others successfully. And as @mckaragoz already said, these BSS-specific changes are hard to test on the integration server and also a drop of -0.06% in coverage is no big deal really IMO.

Sorry, I forgot about your request for a review by @just-the-benno, but it is also not uncommon that a PR is requested to be reviewed and never reviewed by the one that was requested to review. If you look at all the merged PRs you'll find many that were never reviewed by the requested person which is ok. Whoever has time to review shall review and merge if the PR is ok. In any case, Benno can still review it and should he find anything that I overlooked then I am sure @mckaragoz will make the required amends.

In any case, this is a high-profile bug that needs to be fixed and released fast.

@henon
Copy link
Collaborator

henon commented Apr 5, 2022

We have a preview nuget of this fix: https://www.nuget.org/packages/MudBlazor/6.0.10-dev.1

@negator13
Copy link

@henon Unfortunately I think this fix introduces another bug on mobile - tried on both Android and IOS. When you open the Autocomplete dropdown, it will snap closed as soon as you try to scroll past the initially displayed items (and there is barely any screen space because of the keyboard opening on the phone).

Compared to version 6.0.5, where this is not happening. My website is now currently deployed with v6.0.5 again to mitigate this new issue.

@negator13
Copy link

I have a test instance of my site which is deployed using this prerelease version 6.0.10-dev.2, where this new bug can be seen on mobile: https://eldenring-test.azurewebsites.net/calculator

@henon
Copy link
Collaborator

henon commented Apr 17, 2022

This time bisect won't work I think because this bug was masked by the other one.

@negator13
Copy link

Agreed, just posted the test site so people can see/reproduce the behavior, it is very consistent.

@henon
Copy link
Collaborator

henon commented Apr 17, 2022

My guess is that this has to do with the browser forwarding the touch events to the click handler. @mckaragoz what do you think?

@henon
Copy link
Collaborator

henon commented Apr 21, 2022

@negator13 Mckaragoz removed the touch workaround. Please test once more on mobile. https://www.nuget.org/packages/MudBlazor/6.0.10-dev.4
Thanks.

@negator13
Copy link

@negator13 Mckaragoz removed the touch workaround. Please test once more on mobile. https://www.nuget.org/packages/MudBlazor/6.0.10-dev.4 Thanks.

It's definitely improved, I've deployed it here if you want to try: https://eldenring-test.azurewebsites.net/calculator

One thing that I notice is that on my Android device the keyboard tends to pop up AFTER you have selected a choice and the dropdown has closed.

@henon
Copy link
Collaborator

henon commented Apr 22, 2022

Is this a new thing or was it like that before also?

@negator13
Copy link

It looks like that was happening on 6.0.5 as well, so a separate issue. I can open another issue for that. The main regression here seems to be remediated in the 6.0.10-dev4 build.

@henon
Copy link
Collaborator

henon commented Apr 23, 2022

Thanks @negator13. Unfortunately this is not yet the end of this fix marathon. we have still an open problem with recursive popups. I found a solution working will on WASM in #4479 but in BSS there is still a problem.

@henon
Copy link
Collaborator

henon commented Apr 24, 2022

The problem with recursive popups (popovers opened from within popovers) is fixed in WASM and BSS now

jammerware pushed a commit to jammerware/MudBlazor that referenced this pull request Sep 20, 2022
鈥 reverting an optimization by MudBlazor#3776

* Autocomplete Touch Fix
* Popover: Reverting an optimization by MudBlazor#3776 that caused issue MudBlazor#4303

Co-authored-by: Meinrad Recheis <meinrad.recheis@gmail.com>
jammerware pushed a commit to jammerware/MudBlazor that referenced this pull request Sep 20, 2022
鈥 reverting an optimization by MudBlazor#3776

* Autocomplete Touch Fix
* Popover: Reverting an optimization by MudBlazor#3776 that caused issue MudBlazor#4303

Co-authored-by: Meinrad Recheis <meinrad.recheis@gmail.com>
jammerware pushed a commit to jammerware/MudBlazor that referenced this pull request Sep 20, 2022
鈥 reverting an optimization by MudBlazor#3776

* Autocomplete Touch Fix
* Popover: Reverting an optimization by MudBlazor#3776 that caused issue MudBlazor#4303

Co-authored-by: Meinrad Recheis <meinrad.recheis@gmail.com>
jammerware pushed a commit to jammerware/MudBlazor that referenced this pull request Sep 20, 2022
鈥 reverting an optimization by MudBlazor#3776

* Autocomplete Touch Fix
* Popover: Reverting an optimization by MudBlazor#3776 that caused issue MudBlazor#4303

Co-authored-by: Meinrad Recheis <meinrad.recheis@gmail.com>
3dots pushed a commit to 3dots/MudBlazor that referenced this pull request Mar 23, 2023
鈥 reverting an optimization by MudBlazor#3776

* Autocomplete Touch Fix
* Popover: Reverting an optimization by MudBlazor#3776 that caused issue MudBlazor#4303

Co-authored-by: Meinrad Recheis <meinrad.recheis@gmail.com>
ferraridavide pushed a commit to ferraridavide/MudBlazor that referenced this pull request May 30, 2023
鈥 reverting an optimization by MudBlazor#3776

* Autocomplete Touch Fix
* Popover: Reverting an optimization by MudBlazor#3776 that caused issue MudBlazor#4303

Co-authored-by: Meinrad Recheis <meinrad.recheis@gmail.com>
ilovepilav pushed a commit to ilovepilav/MudBlazor that referenced this pull request Nov 25, 2023
鈥 reverting an optimization by MudBlazor#3776

* Autocomplete Touch Fix
* Popover: Reverting an optimization by MudBlazor#3776 that caused issue MudBlazor#4303

Co-authored-by: Meinrad Recheis <meinrad.recheis@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something does not work as intended/expected
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Selects and Autocompletes get "stuck" in deployed Blazor Server app (Azure)
6 participants