materialize icon indicating copy to clipboard operation
materialize copied to clipboard

Select Menu Interaction Failure

Open NavidZ opened this issue 6 years ago • 36 comments

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

NavidZ avatar Apr 01 '19 18:04 NavidZ

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

This is a regression bug in Chrome 73.

DanielRuf avatar Apr 01 '19 18:04 DanielRuf

It affects pickadate and other things too.

DanielRuf avatar Apr 01 '19 18:04 DanielRuf

Also see

https://github.com/amsul/pickadate.js/issues/1138

https://github.com/amsul/pickadate.js/pull/1140

https://github.com/amsul/pickadate.js/pull/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.

DanielRuf avatar Apr 01 '19 18:04 DanielRuf

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?

NavidZ avatar Apr 01 '19 18:04 NavidZ

See https://github.com/Dogfalo/materialize/blob/v0.100.2/js/forms.js

DanielRuf avatar Apr 01 '19 18:04 DanielRuf

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.

DanielRuf avatar Apr 01 '19 18:04 DanielRuf

@smaug----

Wrong user.

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

NavidZ avatar Apr 01 '19 19:04 NavidZ

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.

smaug---- avatar Apr 01 '19 19:04 smaug----

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

NavidZ avatar Apr 01 '19 20:04 NavidZ

@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.

NavidZ avatar Apr 01 '19 20:04 NavidZ

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

It happens in 0.100.x.

See

https://github.com/Dogfalo/materialize/issues/6312 https://github.com/Dogfalo/materialize/issues/6316 https://github.com/Dogfalo/materialize/issues/6318 https://github.com/Dogfalo/materialize/issues/6320 https://github.com/Dogfalo/materialize/issues/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 https://github.com/Dogfalo/materialize/issues/6322#issuecomment-478700529

DanielRuf avatar Apr 01 '19 20:04 DanielRuf

You can see the unminified code at https://github.com/Dogfalo/materialize/blob/v0.100.2/js/forms.js

DanielRuf avatar Apr 01 '19 20:04 DanielRuf

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()))

NavidZ avatar Apr 02 '19 19:04 NavidZ

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.

kcahir avatar Apr 02 '19 20:04 kcahir

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.

DanielRuf avatar Apr 02 '19 21:04 DanielRuf

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

kcahir avatar Apr 02 '19 23:04 kcahir

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 avatar Apr 03 '19 04:04 DanielRuf

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.

DanielRuf avatar Apr 03 '19 04:04 DanielRuf

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 https://github.com/Dogfalo/materialize/issues/6322#issuecomment-479157272 ? (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

j4kim avatar Apr 03 '19 16:04 j4kim

https://github.com/amsul/pickadate.js/pull/1145#issuecomment-479532964 might give an idea.

DanielRuf avatar Apr 03 '19 16:04 DanielRuf

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.

NavidZ avatar Apr 03 '19 16:04 NavidZ

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

j4kim avatar Apr 03 '19 17:04 j4kim

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

DanielRuf avatar Apr 03 '19 17:04 DanielRuf

They recently changed their behavior as well

I saw no such issue in latest Firefox.

DanielRuf avatar Apr 03 '19 17:04 DanielRuf

See https://github.com/Dogfalo/materialize/issues/6322#issuecomment-478711638

smaug---- avatar Apr 03 '19 19:04 smaug----

I've made a Materialize 0.100.2 build that integrates the modifications on pickadate from https://github.com/amsul/pickadate.js/pull/1145 (ad60ed50be6595b79d1d0b80f42f535c7138fb8e) and the trick from https://github.com/Dogfalo/materialize/issues/6322#issuecomment-479157272 (36099c60e77f33d3a6c97579ebedbd716beed12b)

-> 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...

j4kim avatar Apr 04 '19 14:04 j4kim

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?

keshav0016 avatar Apr 10 '19 04:04 keshav0016

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.

DanielRuf avatar Apr 10 '19 05:04 DanielRuf

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?

keshav0016 avatar Apr 10 '19 05:04 keshav0016

@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)

j4kim avatar Apr 11 '19 12:04 j4kim

@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?

keshav0016 avatar Apr 11 '19 12:04 keshav0016

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

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

DanielRuf avatar Apr 11 '19 13:04 DanielRuf

Maybe ask for a volunteer for 0.x maintenance?

ray007 avatar Apr 11 '19 14:04 ray007

Maybe ask for a volunteer for 0.x maintenance?

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

DanielRuf avatar Apr 11 '19 15:04 DanielRuf

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

LucasPenido avatar May 09 '19 11:05 LucasPenido

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?

Richar-p avatar Dec 07 '20 17:12 Richar-p