Closed Bug 1197913 Opened 4 years ago Closed 4 years ago

[e10s] Moving the cursor outside the <select> drop-down list doesn't keep the last hovered item highlighted

Categories

(Core :: Layout: Form Controls, defect)

43 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla48
Tracking Status
e10s m9+ ---
firefox43 --- affected
firefox46 --- wontfix
firefox47 --- fixed
firefox48 --- verified

People

(Reporter: epinal99-bugzilla2, Assigned: ting)

References

(Blocks 1 open bug)

Details

(Whiteboard: [mozfr-community])

Attachments

(2 files, 3 obsolete files)

I'm surprised this issue hasn't been reported yet, so maybe it's a dupe even if I checked bug 1154677.

STR:
1) Open testcase https://bug1165782.bmoattachments.org/attachment.cgi?id=8606895
2) Click on the select (you can mouse up)
3) Move the cursor across the items then outside the drop-down list

Result:
The last item hovered doesn't stay highlighted after the mouse cursor goes outside the drop-down list.
Reproducible on all OS (Lin/OSX/Win) with aurora46.
(Mozilla/5.0 (X11; Linux i686; rv:46.0) Gecko/20100101 Firefox/46.0 20160208004010 )

Result: E10s enabled - The last item hovered doesn't stay highlighted after the mouse cursor goes outside the drop-down list.

Result: E10s dissabled - The last item hovered  stays highlighted after the mouse cursor goes outside the drop-down list.
Whiteboard: [mozfr-community]
It's unhighlighted by https://dxr.mozilla.org/mozilla-central/source/layout/xul/nsMenuFrame.cpp#471. I don't know how will it impact if the behavior is changed.

For non-e10s, nsListControlFrame::MouseMove does not change selection when the mouse event is outside of the rectangle:

https://dxr.mozilla.org/mozilla-central/source/layout/forms/nsListControlFrame.cpp#1865-1866
https://dxr.mozilla.org/mozilla-central/source/layout/forms/nsListControlFrame.cpp#1741-1743
(In reply to Ting-Yu Chou [:ting] from comment #2)
> It's unhighlighted by
> https://dxr.mozilla.org/mozilla-central/source/layout/xul/nsMenuFrame.
> cpp#471. I don't know how will it impact if the behavior is changed.

Neil, what do you think if this line is removed? Will it break anything you know? Thanks.
Flags: needinfo?(enndeakin)
It might be ok, but will need some detailed testing for instance when submenus are present and/or disabled items nearby.

Note though that in the brave new world, where we try to emulate native widget behaviour instead of the arbitrary behaviour of the non-e10s select element, this bug only applies to menulists and only on Windows, so the possibilities of noticeable issues are more limited.
Flags: needinfo?(enndeakin)
I just did some tests see how does the thing work on different places:

Keep it highlighted:
  Windows 10 native widget
  Chrome (Windows, and Ubuntu)

Unhighlighted:
  Utubnut native widget
  Safari
  IE
  Edge
See Also: → 1053981
Assignee: nobody → janus926
Attached patch patch v1 (obsolete) — Splinter Review
I am not sure how to better test it, I ran this test:

  toolkit/content/tests/chrom/test_menu.xul

which has submenu and disabled items I added.

BTW, one thing I noticed on Windows is when cursor moves outside of a menu, the last hover item will be unhighlighted, which is different from selector.
Attachment #8732069 - Flags: review?(enndeakin)
Attached patch patch v2 (obsolete) — Splinter Review
Accidentally included test code, removed it.
Attachment #8732069 - Attachment is obsolete: true
Attachment #8732069 - Flags: review?(enndeakin)
Attachment #8732072 - Flags: review?(enndeakin)
Comment on attachment 8732072 [details] [diff] [review]
patch v2

Review of attachment 8732072 [details] [diff] [review]:
-----------------------------------------------------------------

No, as I said the new behaviour only applies to Windows and only to items in menulists. The current behaviour is correct for regular menus on Windows, and all cases on Mac and GTK.

On Windows, you want to check if menuParent->GetParent() can QI to nsIDOMXULMenuListElement and not call ChangeMenuItem in this case.
Attachment #8732072 - Flags: review?(enndeakin) → review-
Attached patch patch v3 (obsolete) — Splinter Review
So we will still have different behavior between e10s and non-e10s on Linux and Mac.
Attachment #8732072 - Attachment is obsolete: true
Attachment #8732707 - Flags: review?(enndeakin)
Comment on attachment 8732707 [details] [diff] [review]
patch v3

Might be good to have a test.
Attachment #8732707 - Flags: review?(enndeakin) → review+
(In reply to Neil Deakin from comment #10)
> Might be good to have a test.

I will work out one.
Attached patch patch v4Splinter Review
Added a test which checks whether the last hover item is highlighted. I am not sure should this go to mochitest.ini, browser.ini, or chrome.ini. Thank you. :)
Attachment #8732707 - Attachment is obsolete: true
Attachment #8734674 - Flags: review?(enndeakin)
Attachment #8734674 - Flags: review?(enndeakin) → review+
Try looks ok.
Keywords: checkin-needed
This is fixed on Windows only, see comment 4, 8 and 9 for the reason.
https://hg.mozilla.org/mozilla-central/rev/d820bfb71d3a
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Ting-Yu, should your bug fix be uplifted to Aurora 47 in preparation for our e10s experiments on Beta 47?
Flags: needinfo?(janus926)
Comment on attachment 8734674 [details] [diff] [review]
patch v4

Approval Request Comment
[Feature/regressing bug #]: 897060
[User impact if declined]: different <select> drop-down list behavior on Windows between e10s and non-e10s/native widget
[Describe test coverage new/current, TreeHerder]: automated, also added a test to make sure it behaves as expected
[Risks and why]: low, it's just to keep the last hovered drop-down list item highligted after moving the cursor outside
[String/UUID change made/needed]: n/a
Flags: needinfo?(janus926)
Attachment #8734674 - Flags: approval-mozilla-aurora?
Comment on attachment 8734674 [details] [diff] [review]
patch v4

e10s related, Aurora47+
Attachment #8734674 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
I don't think we are taking any uplifts to Beta 46 for e10s related regressions. Updating the status field to reflect that.
Hello Loic, here is another bug that could use your verification? Please use the latest Nightly build as this hasn't been uplifted to Aurora channel yet. Many thanks for your ever awesome help!
Flags: needinfo?(epinal99-bugzilla2)
(In reply to Ritu Kothari (:ritu) from comment #22)
> Hello Loic, here is another bug that could use your verification? Please use
> the latest Nightly build as this hasn't been uplifted to Aurora channel yet.
> Many thanks for your ever awesome help!

The bug is partially fixed: when moving the cursor outside the dropdown list, the hovered option stays highlighted but not the dropdown arrow of the <select>.

I'll file a new bug for the remaining issue.
Flags: needinfo?(epinal99-bugzilla2)
Blocks: 1264271
Verified on Nightly, thanks to Loic!
Status: RESOLVED → VERIFIED
See Also: → 1400589
You need to log in before you can comment on or make changes to this bug.