1351072 - [e10s] (middle)click outside <select> drop-down list doesn't work if it's in iframe

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect, P1)

Description

[684sigma]

Attachment: Click outside select doesn't work if it's in iframe, by reporter.html

I have a problem with Firefox Beta 53. It also happens in Nightly 55. It doesn't happen in Firefox ESR 45, Beta 52.
Sometimes click outside <select> drop-down doesn't work. Here's how to reproduce it:

1. Open attached document
2. Click on select, middle-click on the link. Repeat at least twice
3. Click on select, click on the link.

Result: Сlick and middleclick outside <select> drop-down doesn't work
Expected: Click and middleclick should work

I've added "[e10s]" in the summary, because it isn't reproducible if in Nightlly I disable multiprocess.
Please correct me if this logic is incorrect.

Comment 1

[Jan Sonntag]

Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:55.0) Gecko/20100101 Firefox/55.0
BuildID: 20170327030203

I can confirm this bug in the current Nightly.
Clicking on the link outside the <select> element with e10s enabled doesn't work properly after performing clicks on <select> - link and <select> again.
After this procedure, the link should open on the first click outside of the element however it only reacts to the second click. With e10s disabled this behavior does not occur.

Comment 2

[Jan Sonntag]

Generated by mozregression-gui
app_name: firefox
build_date: 2017-01-20 01:29:58.740000
build_file: C:\Users\Jan Sonntag\.mozilla\mozregression\persist\bde3fc40b9b5--mozilla-central--firefox-53.0a1.en-US.win64.zip
build_type: inbound
build_url: https://queue.taskcluster.net/v1/task/Z4CBqfRhQty2yNiK8wB4gw/runs/0/artifacts/public%2Fbuild%2Ffirefox-53.0a1.en-US.win64.zip
changeset: bde3fc40b9b55435d989393d659a60f46b54fd72
pushlog_url: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=96cb95af530477edb66ae48d98c18533476e57bb&tochange=bde3fc40b9b55435d989393d659a60f46b54fd72
repo_name: mozilla-central
repo_url: https://hg.mozilla.org/mozilla-central
task_id: Z4CBqfRhQty2yNiK8wB4gw

I found a regression range for this issue.
Could possibly be related to the work on the select element in Bug 1321472 or Bug 1311279?

Comment 3

[Loic]

Tracking as the regression starts in Beta 53.

Comment 4

[Neil Deakin]

I think the issue here is that the active state is in the child process cleared when the mouse is released, but nsEventStateManager::sActiveESM is not.

Comment 5

[Neil Deakin]

Yes, a mouseup message is being sent the child process and it calls nsIDOMUtils::RemoveContentState to remove the active state. It seems like it could take an argument whether to clear the activeESM as well; I suspect we don't want to clear it every time RemoveContentState is called.

Does this seem reasonable?

Comment 6

[Marcia Knous [:marcia]]

Tracking 53/54/55+. Getting late in the 53 cycle, we'll see what happens.

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

Any chance we can get a simple/safe fix for 53?

Comment 8

[Neil Deakin]

I meant to needinfo Olli about comment 5.

Comment 9

[Liz Henry (:lizzard) (relman/hg->git project)]

The RC build started today, so too late for 53 at this point except for critical feature/crash/security issues with broad user impact.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

I don't see difference in e10s vs non-e10s window.
"Result: Сlick and middleclick outside <select> drop-down doesn't work
Expected: Click and middleclick should work"

What does "doesn't work" or "work" mean here? What should happen?

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

But yes, sounds like we could perhaps not clear activeESM in this case.

Comment 12

[Neil Deakin]

You will need Windows to reproduce this bug.

Comment 13

[Neil Deakin]

Attachment: Possible patch

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

Neil, are you still waiting for review or do you need any QE help?

Comment 15

[Neil Deakin]

Attachment: Clear active state when closing popup

Don't think a test is possible here.

Comment 16

[Neil Deakin]

This just needs someone to review it, but smaug isn't accepting reviews.

Comment 17

[David Durst [:ddurst]]

Smaug -- can you recommend someone to review this, please?

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

That inDOMUtils patch? I don't know who is particularly familiar with that, but I guess I could review.

Comment 19

[Gerry Chang [:gchang]]

Too late for 54 as 54 RC is released. Mark 54 won't fix.

Comment 21

[Astley Chen (inactive)]

When applying the patch from comment 15, the link is clickable and cursor shows correctly.
However, I also found a case that the cursor was incorrectly rendered.

STR:
1. Apply the patch from comment 15.
2. Click the dropdown and keep it shown up.
3. Move cursor to the link(Bugzilla link).
4. It shows a clickable-style cursor when mouse is over the link.

HsinYi, as Neil won't be available until 8/9, are you able to assign someone to follow up his patch?

Comment 22

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Astley Chen [:astley] (UTC+8) from comment #21)
> When applying the patch from comment 15, the link is clickable and cursor
> shows correctly.
> However, I also found a case that the cursor was incorrectly rendered.
> 
> STR:
> 1. Apply the patch from comment 15.
> 2. Click the dropdown and keep it shown up.
> 3. Move cursor to the link(Bugzilla link).
> 4. It shows a clickable-style cursor when mouse is over the link.
> 
> HsinYi, as Neil won't be available until 8/9, are you able to assign someone
> to follow up his patch?

Yes, happy to my team to help, assigning to Ben (@Neil, I hope you don't mind. :) )

Comment 23

[Ben Tian (inactive)]

Attachment: WIP - Clear active ESM when closing popup

Revise based on comment 15 patch:
- clear active ESM only when dismissing popup, no matter selected option changes or not.

Verified with examples in comment 0, bug 1365959 comment 3, and bug 1314647 comment 0. Need further logic check and verification.

Also opt to make "clear active ESM" a method, independent from remove content state.

===

Other problems spotted, need to double check.
- mouseup event doesn't fire when first time open popup. Hence active state remains.
- mousedown/up, and click event don't fire when option remain unchanged. E.g, open popup and click on original option

Comment 24

[Ben Tian (inactive)]

Attachment: Bug 1351072 - [e10s] Clear active document when closing popup of <select>, r=smaug

Attach patch that clears active document when closing popup of <select>. Will investigate problem of no mouseup event when first time open popup (comment 23).

Followup checks:
- remove|DOMUtils.removeContentState| from "Forms:MouseUp"?
- move |DOMUtils.removeContentState| outside if, right before |uninit|?
- move |DOMUtils.removeContentState| into |uninit|?

Comment 25

[Ben Tian (inactive)]

> Attach patch that clears active document when closing popup of <select>.
> Will investigate problem of no mouseup event when first time open popup
> (comment 23).

In child process, when popup is opened |EventStateManager::PostHandleEvent| of eMouseDown sets activeESM [1] and clickable-style cursor wouldn't show; however when popup is opened __at_first_time__, |EventStateManager::PostHandleEvent| of eMouseUp clears activeESM [2] after [1], and the clickable-style cursor would appear accordingly.

[1] http://searchfox.org/mozilla-central/rev/d67ef71097da4d1aa344c9d9c672e49a7228e765/dom/events/EventStateManager.cpp#3127
[2] http://searchfox.org/mozilla-central/rev/d67ef71097da4d1aa344c9d9c672e49a7228e765/dom/events/EventStateManager.cpp#3150

Comment 26

[Ben Tian (inactive)]

Only see the problem with debug build on Mac, neither with non-debug build on Mac nor debug build on Win 10/Linux. Also it only occurs at first time opening popup.

Lower the priority.

(In reply to Ben Tian [:btian] from comment #25)
> > Attach patch that clears active document when closing popup of <select>.
> > Will investigate problem of no mouseup event when first time open popup
> > (comment 23).
> 
> In child process, when popup is opened |EventStateManager::PostHandleEvent|
> of eMouseDown sets activeESM [1] and clickable-style cursor wouldn't show;
> however when popup is opened __at_first_time__,
> |EventStateManager::PostHandleEvent| of eMouseUp clears activeESM [2] after
> [1], and the clickable-style cursor would appear accordingly.
> 
> [1]
> http://searchfox.org/mozilla-central/rev/
> d67ef71097da4d1aa344c9d9c672e49a7228e765/dom/events/EventStateManager.
> cpp#3127
> [2]
> http://searchfox.org/mozilla-central/rev/
> d67ef71097da4d1aa344c9d9c672e49a7228e765/dom/events/EventStateManager.
> cpp#3150

Comment 27

[Ben Tian (inactive)]

(In reply to Ben Tian [:btian] from comment #26)
> Only see the problem with debug build on Mac, neither with non-debug build
> on Mac nor debug build on Win 10/Linux. Also it only occurs at first time
> opening popup.
> 
> (In reply to Ben Tian [:btian] from comment #25)
> > > Will investigate problem of no mouseup event when first time open popup
> > > (comment 23).

Correction: on Win 10, the clickable cursor ALWAYS shows no matter popup is opened or not. The behavior is the same as Chrome on Win 10.

Comment 28

[Ben Tian (inactive)]

Comment on attachment 8877420 [details] [diff] [review]
Bug 1351072 - [e10s] Clear active document when closing popup of <select>, r=smaug

Olli, can you review this patch that clears activeESM when closing popup of <select>?

This patch, based on Neil's patch in comment 15, clears activeESM no matter 1) selected option changes or not and 2) popup is closed by keyboard or mouse. Note if mouseup clears activeESM, comment 21 problem would occur.

Verified with examples in comment 0, bug 1365959 comment 3, and bug 1314647 comment 0. Try result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=44ed4ea887fef098474f465737055ec0076ea1ec

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8877420 [details] [diff] [review]
Bug 1351072 - [e10s] Clear active document when closing popup of <select>, r=smaug

I guess this is reasonable :)

Comment 30

[Ben Tian (inactive)]

Try result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=44ed4ea887fef098474f465737055ec0076ea1ec

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6b5039beea5
[e10s] Clear active document when closing popup of <select>. r=smaug

Comment 32

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 33

[Astley Chen (inactive)]

As firefox55 is affected and this bug was tracked by RM, we probably want to uplift to beta55. Do we have the concerns?

Comment 34

[Ben Tian (inactive)]

Comment on attachment 8877420 [details] [diff] [review]
Bug 1351072 - [e10s] Clear active document when closing popup of <select>, r=smaug

(In reply to Astley Chen [:astley] (UTC+8) from comment #33)
> As firefox55 is affected and this bug was tracked by RM, we probably want to
> uplift to beta55. Do we have the concerns?

No concern. Request uplift as following.

=====
Approval Request Comment

[Feature/Bug causing the regression]:
  e10s <select> dropdown list
[User impact if declined]:
  1) after changing <select> option, link outside <select> element reacts to second click only
  2) changing <select> option in iframe makes content in outer frame react to second click only
[Is this code covered by automated tests?]:
  No
[Has the fix been verified in Nightly?]:
  Verified with examples listed in comment 28
[Needs manual test from QE? If yes, steps to reproduce]:
  No
[List of other uplifts needed for the feature/fix]:
  None
[Is the change risky?]:
  No
[Why is the change risky/not risky?]:
  The change clears activeESM only when closing popup, required to fix this bug without affecting other <select> actions.
[String changes made/needed]:
  None

Comment 35

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 8877420 [details] [diff] [review]
Bug 1351072 - [e10s] Clear active document when closing popup of <select>, r=smaug

let's try this in 55.0b4

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/54fe4bc8e007

Comment 37

[Cornel Ionce [:noni] [Hubs QA]]

Based on comment 34, this does not manual coverage. 
Updating the qe‑verify flag to reflect this.