material-web icon indicating copy to clipboard operation
material-web copied to clipboard

fix(dialog): prevent close when click released in scrim

Open vdegenne opened this issue 2 years ago • 9 comments

This is an attempt to avoid the dialog to close when the mouse click was initiated inside the content but released from the scrim.

vdegenne avatar Aug 23 '23 20:08 vdegenne

Update on this, there's some screenshot test failures that need more investigating, so apologies for the delay

asyncliz avatar Sep 11 '23 16:09 asyncliz

Update on this, there's some screenshot test failures that need more investigating, so apologies for the delay

Can you give me more details? the tests run fine on my end.

vdegenne avatar Sep 12 '23 15:09 vdegenne

Update on this, there's some screenshot test failures that need more investigating, so apologies for the delay

Can you give me more details? the tests run fine on my end.

It's internal tests, so they don't run on github 😔

asyncliz avatar Sep 12 '23 17:09 asyncliz

I haven't had a chance to dig into it, but the failing test looks something like this: https://lit.dev/playground/#gist=62443e4343a872dc51982907ea6dfc9d

  <md-dialog open>
       <div slot="headline">Dialog</div>
        <div slot="content" style="height: 500px;">
          <md-filled-select menu-fixed>
            <md-select-option headline="Apple" value="apple"></md-select-option>
            <md-select-option headline="Apricot" value="apricot"></md-select-option>
          </md-filled-select>
        </div>
        <div slot="actions">
          <md-filled-button>Cancel</md-filled-button>
        </div>
  </md-dialog>

The failing test is that clicking the md-filled-select after this change doesn't open the select, when it expects the md-filled-select to open.

AndrewJakubowicz avatar Sep 13 '23 01:09 AndrewJakubowicz

I noticed quite a few quirks on the select element (not just in a dialog), maybe it's a good opportunity to polish this element instead of trying to fix one problem related to this specific case.

This is the first one I spotted: https://github.com/material-components/material-web/issues/4915

vdegenne avatar Sep 13 '23 13:09 vdegenne

the failing screenshot test seems to be weird. I can't nail down the exact issue but looks flaky when attempting to reproduce the issue with a debugger in a live environment in the test. Otherwise, normal interaction looks good. Gonna have to spend a bit of time to figure out what's going on here

e111077 avatar Sep 19 '23 00:09 e111077

I've got a failing case that's easy to reproduce:

  1. Put a checkbox in the dialog
  2. Toggle the checkbox with the Spacebar
  3. Watch it close the dialog

Not every click event has a preceding pointerdown event so the next click will fail the check. You should probably just handle both events.

austinw-fineart avatar Jun 27 '24 06:06 austinw-fineart

@austinw-fineart This looks like a different issue to me, plus I couldn't reproduce it. If you have a problem could you open another issue and join a playground to it? You can use this template: https://lit.dev/playground/#gist=db372518cfa9cd6097b7842e6da5f740 Thanks

vdegenne avatar Jun 30 '24 11:06 vdegenne

@vdegenne I'm afraid I do not know how to import this into playground. I ran the test case on this pull request.

austinw-fineart avatar Jul 01 '24 02:07 austinw-fineart