908100 - Buttons remains in :active state depending on containers' CSS position attribute.

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Salvador de la Puente González [:salva]]

Attachment: Test application to check how buttons remains in :active state under certain circumstances depending on the container CSS position property.

STR:

0- Install the attached test application
1- Open Test Active Buttons application
2- Tap on a button, keep pressing and slide the finger without releasing until leaving the button.
3- Check how the button is in a blue state (active)
4- Release the finger.

Expected:
The button returns to its normal state.

Actual:
The button remains in active mode.

Now if you edit the app.css file and change

position: absolute;

by

position: fixed;

in the .wrapper selection and repeat the steps above, you will see this does not reproduce.

Comment 1

[Preeti Raghunath(:Preeti)]

Salva,

User impact?

Comment 2

[Salvador de la Puente González [:salva]]

I think the impact is high. To cancel an operation before it happens is very important as it gives the user a chance to change his mind. We allow this by counting the tap only when the user leaves the button. To not return the state of the button to the default state can cause a misleading effect making the user curious about why the button remains active and inducing a new and definitive tap.

Furthermore, this is a standard violation because the state of the button should return to normal with no dependence of the containers' position properties.

Comment 3

[bhavana bajaj [:bajaj]]

Is this blocking https://bugzilla.mozilla.org/show_bug.cgi?id=908100 ?

Comment 4

[bhavana bajaj [:bajaj]]

and would you be the right assignee here ?

Comment 5

[Salvador de la Puente González [:salva]]

(In reply to bhavana bajaj [:bajaj] from comment #3)
> Is this blocking https://bugzilla.mozilla.org/show_bug.cgi?id=908100 ?

I suppose you are referring to bug 903401. Yes it is blocking it. There is a workaround but the performance decreases a lot so it is not acceptable.

Comment 6

[bhavana bajaj [:bajaj]]

(In reply to Salvador de la Puente González [:salva] from comment #5)
> (In reply to bhavana bajaj [:bajaj] from comment #3)
> > Is this blocking https://bugzilla.mozilla.org/show_bug.cgi?id=908100 ?
> 
> I suppose you are referring to bug 903401.
Yes, sorry for the confusion

> Yes it is blocking it. There is a
> workaround but the performance decreases a lot so it is not acceptable.

So would you be the right assignee here and what is the LOE and by when do you think this can be completed ?(Need to assign a 1.2 Target milestone here, so the info you give will be very helpful).

Comment 7

[Salvador de la Puente González [:salva]]

I think I'm not the one for this bug. It seems a platform bug, nothing to do with Gaia. I could research on Gecko but I'm in other matters.

Comment 8

[Noemí Freire (:noemi)]

Comms Triage - This issue blocks a blocker (Bug 903401) so marking it as koi+

Comment 9

[Joe Cheng [:jcheng] (please needinfo)]

Andrew, do you know who would be the best to look at this? thanks

Comment 10

[Andrew Overholt [:overholt]]

David, can you help here?  If not, can you think of someone that should?  FWIW, I've been told we're aiming for koi+ bugs to be fixed by the end of October.

Comment 11

[Andrew Overholt [:overholt]]

Jet, is there anyone on the layout team able to take a look here?

Comment 12

[Milan Sreckovic [:milan] (needinfo for best results)]

Timothy, could you take a look at this and let us know if it's something you can sort out or if we need to keep looking?

Comment 13

[Timothy Nikkel (:tnikkel)]

Does taking the contents of the app and just viewing it as a regular webpage in the browser exhibit the problem?

Comment 14

[Timothy Nikkel (:tnikkel)]

Attachment: active testcase

It seems like active changes in general are broken in v1.2. This test case does nothing on unagi with v1.2. It works as expected on unagi with v1.1 and hamachi with v1.1 though.

Comment 15

[Salvador de la Puente González [:salva]]

This is working in the latest Nightly. I discover if I click in the X button and leave the area without releasing the mouse then it remains in active state as the browser thinks it is a drag and drop situation but then I add `draggable="false"` and everything is OK. In the other hand, the button case does not give any problem.

Comment 16

[Andrew Overholt [:overholt]]

(In reply to Salvador de la Puente González [:salva] from comment #15)
> This is working in the latest Nightly.

So something landed between the 26 branch point and now that fixes this?

Comment 17

[Salvador de la Puente González [:salva]]

(In reply to Andrew Overholt [:overholt] from comment #16)
> (In reply to Salvador de la Puente González [:salva] from comment #15)
> > This is working in the latest Nightly.
> 
> So something landed between the 26 branch point and now that fixes this?

I'm referring to the browser version. Not Gecko nor Gaia.

Comment 18

[Andrew Overholt [:overholt]]

(In reply to Salvador de la Puente González [:salva] from comment #17)
> (In reply to Andrew Overholt [:overholt] from comment #16)
> > (In reply to Salvador de la Puente González [:salva] from comment #15)
> > > This is working in the latest Nightly.
> > 
> > So something landed between the 26 branch point and now that fixes this?
> 
> I'm referring to the browser version. Not Gecko nor Gaia.

I don't think the layout code is different on B2G so I was hoping this would imply something could be backported for B2G 1.2.

Comment 20

[Andrew Overholt [:overholt]]

Setting owner to Timothy.

Comment 21

[Milan Sreckovic [:milan] (needinfo for best results)]

Timothy confirmed this particular case is not a regression.

Comment 22

[Jason Smith [:jsmith]]

(In reply to Milan Sreckovic [:milan] from comment #21)
> Timothy confirmed this particular case is not a regression.

comment 14 though implies the opposite?

Comment 23

[Timothy Nikkel (:tnikkel)]

comment 14 is about a similar, but different issue. I thought it was related (or the same) but it turns out it is not.

Comment 24

[Timothy Nikkel (:tnikkel)]

Seems to be some bug with our touch event handling. We fail to remove the active state from the button, so we are rendering correctly. Continuing to dig.

Comment 25

[Timothy Nikkel (:tnikkel)]

The bug seems to be in dom/browser-element/BrowserElementPanning.js

When we receive a touchstart event we set an activation timer, when it fires it sets pointerDownTarget as active. pointerDownTarget is the element under the first touch point. So the button in this case. Then when the touch point has moved far enough we decide that a pan is starting in the touchmove handler, and we call resetActive. resetActive calls setActive on the root element of whatever document this.target is in. This clears the active state from any descendant of the root element in that document. So if this.target is in the same document as pointerDownTarget this would clear the active state of pointerDownTarget. this.target is set to the nearest ancestor of pointerDownTarget that is "scrollable" as defined by findPannable. In this case the nearest scrollable is in the parent document, so this.target is in the parent doc, and hence pointerDownTarget's active state doesn't get cleared.

Since we are setting pointerDownTarget active I think it makes most sense to also use it when resetting active. Patch coming up.

Comment 26

[Timothy Nikkel (:tnikkel)]

Attachment: patch

Not sure who knows this code, tagging a couple people that might know, could you please direct review for this patch to someone suitable?

Comment 27

[Salvador de la Puente González [:salva]]

(In reply to Timothy Nikkel (:tn) from comment #25)
> The bug seems to be in dom/browser-element/BrowserElementPanning.js
> 
> When we receive a touchstart event we set an activation timer, when it fires
> it sets pointerDownTarget as active. pointerDownTarget is the element under
> the first touch point. So the button in this case. Then when the touch point
> has moved far enough we decide that a pan is starting in the touchmove
> handler, and we call resetActive. resetActive calls setActive on the root
> element of whatever document this.target is in. This clears the active state
> from any descendant of the root element in that document. So if this.target
> is in the same document as pointerDownTarget this would clear the active
> state of pointerDownTarget. this.target is set to the nearest ancestor of
> pointerDownTarget that is "scrollable" as defined by findPannable. In this
> case the nearest scrollable is in the parent document, so this.target is in
> the parent doc, and hence pointerDownTarget's active state doesn't get
> cleared.

What a subtle bug. Thank you very much for the clarification.

Comment 28

[Kartikaya Gupta (email:[e-mail])]

Vivien should be able to review this :)

Comment 29

[Timothy Nikkel (:tnikkel)]

https://tbpl.mozilla.org/?tree=Try&rev=0c05e7be8991

Comment 30

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

Comment on attachment 8334022 [details] [diff] [review]
patch

Probably my bad. I can't remember why we were doing that in that order, probably just a mistake.

Comment 31

[Timothy Nikkel (:tnikkel)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/718017bd6265

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/718017bd6265

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/a4315ce4cbd5

Comment 34

[isabelrios]

Verified on v1.2 11/21 build:
Gecko-ccc4930
Gaia-e54a2d4
Cannot reproduce what happened on bug 903401 (dup of this one)