863702 - [B2G] :active state is sometime not rendered if you tap quickly

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Anthony Ricaud (:rik)]

When taping an item that has an :active state, if you do it really fast, the :active rules will not be rendered. Because of that, users can think that a tap was not registered and tap a second time or think the app is not reactive.

I think we should let :active be "active" for a certain amount of time (to be determined). If the user taps another element, we should switch the :active state immediately to that new element.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So the way I could imagine this working is that we'd extend :active applying after the mouseup until either:
 (a) a pref-controlled number of milliseconds since mouse*down* have elapsed, or
 (b) there's another mousedown

Comment 2

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

The patch at https://bug921824.bugzilla.mozilla.org/attachment.cgi?id=811675 will fix it. I don't really like it tbh because it spin the event loop and delay the mouse events sequence. And because there are delayed the :active class last longer.

Now, even if I don't like it it may worth it since it won't delayed touch events and will improve a lot the feeling of the device, both in terms of reliability and responsiveness.

Comment 3

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: delay.active.patch

Comment 5

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0f1efa209f53

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Not until it hits m-c :)

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Backed out for B2G mochitest-2 failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb0f1d1bd9ca

https://tbpl.mozilla.org/php/getParsedLog.php?id=28994591&tree=Mozilla-Inbound

Comment 8

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #6)
> Not until it hits m-c :)

Oups! sorry about that. And followed by a backout!

Comment 9

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: bug863702.patch

Let's not break clicks...

Comment 10

[[:fabrice] Fabrice Desré]

Comment on attachment 816655 [details] [diff] [review]
bug863702.patch

Review of attachment 816655 [details] [diff] [review]:
-----------------------------------------------------------------

Can you send this one to try? Meanwhile I'll try to thing about a less ugly solution.

::: dom/browser-element/BrowserElementPanning.js
@@ +78,5 @@
>        return;
>      }
>  
> +    var start = Date.now();
> +    var thread = Services.tm.currentThread;

s/var/let

@@ +82,5 @@
> +    var thread = Services.tm.currentThread;
> +    while (this._block && (Date.now() - start) < this._activeDurationMs) {
> +      thread.processNextEvent(true);
> +    }
> +    this._block = false;

'_block' is a bit too generic. And oh, the evil of spinning the event loop :(

@@ +240,5 @@
> +      this.notify(this._activationTimer);
> +
> +      this._block = true;
> +      var start = Date.now();
> +      var thread = Services.tm.currentThread;

here too

Comment 11

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Fabrice Desré [:fabrice] from comment #10)
> Comment on attachment 816655 [details] [diff] [review]
> bug863702.patch
> 
> Review of attachment 816655 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Can you send this one to try? 

Sure I don't want to break events again :)

> Meanwhile I'll try to thing about a less ugly
> solution.
>

If you can I will be more than happy. But tbh I think this kind of code is why people have learned to use touch events to be fast on mobile devices. I bet that other vendors are doing something similar in the background.

Comment 12

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

https://tbpl.mozilla.org/?tree=Try&rev=67dbaf45fbac

Comment 13

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #12)
> https://tbpl.mozilla.org/?tree=Try&rev=67dbaf45fbac

Seems green.

Comment 14

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: bug863702.patch

Comment 15

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/afb6450f58cd

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/afb6450f58cd