Closed Bug 1351072 Opened 3 years ago Closed 3 years ago

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

Categories

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

53 Branch
All
Windows
defect

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox-esr45 --- unaffected
firefox52 --- unaffected
firefox-esr52 --- unaffected
firefox53 + wontfix
firefox54 + wontfix
firefox55 + fixed
firefox56 --- fixed

People

(Reporter: 684sigma, Assigned: ben.tian)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(3 files, 2 obsolete files)

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.
Has STR: --- → yes
Keywords: regression
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
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?
Flags: needinfo?(enndeakin)
Has Regression Range: --- → yes
Tracking as the regression starts in Beta 53.
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.
Flags: needinfo?(enndeakin)
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?
Tracking 53/54/55+. Getting late in the 53 cycle, we'll see what happens.
Any chance we can get a simple/safe fix for 53?
Flags: needinfo?(enndeakin)
I meant to needinfo Olli about comment 5.
Flags: needinfo?(enndeakin) → needinfo?(bugs)
The RC build started today, so too late for 53 at this point except for critical feature/crash/security issues with broad user impact.
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?
Flags: needinfo?(bugs)
But yes, sounds like we could perhaps not clear activeESM in this case.
You will need Windows to reproduce this bug.
OS: Unspecified → Windows
Hardware: Unspecified → All
Assignee: nobody → enndeakin
Status: NEW → ASSIGNED
Attached patch Possible patch (obsolete) — Splinter Review
Neil, are you still waiting for review or do you need any QE help?
Don't think a test is possible here.
Attachment #8859150 - Attachment is obsolete: true
This just needs someone to review it, but smaug isn't accepting reviews.
Smaug -- can you recommend someone to review this, please?
Flags: needinfo?(bugs)
That inDOMUtils patch? I don't know who is particularly familiar with that, but I guess I could review.
Flags: needinfo?(bugs)
Too late for 54 as 54 RC is released. Mark 54 won't fix.
See Also: → 1365959
Priority: -- → P2
Duplicate of this bug: 1365959
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?
Flags: needinfo?(htsai)
Priority: P2 → P1
(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. :) )
Assignee: enndeakin → btian
Flags: needinfo?(htsai)
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
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|?
Attachment #8877045 - Attachment is obsolete: true
> 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
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
(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 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
Attachment #8877420 - Flags: review?(bugs)
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 :)
Attachment #8877420 - Flags: review?(bugs) → review+
Attachment #8877420 - Attachment description: Bug 1351072 - [e10s] Clear active document when closing popup of <select> → Bug 1351072 - [e10s] Clear active document when closing popup of <select>, r=smaug
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6b5039beea5
[e10s] Clear active document when closing popup of <select>. r=smaug
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d6b5039beea5
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
As firefox55 is affected and this bug was tracked by RM, we probably want to uplift to beta55. Do we have the concerns?
Flags: needinfo?(btian)
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
Flags: needinfo?(btian)
Attachment #8877420 - Flags: approval-mozilla-beta?
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
Attachment #8877420 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Based on comment 34, this does not manual coverage. 
Updating the qe‑verify flag to reflect this.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.