Closed Bug 1129066 Opened 5 years ago Closed 5 years ago

[Window Management][Power Menu] Tapping the text of a Power Menu list item does not select the item.

Categories

(Core :: DOM: Events, defect)

38 Branch
ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla38
blocking-b2g 2.5?
Tracking Status
firefox36 --- wontfix
firefox37 --- fixed
firefox38 --- fixed
b2g-v2.2 --- verified
b2g-master --- verified

People

(Reporter: Marty, Assigned: kats)

References

()

Details

(Keywords: regression, Whiteboard: [3.0-Daily-Testing])

Attachments

(5 files)

Description:
Tapping on the text for 'Silence/Ring incoming calls,' 'Restart,' or 'Power off' will not activate that function.  The user must tap the blank space next to the text in that list item to activate the function.

Note: This issue does NOT occur when tapping the text of the first list item, 'Turn on/off airplane mode.'  Tapping that text has the correct functionality.

Repro Steps:
1) Update a Flame to 20150203055641
2) Long-Press the power button to bring up the Power Menu
3) Select 'Silence incoming calls' by tapping directly on the text.
4) Select 'Silence incoming calls' by tapping in the blank space next to the text.

Actual:
Tapping on the text does not select the menu item, while tapping next to the text does select the item.

Expected:
The menu item is selected when tapping the text.

Environmental Variables:
Device: Flame 3.0 (Full Flash)(319MB)
Build ID: 20150203055641
Gaia: ae5a1580da948c3b9f93528146b007fc4f6a712b
Gecko: ae5d04409cd9
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 38.0a1 (3.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:38.0) Gecko/38.0 Firefox/38.0


Repro frequency: 7/8
See attached: video clip (URL), logcat

-------------------------------

This issue does NOT occur on Flame 2.2.
The menu item is selected when tapping the text.

Environmental Variables:
Device: Flame 2.2 (Full Flash)(319MB)
Build ID: 20150203002504
Gaia: cd62ff9fe199fb43920ba27bd5fdbc5c311016fc
Gecko: 11d93135c678
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 37.0a2 (2.2)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(pbylenga)
[Blocking Requested - why for this release]:
Functional regression of a core feature.

Requesting a window.
blocking-b2g: --- → 3.0?
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(pbylenga)
I can reproduce this bug in a Flame device with the same 3.0 build but using base image v188.

Repro frequency: 5/5
QA Contact: jmercado
Bug 950934 seems to have caused this issue.

Mozilla-inbound Regression Window

Last Working 
Environmental Variables:
Device: Flame 3.0
BuildID: 20150201170935
Gaia: 740c7c2330d08eb9298597e0455f53d4619bbc1a
Gecko: 231a8c61b49f
Version: 38.0a1 (3.0) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:38.0) Gecko/38.0 Firefox/38.0

First Broken 
Environmental Variables:
Device: Flame 3.0
BuildID: 20150201174135
Gaia: 740c7c2330d08eb9298597e0455f53d4619bbc1a
Gecko: bcefc7d8d885
Version: 38.0a1 (3.0) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:38.0) Gecko/38.0 Firefox/38.0

Last Working gaia / First Broken gecko - Issue DOES occur
Gaia: 740c7c2330d08eb9298597e0455f53d4619bbc1a
Gecko: bcefc7d8d885

First Broken gaia / Last Working gecko - Issue does NOT occur 
Gaia: 740c7c2330d08eb9298597e0455f53d4619bbc1a
Gecko: 231a8c61b49f

Gecko Pushlog: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=231a8c61b49f&tochange=bcefc7d8d885
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Botond, can you take a look at this please. This might have been caused by the work done on bug 950934.
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker) → needinfo?(botond)
The logcat contains the error "TypeError: evt.target.dataset is undefined" when this happens. Inspecting in WebIDE indicates that evt.target is a text node rather than the <li> element containing the text, which is odd. Not sure why bug 950934 would have caused the click event to get generated with a bad target.
If I add a mousedown listener in sleep_menu.js it gets the <li> element as the target, so the bad target is specific to the click event.
Flags: needinfo?(botond)
After a bunch of tracing through code it seems like the mCurrentTarget pointer inside the EventStateManager gets changed while in the process of dispatching the mouse-up event, and so the click event ends up with a bad target. As best I can tell the attached code is what's fiddling with the mCurrentTarget.
Flags: needinfo?(bugs)
Here's another stack that messes with the ESM state. It's not clear to me why sometimes we hit one and sometimes we hit the other.
Even with both of these removed there's something screwing up. Falling down the rabbit hole...
Attached file Frame tree snippet
Ok, I think I figured out what's going on. For the mouse-up event, the "current frame" is aec4bed8 and code at [1] finds the correct containing element node to dispatch the event to. During processing of the mouse-up event, it looks like the frame is removed from the frame tree due to some restyling happening. I'm not sure of the details there, but the code at [2] runs and sets the mCurrentEventContent to aec12680 as expected. Then, the mouse-up's post-handler runs from [3]. Note that one of the arguments to this function is GetCurrentEventFrame(), which in this instance returns aec4c018 which is actually the :before text frame for the node. I have no idea why this happens and I asked on #layout but nobody has responded yet. Finally, when the click event is dispatched it calls GetEventTargetContent at [4] and dispatches to that. Unfortunately here the target content is computed from the :before text frame and the target content ends up being a #text node. There is no code that ensures we walk up to the containing element, and that's what seems to be the problem.

[1] http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp?rev=5af5c02018a2#7934
[2] http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp?rev=5af5c02018a2#2178
[3] http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp?rev=5af5c02018a2#8257
[4] http://mxr.mozilla.org/mozilla-central/source/dom/events/EventStateManager.cpp?rev=5af5c02018a2#4439
Assignee: nobody → bugmail.mozilla
Flags: needinfo?(bugs)
Attachment #8559333 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/4ceae3faa925
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Comment on attachment 8559333 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/regressing bug #]: unknown
[User impact if declined]: in some cases it is possible for click events to be dispatched to web content with a #text node as the target, in violation of specifications. this specifically causes a user-visible bug on B2G but technically affects all platforms which is why i'm requesting aurora approval instead of just b2g37. this bug has existed for a long time as far as I can tell, so I don't consider it important enough to request beta approval.
[Describe test coverage new/current, TreeHerder]: tested in the b2g scenario as described in this bug
[Risks and why]: fairly low risk; the code added is simple and will be a no-op except in the case that we care about. the code itself is a copy of code that already exists on the paths that other mouse events go through so in that respect it's well-tested
[String/UUID change made/needed]: none
Attachment #8559333 - Flags: approval-mozilla-aurora?
Comment on attachment 8559333 [details] [diff] [review]
Patch

Although I'm not aware of reports of issues on Aurora, I think we should take this fix for platform consistency. Aurora+
Attachment #8559333 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Component: Gaia::System::Window Mgmt → DOM: Events
Product: Firefox OS → Core
Target Milestone: --- → mozilla38
Version: unspecified → 38 Branch
This issue is verified fixed on 3.0 Flame Nightly.  The current Flame 2.2 Nightly does not have this yet.  Ni? myself to verify that tomorrow.

Results:  Tapping all parts of the item triggers the list item including the text.

Environmental Variables:
Device: Flame 3.0
BuildID: 20150211010216
Gaia: 8c7865486a1b11076b849bbf8f7fccbaffbfafe7
Gecko: ee093ca70666
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 38.0a1 (3.0) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:38.0) Gecko/38.0 Firefox/38.0
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Flags: needinfo?(jmercado)
Keywords: verifyme
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
This issue was not on 2.2 but the uplift did not seem to have any ill effects either.

Environmental Variables:
Device: Flame 2.2
BuildID: 20150212002504
Gaia: 791e53728cd8018f1d7cf7efe06bbeb1179f0370
Gecko: 5dec207fcbeb
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 37.0a2 (2.2) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0
Flags: needinfo?(jmercado)
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.