1005733 - [email/UI] Cards's click-eating logic calls stopPropagation() but not preventDefault() so if we eat a click on a <button> inside a <form> the app will reload

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::E-Mail, defect)

Description

[Andrew Sutherland [:asuth] (he/him)]

While looking into bug 973038 and sanity checking our click suppression, I noticed the app was reloading itself when we were eating a click on the "next" button on setup_account_info.

The short story is that (and this is no surprise), if we're using a <button> and we stopPropagation() on it but don't preventDefault() and it lives inside a <form> and has no "type" specified (and thereby has a default value of "submit").

There are really two core hygiene things we need to be doing:
1) We need to be calling preventDefault() if we're trying to gobble an event so it's as if it never happened.  Besides buttons there are also focus/blur things that can happen if we fail to do this.
2) We need to be setting type="button" on all of our <button>s.

I think in general we haven't seen tons of problems from this because:
- Much of our markup has been wacky and not actually using <button>s.
- Much of our markup has been wacky and not using <form>s.
- Not many people are multi-clicking buttons.

I think we should just address point '1' on this bug.

In terms of known direct regressions, bug 805501 moved the button inside the form and it is trunk-only.  So I'm not sure this actually needs to be uplifted anywhere.

Patch imminent; need bug #.

Comment 1

[Andrew Sutherland [:asuth] (he/him)]

Attachment: preventDefault

Comment 2

[Andrew Sutherland [:asuth] (he/him)]

I should note that because we're only preventing click and not also mousedown/mouseup, buttons will still appear to be interactive in the pathological case where we screw-up and are eating events forever.  In practice I'm not sure how much this matters; it might actually be desirable to not interfere with those events to avoid weirdness from a mouseup that's allowed but with a forbidden mousedown/etc.

Comment 3

[James Burke [:jrburke]]

Comment on attachment 8417130 [details] [review]
preventDefault

I definitely see a need for this fix, as I was able to reproduce the page reload without it. 

To also address bug 973038, which prompted this investigation, I looked more into it, and it seems like in the rapid click case, even with this pull request change, some transitionend events are dropped, as in, our transitionend listener is not called.

I tried a change that uses a timestamp for _eatingEventsUntilNextCard instead of a boolean, then in _onMaybeIntercept, if the timestamp is later than a threshold (I set to 1 second), then it is reset.

This seemed to fix bug 973038. I'm going to flip off the review for this pull request, and instead get your thoughts on the timestamp approach. I cherry-picked your change and added the timeout change here:

https://github.com/jrburke/gaia/compare/bug1005733-email-click-suppress-timeout

I'll ping you in irc/meeting about it too, but I can squash/rebase after confirming tests pass if this looks good.

Comment 4

[James Burke [:jrburke]]

After talking in realtime, we discussed how this is likely an issue with Cards.removeCardAndSuccessors being called too quickly in setup_progress, and that Cards.removeCardAndSuccessors should queue up actions to be done after the transition completes, which should maintain the internal state correctly.

Per discussion, I'm going to steal and work on that Cards plumbing fix.

Comment 5

[James Burke [:jrburke]]

Attachment: GitHub pull request

This pull request just restores :asuth's original changeset of just doing the preventDefault work, but applied to latest master -- cherry pick of the old one resulted in a gnarly diff.

I will track any Cards.removeCardAndSuccessors work in bug 973038, which was the appropriate thing to do anyway instead of clogging this specific bug with the changes for that bug.

Comment 6

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8450583 [details] [review]
GitHub pull request

Fight the default!

Comment 7

[James Burke [:jrburke]]

Merged in master:
https://github.com/mozilla-b2g/gaia/commit/f7e704210d73e16d1a372cc7946180efca7497ba

from pull request:
https://github.com/mozilla-b2g/gaia/pull/21369