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

site: fix firefox bug #1811

Merged
merged 1 commit into from
Aug 26, 2022
Merged

site: fix firefox bug #1811

merged 1 commit into from
Aug 26, 2022

Conversation

ukane-philemon
Copy link
Contributor

@ukane-philemon ukane-philemon commented Aug 23, 2022

I investigated the issue resulting in #1808 and found out firefox and chrome behave differently on mousedown events.
I think the issue lies in the fact that Firefox triggers 3 events (onmousedown, onmousedown, onchange) instead of 2 (onmousedown, onchange) like Chrome does. The last onmousedown will have the screen coordination value of the event set to zero(cause the last mouse event is actually right in the Element?). Closes #1808.

Reference 1: w3c uievents #201

@chappjc
Copy link
Member

chappjc commented Aug 23, 2022

This only happens on master, not release-v0.5, so it seems clear it's the recent wallet page restyling, but I don't understand why. There doesn't seem to be any difference with mouseInElement or the mousedown binding in wallets.ts.

@chappjc chappjc added this to the 0.5.1 milestone Aug 24, 2022
@chappjc chappjc requested a review from buck54321 August 24, 2022 16:55
@chappjc
Copy link
Member

chappjc commented Aug 24, 2022

I'm going to investigate too, but @buck54321 your input is probably needed to discern why this is a problem on master but not release-v0.5, or if this is the best fix.

@buck54321
Copy link
Member

I think the issue lies in the fact that Firefox triggers 3 events (onmousedown, onmousedown, onchange) instead of 2 (onmousedown, onchange) like Chrome does.

It's actually because firefox fires a mousedown event for the click on the <option> element, and chrome doesn't. Could you instead do something like

static mouseInElement (e: MouseEvent, el: HTMLElement): boolean {
  if (e.target && (e.target as HTMLElement).tagName === 'OPTION') return true
  ...

so it seems clear it's the recent wallet page restyling, but I don't understand why

Before re-styling, I don't think we were hiding the form when you clicked outside. It wasn't a modal dialog.

@ukane-philemon
Copy link
Contributor Author

It's actually because firefox fires a mousedown event for the click on the element,

So I guess this behaviour only affects the <option> element? Firefox fires one mousedown event for the click on the element, and another for select.

@buck54321
Copy link
Member

In my tests, it was the option element that had the zeroed coordinates. Please do verify.

@ukane-philemon
Copy link
Contributor Author

ukane-philemon commented Aug 24, 2022

In my tests, it was the option element that had the zeroed coordinates. Please do verify.

Yes, I can verify. That was why I added this line. @buck54321, can we have this check with your suggestion? zeroed coordinates could come from somewhere else.

|| (e.pageX === 0 && e.pageY === 0)

Comment on lines 112 to 116
static mouseInElement (e: MouseEvent, el: HTMLElement): boolean {
if (e.target && (e.target as HTMLElement).tagName === 'OPTION') return true
const rect = el.getBoundingClientRect()
return e.pageX >= rect.left && e.pageX <= rect.right &&
e.pageY >= rect.top && e.pageY <= rect.bottom
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like we're overthinking this.

  static mouseInElement (e: MouseEvent, el: HTMLElement): boolean {
    return el.contains(e.target as Node)
  }

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice find. That should work, with a couple of caveats.

  1. You probably need to check if the target is the element itself, otherwise that click padding would become ineffective. Something like
return el === e.target || el.contains(e.target as Node)
  1. This would fail if we had an element that's not a descendant absolutely positioned over the form. I'm not sure that we do that with anything but tooltips, so it probably won't matter to us.

Copy link
Member

@chappjc chappjc Aug 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. You probably need to check if the target is the element itself, otherwise that click padding would become ineffective. Something like

https://developer.mozilla.org/en-US/docs/Web/API/Node/contains

contains covers self, apparently:

The contains() method of the Node interface returns a boolean value indicating whether a node is a descendant of a given node, that is the node itself, one of its direct children (childNodes), one of the children's direct children, and so on.

Note: A node is contained inside itself.

It also doesn't seem to be related to position, just DOM hierarchy.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

contains covers self, apparently

yeah, your suggestion is efficient.

Copy link
Member

@JoeGruffins JoeGruffins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can confirm the bug and this fixes it.

@chappjc chappjc merged commit a891a77 into decred:master Aug 26, 2022
@chappjc chappjc modified the milestones: 0.5.1, 0.6/1.0 Aug 26, 2022
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

Successfully merging this pull request may close these issues.

site: change wallet type settings, close form on firefox
4 participants