Loic Reporter
	Description • 8 years ago

Follow-up of bug 1247209.

STR:
0) Enable e10s
1) Open http://otto.yndenz.com/
2) Hover the menu entry "Occasions"
3) Click on the <select> "Merk" and hover some items of its dropdown list

Result: the menu "Occasions" disappears.

The bug has surfaced after bug 1083365 landed:
https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=2e1d5dc5393f8a273efc48523fc81eaac3b16ef8&tochange=b4fd9967aa854ad41952f324b196e16a52283eb5

Works fine with e10s disabled.

	Vasilica Tamas, QA [:vtamas]
	Comment 1 • 8 years ago

I’ve also managed to reproduce this issue on Firefox 47.0a1 (2016-02-11), Firefox 46.0a2 (2016-02-11) and Firefox 45 Beta 3 (with e10s enabled) under Ubuntu 12.04 64-bit.
I am marking Firefox 44 as unaffected since e10s cannot be enabled in Firefox 44 RC.

	Loic Reporter
	Comment 2 • 8 years ago

It's an issue about <select> inside double <ul> that doesn't keep the focus on the parent <ul> when highlighting entries of the dropdown list.

	Jim Mathies [:jimm]
	Comment 3 • 8 years ago

clearing this to get it into triage.

	Jim Mathies [:jimm]
	Comment 4 • 8 years ago

Mike, would you consider this a serious webcompat issue with e10s? We're trying to figure out if this should block.

	Mike Taylor [:miketaylr]
	Comment 5 • 8 years ago

Hmmm. Loic, can you try to reproduce again?

I can't reproduce in Ubuntu 15, Windows 10 or OSX Nightlies. Maybe this fixed itself?

	Mike Taylor [:miketaylr]
	Comment 6 • 8 years ago

(Note: my Ubuntu and Windows machines are just VMs, so that might possibly be related -- but that would be a bit surprising)

	Loic Reporter
	Comment 7 • 8 years ago

Not fixed with the latest Nightly on Win 7.

	Loic Reporter
	Comment 8 • 8 years ago

In the reduced testcase, the red background on hover should NOT disappear when you are browsing the <select> dropdown list.

	Mike Taylor [:miketaylr]
	Comment 9 • 8 years ago

(and the website issue still reproduces for you, not just the reduced testcase?)

	Mike Taylor [:miketaylr]
	Comment 10 • 8 years ago

(oops, forgot ni?)

	Loic Reporter
	Comment 11 • 8 years ago

Website and testcase are still broken.

	Mike Taylor [:miketaylr]
	Comment 12 • 8 years ago

So I'm sort of stumped. Here's what I see (on Mac, Win 10 and Ubuntu 15): https://cloudup.com/c3D7ub9f1qI

The test case *is* busted, but the site isn't. I had Hallvord test as well and he got the functional site behavior as me.

Jim, it's hard to say if this is a serious site compat issue, or if there are certain environmental factors that influence it (not sure what that could be right now...).

On the face of it, it feels kind of edge-casey, but the test case shows it's a clear regression. Is there anyone on your team that can reproduce on the site?

	Loic Reporter
	Comment 13 • 8 years ago

(In reply to Mike Taylor [:miketaylr] from comment #12)
> So I'm sort of stumped. Here's what I see (on Mac, Win 10 and Ubuntu 15):
> https://cloudup.com/c3D7ub9f1qI

You're doing wrong on the website, you need to move the mouse outside the dropdown list, see my screencast.

	Mike Taylor [:miketaylr]
	Comment 14 • 8 years ago

OK, that's not exactly what the STR in Comment #0 say. ^_^

0) Enable e10s
1) Open http://otto.yndenz.com/
2) Hover the menu entry "Occasions"
3) Click on the <select> "Merk" and hover some items of its dropdown list
4) *Hover the mouse to the left off of the dropdown list and outside of the menu*

Expected: dropdown menu fades away, select menu closes
Actual: dropdown menu fades away, select menu remains open (floating in space)

OK yeah, we should probably fix this.

	Jeff Griffiths (:canuckistani) (:⚡︎)
	Comment 16 • 8 years ago

Jim: I think this should block M9. It feels a bit like an edge case but there are lot of fancy hover / menu things out there on the web and this is pretty surprising behaviour vs blink/webkit/non-e10s.

	Jim Mathies [:jimm]
	Comment 17 • 8 years ago

removing my ni so this falls into triage for an assignee

	Jim Mathies [:jimm]
	Comment 18 • 8 years ago

(In reply to Loic from comment #2)
> Created attachment 8718414 [details]
> index.html (reduced testcase)


STR with this test case:

1) hover over the select, the text in the ul should turn red
2) click the select displaying the drop down
3) move the mouse down to the popup entries

result: red background goes away
expected: the red background remains

The list element has a style on it associated with the 'hover' css selector - 

li:hover {
  background-color: red;
}

	Gabor Krizsanits (INACTIVE) Assignee
	Comment 19 • 8 years ago

So I looked into Bug 1083365 and with a simple tweak on that patch the situation became somewhat better, now if you follow steps in Comment 18 and you move the cursor slowly in step 3 then everything is working correctly, but if you move the mouse fast then we still get the buggy behavior. Anyway, while I'm working on this I thought the best if I ask for a quick feedback from you.

	:Felipe Gomes (needinfo for replies!)
	Comment 20 • 8 years ago

So yeah, the root of the issue is that the two DOMs are separate (the popup in the chrome process, the <select> in the child), so tracking what the mouse is hovering gets complicated.

I'd just like to mention that Chrome/Safari don't handle this either. It just looks like they do, because they handle it equally well as we did before bug 1083365.

How to test:
 - open the reduced test case
 - navigate through the page with the keyboard until you hit focus on the select
 - open the select with the keyboard (*)
 - move mouse over the options

Expected:
 - If the chain was tracked perfectly, all browsers should show the red background when hovering over the options

But they don't. So what actually happens is that clicking on the select just doesn't cause a mouseout to happen in the content process, like before bug 1083365. So I suggest just looking for a way to revert that bug or tweaking it.


(*) Alt+↓ on Firefox, space on wekbit

	Gabor Krizsanits (INACTIVE) Assignee
	Comment 21 • 8 years ago

(In reply to :Felipe Gomes (needinfo me!) from comment #20)
> So yeah, the root of the issue is that the two DOMs are separate (the popup
> in the chrome process, the <select> in the child), so tracking what the
> mouse is hovering gets complicated.
> 
> I'd just like to mention that Chrome/Safari don't handle this either. It
> just looks like they do, because they handle it equally well as we did
> before bug 1083365.
> 
> How to test:
>  - open the reduced test case
>  - navigate through the page with the keyboard until you hit focus on the
> select
>  - open the select with the keyboard (*)
>  - move mouse over the options
> 
> Expected:
>  - If the chain was tracked perfectly, all browsers should show the red
> background when hovering over the options
> 
> But they don't. So what actually happens is that clicking on the select just
> doesn't cause a mouseout to happen in the content process, like before bug
> 1083365. So I suggest just looking for a way to revert that bug or tweaking
> it.
> 
> 
> (*) Alt+↓ on Firefox, space on wekbit

Yeah, that's what we agreed on with Smaug on IRC. I'm still a bit unsure though if there is any use case we should worry about. Anyway, I think I will just flag the remote event we fire and then ignore it for the state update code somehow.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 22 • 8 years ago

Comment on attachment 8730687 [details] [diff] [review]
WIP

Clearing this since I expect the right exit value should be based on the widget type, as I said on IRC:

	Gabor Krizsanits (INACTIVE) Assignee
	Comment 23 • 8 years ago

(In reply to Olli Pettay [:smaug] from comment #22)
> Comment on attachment 8730687 [details] [diff] [review]
> WIP
> 
> Clearing this since I expect the right exit value should be based on the
> widget type, as I said on IRC:

I don't think we can do that. Widget type is always panel here since the widget is the puppet widget for the child content. From parent side I don't think we can get more info from the underlying content.

	Gabor Krizsanits (INACTIVE) Assignee
	Comment 24 • 8 years ago

(In reply to :Felipe Gomes (needinfo me!) from comment #20)

> But they don't. So what actually happens is that clicking on the select just
> doesn't cause a mouseout to happen in the content process, like before bug
> 1083365. So I suggest just looking for a way to revert that bug or tweaking
> it.

It's a bit more complex than that. Bug 1083365 is about a case where we want to notify the child about the mouse exit event because we don't want to show the tooltip when then mouse pointer leaves the browser window or content area. But you're right, google chrome seem to have the same problem. If we want to use chromium as a reference for this behavior their solution seems to be a simple timeout for the tooltip... so it won't stay there forever only for a few seconds and then fades out.

Using the type of the popup did not work out. We don't have information about the new content the mouse enters when the exit event happens so I cannot use the related content either (it's null).

Smaug, what do you think about reverting bug 1083365 and introducing a timeout for tooltips? (it should run in the content process and check if the pointer is still over the element the tooltip belongs to and if not then just hide).

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 25 • 8 years ago

Ah, so I was thinking to check the widget type of the thing mouse is moved to be over. So I guess we'd need to change the code send eMouseExitFromWidget to child process when we get eMouseEnterIntoWidget
in parent for a widget which doesn't contain that child process.

Timeout sounds like a hack to me.

	Gabor Krizsanits (INACTIVE) Assignee
	Comment 26 • 8 years ago

(In reply to Olli Pettay [:smaug] from comment #25)
> Ah, so I was thinking to check the widget type of the thing mouse is moved
> to be over. So I guess we'd need to change the code send
> eMouseExitFromWidget to child process when we get eMouseEnterIntoWidget
> in parent for a widget which doesn't contain that child process.
> 
> Timeout sounds like a hack to me.

We discussed this over IRC. Summary: it is not guaranteed that an enter event will follow the exit event, the pointer can leave the browser window let's say, like in Bug 1083365 Comment 6. Before I try my luck with the timeout approach I think it's worth to check if we can teach the popup to know about the content it belongs to. That way it could send a hover event to the related content which would be a nicer solution for sure.

	Gabor Krizsanits (INACTIVE) Assignee
	Comment 27 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=28b27b59f263

	Gabor Krizsanits (INACTIVE) Assignee
	Comment 28 • 8 years ago

Looks green. What do you think?

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 29 • 8 years ago

Comment on attachment 8733895 [details] [diff] [review]
Cross process hover state management for <select>. v1


>+
>+    /*
>+     * Set hover state on an element and its ancestors. This function is only for internal usage
>+     * namely to help cross process hover state management for <select>.
>+     */
>+    [implicit_jscontext]
>+    void setHoverState(in jsval object, in boolean state);
This is very wrong place for this kind of code.


>+nsXPCComponents_Utils::SetHoverState(HandleValue value, bool state, JSContext* cx)
>+{
>+    NS_ENSURE_TRUE(value.isObject(), NS_ERROR_INVALID_ARG);
>+    RootedObject obj(cx, &value.toObject());
>+    obj = js::CheckedUnwrap(obj);
>+    NS_ENSURE_TRUE(obj, NS_ERROR_INVALID_ARG);
>+    nsXPConnect* xpc = nsXPConnect::XPConnect();
>+    nsISupports* native = xpc->GetNativeOfWrapper(cx, obj);
>+    EventStateManager::UpdateAncestorState(native,
>+                                           NS_EVENT_STATE_HOVER,
>+                                           state);
Please no manual JS API usage when all this should happen automatically in webidl bindings layer or in xpconnect.

>+      case "Forms:MouseOver":
>+        Cu.setHoverState(this.element, true);
>+        break;
>+
>+      case "Forms:MouseOut":
>+        Cu.setHoverState(this.element, false);
>+        break;
If we didn't have setContentState already, I'd say setHoverState should go to DOMWindowUtils.
But looks like DOMUtils has setContentState. I think you could reuse that one.
It is unclear why you want to explicitly call UpdateAncestorState and not SetContentState, but perhaps there is
some good reason?

>+++ b/toolkit/modules/SelectParentHelper.jsm
Whoever has looked at this code, should review this too, but looks reasonable to me.

	Gabor Krizsanits (INACTIVE) Assignee
	Comment 30 • 8 years ago

(In reply to Olli Pettay [:smaug] from comment #29)
> This is very wrong place for this kind of code.

Yeah... I knew that there has to be a better place than this just could not find one.


> If we didn't have setContentState already, I'd say setHoverState should go
> to DOMWindowUtils.

Sounds good.

> But looks like DOMUtils has setContentState. I think you could reuse that
> one.
> It is unclear why you want to explicitly call UpdateAncestorState and not
> SetContentState, but perhaps there is
> some good reason?

I haven't realized that setContentState is exposed to start with... that being said I
did have a reason why I used UpdateAncestorState instead. When hovering over the menu
if I call setContentState on the related ancestor in content that'll set the mHoverContent
to the ancestor in the content EventStateManager, which is not true and I don't know what
side effect it might cause. The second reason is that when leaving the menu we might end up
in a racy situation... Content notices that some element is getting hovered and updates but we
also get a message from parent to unhover the ancestor. I did not experience any issues but my
version mitigates the damage if it ever happens. Plus the current API is not good for setting state to false, because for that we should pass in a null for the element but then this method will fail (it cannot find the state manager, the update ancestor is a static function so it's a simpler case)

What do you think? Are these good enough reasons to use UpdateAncestorState?

Also, thanks for the quick feedback.

	Gabor Krizsanits (INACTIVE) Assignee
	Comment 31 • 8 years ago

I've added removeContentState but I also found setContentState handling the return value of the esm function wrong, so I fixed that too and simplified it a bit while I was there.

	Gabor Krizsanits (INACTIVE) Assignee
	Comment 32 • 8 years ago

Setting the r? flag on Felipe as Olli requested.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 33 • 8 years ago

Comment on attachment 8734375 [details] [diff] [review]
removeContentState. v1

> inDOMUtils::SetContentState(nsIDOMElement* aElement,
>-                            EventStates::InternalType aState)
>+                            EventStates::InternalType aState,
>+                            bool* _retval)
aRetVal


>+inDOMUtils::RemoveContentState(nsIDOMElement* aElement,
>+                               EventStates::InternalType aState,
>+                               bool* _retval)
aRetVal



>   unsigned long long getContentState(in nsIDOMElement aElement);
>-  void setContentState(in nsIDOMElement aElement, in unsigned long long aState);
>+  bool setContentState(in nsIDOMElement aElement, in unsigned long long aState);
>+  bool removeContentState(in nsIDOMElement aElement, in unsigned long long aState);
You could add a comment telling what the return value means, or at least a comment saying that 
see EventStateManager::SetContentState for return value.

	Pulsebot
	Comment 34 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/d791887510c8
https://hg.mozilla.org/integration/mozilla-inbound/rev/61241975d4da

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 35 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/d791887510c8
https://hg.mozilla.org/mozilla-central/rev/61241975d4da

	Gabor Krizsanits (INACTIVE) Assignee
	Comment 36 • 8 years ago

Comment on attachment 8734375 [details] [diff] [review]
removeContentState. v1

Approval Request Comment
[Feature/regressing bug #]: Under e10s this never worked as it should have.
[User impact if declined]: removeContentState API is needed for implementing correct hovering machinery for select popup menus. This patch also fixes some trivial mistakes in the existing SetContentState API. This patch is blocking e10s release.
[Describe test coverage new/current, TreeHerder]: Manually tested, existing tests are passing, it's on mc for a while.
[Risks and why]: Very low, the new API is not even used in this patch. The already existing API with the bugfix is harmless.
[String/UUID change made/needed]: inIDOMUtils uuid change for the new API

	Gabor Krizsanits (INACTIVE) Assignee
	Comment 37 • 8 years ago

Comment on attachment 8734376 [details] [diff] [review]
Cross process hover state management for <select>. v2

Approval Request Comment
[Feature/regressing bug #]: Never worked as it should have.
[User impact if declined]: Hovering machinery for select popup will be broken for e10s. This patch is blocking e10s release.
[Describe test coverage new/current, TreeHerder]: Same as previous.
[Risks and why]: Very low risk.
[String/UUID change made/needed]: none

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 38 • 8 years ago

Comment on attachment 8734375 [details] [diff] [review]
removeContentState. v1

e10s related, Aurora47+

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 39 • 8 years ago

Hello Loic, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 40 • 8 years ago

Jorge fyi, there is a UUID change here but I don't think this could impact addon compat. Please let me know if I was wrong and we should not uplift to Aurora.

	Ryan VanderMeulen [:RyanVM]
	Comment 41 • 8 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-aurora/rev/02165cf8bba1
https://hg.mozilla.org/releases/mozilla-aurora/rev/9c2756198915

	Loic Reporter
	Comment 42 • 8 years ago

(In reply to Ritu Kothari (:ritu) from comment #39)
> Hello Loic, could you please verify this issue is fixed as expected on a
> latest Nightly build? Thanks!

The testecase is fixed with e10s and the latest Nightly on Win 7 (Built from https://hg.mozilla.org/mozilla-central/rev/e847cfcb315f511f4928b03fd47dcf57aad05e1e), but there is still a remaining issue with the website. I'll investigate and file a new bug report about this issue.

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 43 • 8 years ago

(In reply to Loic from comment #42)
> (In reply to Ritu Kothari (:ritu) from comment #39)
> > Hello Loic, could you please verify this issue is fixed as expected on a
> > latest Nightly build? Thanks!
> 
> The testecase is fixed with e10s and the latest Nightly on Win 7 (Built from
> https://hg.mozilla.org/mozilla-central/rev/
> e847cfcb315f511f4928b03fd47dcf57aad05e1e), but there is still a remaining
> issue with the website. I'll investigate and file a new bug report about
> this issue.

Sounds good. Thanks again!

	Jorge Villalobos [:jorgev] (he/him)
	Comment 44 • 8 years ago

Not a problem for add-on compat.