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

Select Menu Interaction Failure #6322

Open
NavidZ opened this issue Apr 1, 2019 · 36 comments
Open

Select Menu Interaction Failure #6322

NavidZ opened this issue Apr 1, 2019 · 36 comments

Comments

@NavidZ
Copy link

NavidZ commented Apr 1, 2019

Steps to Reproduce:

  1. Visit http://archives.materializecss.com/0.100.2/forms.html#select)
  2. Select a "Select Menu"

Expected Behavior

The select menu opens.

Current Behavior

The select menu doesn't open on the first mouse click on Chrome 73 and higher.

Possible Solution

Chrome recently changed their click targeting logic to match the ui event spec. I don't seem to be able to find the exact code here in the repo but it seems that select component is reacting to the click event as well as maybe mousedown/up events for closing or opening the options. That may need some changes to get this issue fixed

Context

https://bugs.chromium.org/p/chromium/issues/detail?id=947874

@DanielRuf
Copy link
Contributor

Duplicate of https://bugs.chromium.org/p/chromium/issues/detail?id=941910#c6

This is a regression bug in Chrome 73.

@DanielRuf
Copy link
Contributor

It affects pickadate and other things too.

@DanielRuf
Copy link
Contributor

Also see

amsul/pickadate.js#1138

amsul/pickadate.js#1140

amsul/pickadate.js#1145

Again, this is not a bug on our side as the code worked like thjs the whole time and still works in all other browsers. This happened due to refactorin and changes in the Chromium codebase.

It seems you are quick to label this bug as wontfix which is sad.

@NavidZ
Copy link
Author

NavidZ commented Apr 1, 2019

FYI @smaug----

Chrome had an unspecified behavior that blocked calculating click target over the interactive elements. This was never specified and nor implemented in other browsers like FF.

Chrome 73 removed that behavior to match the FF implementation. Unfortunately at the same time FF also switch to Chrome behavior to get more consistency but this behavior is nevertheless unspecified. I suggest changing some listeners around in this framework to rely on a more specified behavior. WDYT?

@DanielRuf
Copy link
Contributor

@DanielRuf
Copy link
Contributor

DanielRuf commented Apr 1, 2019

Chrome 73 removed that behavior to match the FF implementation. Unfortunately at the same time FF also switch to Chrome behavior to get more consistency but this behavior is nevertheless unspecified. I suggest changing some listeners around in this framework to rely on a more specified behavior. WDYT?

Please see the linked issue. It is a confirmed regression.

@NavidZ
Copy link
Author

NavidZ commented Apr 1, 2019

@smaug----

Wrong user.

No. I meant to cc him. He is from Mozilla and worked on the same change on FF part.

@smaug----
Copy link

So Firefox 66 changed click detection.
It used to follow earlier version of UI events spec, where mousedown/up had to happen on same element. Now it finds the common ancestor, but stops at first interactive content. Chrome used to have the same behavior as FF has now.

@NavidZ
Copy link
Author

NavidZ commented Apr 1, 2019

@DanielRuf Let me look at this as part of
https://bugs.chromium.org/p/chromium/issues/detail?id=941910

@NavidZ
Copy link
Author

NavidZ commented Apr 1, 2019

@DanielRuf do you by any chance know what has changed from
https://materializecss.com/select.html
to
http://archives.materializecss.com/0.100.2/forms.html#select
?

I don't know exactly which branch are those links from and the code is obfuscated and hard to parse.

@DanielRuf
Copy link
Contributor

http://archives.materializecss.com/0.100.2/forms.html#select

It happens in 0.100.x.

See

#6312
#6316
#6318
#6320
#6323

Go to the docs : http://archives.materializecss.com/0.100.2/forms.html with Chrome
On Select section and try to the first select.

and a few more.

Plus #6322 (comment)

@DanielRuf
Copy link
Contributor

@NavidZ
Copy link
Author

NavidZ commented Apr 2, 2019

I debugged the framework code a bit. I believe the better fix seems to be in the framework rather than restoring the old behavior of Chrome as we want to move away from unspecified behaviors in general in browsers.

Looking at the way the select is used I believe you need to have a call to stopPropagation of click event for ".select-wrapper" div. This whole issue happens because there is a click handler on the document that closes any opened dropdown or anything of that sort and the assumption here was that there was no click without really preventing it in the first common ancestor of your common ancestor. WDYT?

As a quick temporary fix if you run this in that page things seem to work without anything else being broken. Let me know if I missed a use case that might be broken due to this:

document.querySelectorAll('.select-wrapper').forEach(t => t.addEventListener('click', e=>e.stopPropagation()))

@kcahir
Copy link

kcahir commented Apr 2, 2019

This is sensible temporary fix, can be applied where updating materialize / pickadate version isn't possible, and keeps the quick UI response when elements are focused. The original UI event behaviour was a bit arbitrary.

@DanielRuf
Copy link
Contributor

This does not fix most cases, see https://codepen.io/DanielRuf/pen/rRENBm?editors=1010 and the two lines with // this will not work as this is still a regression bug in Chromium.

@kcahir
Copy link

kcahir commented Apr 2, 2019

Maybe something closer to this https://codepen.io/anon/pen/QPjbWj?editors=1011 where it says // common ancestor

@DanielRuf
Copy link
Contributor

Maybe but let's get back to the root cause. A change in Chromium. It worked the last 3-5 years. So it is still a regression on your side as this is something that breaks much more things (in pickadate, materialize and others).

Otherwise there would not be so many people who watch my initial bug report.

Because no one would try to "fix" it like this.

@DanielRuf
Copy link
Contributor

As previous contributor and maintainer of both projects I never saw such an issue in the last years - in any of the big browsers - until the change in M73 landed.

@j4kim
Copy link

j4kim commented Apr 3, 2019

Hi !
I use Materialize 0.100.2 in many internal webapps and most of my users use Chrome (others use IE, there is no FF in my company).
Sorry but I am a little lost here and I really don't know what I should do:

  • Recommand all my users to use IE ? (I fought to move from IE to Chrome so I can't really do that)
  • Use the trick from Select Menu Interaction Failure #6322 (comment) ? (this doesn't fix date and time pickers so not a solution)
  • Upgrade all my apps to Materialize 1.0.0 ? (this is a huge refactoring work)

As I understood Chrome won't rollback and Materialize can't do anything so I'm interested in your opinion

@DanielRuf
Copy link
Contributor

DanielRuf commented Apr 3, 2019

amsul/pickadate.js#1145 (comment) might give an idea.

@NavidZ
Copy link
Author

NavidZ commented Apr 3, 2019

This does not fix most cases, see https://codepen.io/DanielRuf/pen/rRENBm?editors=1010 and the two lines with // this will not work as this is still a regression bug in Chromium.

I may need to look at each of the different frameworks to see what the problem is. However as far as I looked into Materialize the click handler on the document usually dismisses the dropdown. So even now when the user clicks on the options (like in a multiple option dropdown) it is calling stopPropagation to prevent that click event to get to the body to result in the dismissal of the dropdown. So that the multiple choice dropdown remains open due to that stopPropagation. So that idea of preventing click propagation to get the to document level handler was already employed in the code. But just not at the right level of the hierarchy of DOM. Again I don't deny it used to work on Chrome as is. But it has been working just because of the unspecified behavior of Chrome. So I still don't quite see the reasoning that you call this a temporary fix and not the right fix in the framework.

Just to explain the situation a little better. It used to work on other browsers because of different reasons. In Chrome we have always sent the click to the fist common ancestor of mouse down and up targets (as oppose to what amsul/pickadate.js#1145 reports) except when the ancestor path was passing an interactive element boundary (which was the unspecified behavior). The change we landed in 73 was to relax this interactive element thing which was discussed in W3C UI events working group. FF used to not to send the click at all if the down and up targets were different. They recently changed their behavior as well. Safari is even slightly different from what Chrome does today. They all have their own little different behaviors in this particular case. We are just trying to land on a common and most reasonable behavior for all the browsers and get them all to provide the same behavior.

There are indeed lots of different behaviors in different scenarios when in comes to different browsers like the one above. We are working through those to make them more consistent and predictable. So through these we need to work with framework owners/maintainers to have a more robust code and get the right fixes in the frameworks as well. @smaug---- can talk more about FF plans on this particular issue.

@j4kim
Copy link

j4kim commented Apr 3, 2019

amsul/pickadate.js#1145 (comment) might give an idea.

Great, so this fixes the datepicker, right ?
Do you think it's possible to have a Materialize 0.100.2 release with these changes merged ?
And what is your recommandation about the Select elements ?

Thank's a lot and sorry for my lack of expertise

@DanielRuf
Copy link
Contributor

Materialize 0.x does not get any further patch releases, only 1.x.

@DanielRuf
Copy link
Contributor

They recently changed their behavior as well

I saw no such issue in latest Firefox.

@smaug----
Copy link

See #6322 (comment)

@j4kim
Copy link

j4kim commented Apr 4, 2019

I've made a Materialize 0.100.2 build that integrates the modifications on pickadate from amsul/pickadate.js#1145 (ad60ed5) and the trick from #6322 (comment) (36099c6)

-> dist js files

I don't know if it's robust but now date picker and select elements seem to work fine.
Time picker is still broken though...

@keshav0016
Copy link

keshav0016 commented Apr 10, 2019

I've made a build that integrates the modifications on pickadate from amsul/pickadate.js#1145 (ad60ed5) and the trick from #6322 (comment) (36099c6)

-> dist js files

I don't know if it's robust but now date picker and select elements seem to work fine.
Time picker is still broken though...

Are these changes made according to the materialize 0.100.2 version?
If yes, can we merge it to master after reviewing, so that it can be used in the current projects?

@DanielRuf
Copy link
Contributor

Are these changes made according to the materialize 0.100.2 version?
If yes, can we merge it to master after reviewing, so that it can be used in the current projects?

Again, v0.x is not developed anymore. v1.x is the current release branch.

I am not aware of patches which fix all components in Materialize.

@keshav0016
Copy link

Are these changes made according to the materialize 0.100.2 version?
If yes, can we merge it to master after reviewing, so that it can be used in the current projects?

Again, v0.x is not developed anymore. v1.x is the current release branch.

I am not aware of patches which fix all components in Materialize.

Can we fix it for 0.x as well. Because same issue is replicated in 0.x as well.
The datepicker flickers when we try to open it.
And we need to click twice for the input type='select', to open the dropdown.

Is there a way it can be fixed in the 0.x version as well?

@j4kim
Copy link

j4kim commented Apr 11, 2019

@keshav0016 yes, it fixes select elements and date picker for 0.100.2.
(I updated my comment above as I forgot to specify the version)

@keshav0016
Copy link

@keshav0016 yes, it fixes select elements and date picker for 0.100.2.
(I updated my comment above as I forgot to specify the version)

@DanielRuf So, when can we expect the fixes marge?

@DanielRuf
Copy link
Contributor

@DanielRuf So, when can we expect the fixes marge?

0.x is not actively developed anymore. Only 1.x.

@ray007
Copy link
Contributor

ray007 commented Apr 11, 2019

Maybe ask for a volunteer for 0.x maintenance?

@DanielRuf
Copy link
Contributor

Maybe ask for a volunteer for 0.x maintenance?

If someone wants to provide a patch as PR, this is very welcome.

@LucasPenido
Copy link

This Merge worked for me!! Thank you, Abe!!

@Richar-p
Copy link

Richar-p commented Dec 7, 2020

Hello,

Using rails with materialize-sass 1.0.0 and materialize-form 1.0.8 gems I also have this problem with chrome version 87.0.4280.88

Anyone else still has the problem?

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

No branches or pull requests

10 participants