Closed Bug 908100 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: Layout, defect)

x86_64
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla28
blocking-b2g koi+
Tracking Status
firefox26 --- wontfix
firefox27 --- wontfix
firefox28 --- fixed
b2g-v1.2 --- fixed

People

(Reporter: salva, Assigned: tnikkel)

References

Details

Attachments

(2 files, 1 obsolete file)

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.
blocking-b2g: --- → koi?
Depends on: 903401
Blocks: 903401
No longer depends on: 903401
Salva,

User impact?
Flags: needinfo?(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.
Flags: needinfo?(salva)
and would you be the right assignee here ?
Flags: needinfo?(sespinoza)
Flags: needinfo?(sespinoza) → needinfo?(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.
Flags: needinfo?(salva)
(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).
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.
Comms Triage - This issue blocks a blocker (Bug 903401) so marking it as koi+
blocking-b2g: koi? → koi+
Andrew, do you know who would be the best to look at this? thanks
Flags: needinfo?(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.
Flags: needinfo?(overholt) → needinfo?(dbaron)
Jet, is there anyone on the layout team able to take a look here?
Component: General → Layout
Flags: needinfo?(bugs)
Product: Firefox OS → Core
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?
Flags: needinfo?(tnikkel)
Flags: needinfo?(dbaron)
Flags: needinfo?(bugs)
OS: Linux → Gonk (Firefox OS)
Does taking the contents of the app and just viewing it as a regular webpage in the browser exhibit the problem?
Flags: needinfo?(salva)
Attached file active testcase (obsolete) —
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.
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.
Flags: needinfo?(salva)
(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?
(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.
(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.
No longer blocks: 903401
Duplicate of this bug: 903401
Setting owner to Timothy.
Assignee: nobody → tnikkel
Timothy confirmed this particular case is not a regression.
Keywords: regression
(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 14 is about a similar, but different issue. I thought it was related (or the same) but it turns out it is not.
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.
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.
Flags: needinfo?(tnikkel)
Attached patch patchSplinter Review
Not sure who knows this code, tagging a couple people that might know, could you please direct review for this patch to someone suitable?
Flags: needinfo?(bugmail.mozilla)
Flags: needinfo?(21)
(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.
Attachment #826136 - Attachment is obsolete: true
Vivien should be able to review this :)
Flags: needinfo?(bugmail.mozilla)
Flags: needinfo?(21)
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.
Attachment #8334022 - Flags: review?(21) → review+
https://hg.mozilla.org/mozilla-central/rev/718017bd6265
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Verified on v1.2 11/21 build:
Gecko-ccc4930
Gaia-e54a2d4
Cannot reproduce what happened on bug 903401 (dup of this one)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.