schwarz.ware Reporter
	Description • 8 years ago

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:44.0) Gecko/20100101 Firefox/44.0
Build ID: 20160210153822

Steps to reproduce:

Wrote a html document with a <select> (in fact its part of a big page but I reduced it to the minimum to reproduce problem).


Actual results:

The select-drop-down list closes unexpectedly before an option could be selected. Sometimes. It seem to depend on mouse-move-speed.

I provide a html doc that shows the problem and more.



Expected results:

The drop-down list should stay open when I try to move the mouse on it.

	Loic
	Comment 1 • 8 years ago

There are 2 bugs in your testcase and, in general, we prefer 1 bug per report. ;)

Bug #1: the dropdown list disappears when the mouse is hovering over it slowly.
Bug #2:  the dropdown list disappears and the highlighted option is selected if the mouse goes outside the dropdown list even if the user didn't click on the highlighted option.

Bug #1 is a recent regression, bug #2 is an old bug (FF21 is affected e.g.), so let's keep this bug report for bug #1 and open a new bug report for bug #2.

Reg range for bug #1:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=0288a0a7003ffba272dc8040567e225d7115d9bc&tochange=5efb9a11196ba113b830cf4717d85513e3468783
Chris H-C — Bug 506815 - Replace MouseTrailer with TrackMouseEvent. r=jimm

In addition, only non-e10s mode is affected, in e10s, both bugs are not reproducible.

	Chris H-C :chutten Assignee
	Comment 2 • 8 years ago

I appreciate the ni?. I don't suppose you know anyone who knows the dropdown code on Windows? I wasn't aware that it could be using a mouseenter/mouseleave pair (still not sure that it is).

My wild guess is that the border widens the separation between the embedded widget (the select) and the floating widget (the options list). If a mousemove happens over that separation area (depends on the velocity of the pointer, due to throttling) then it behaves the same as though the mouse left the select+optionslist area (iow, the options list dismisses).

I'd have to run back that patch to doublecheck, but my guess is, if you left the pointer in the separation area for longer than the 200ms MousTrailer timer, you'd get the bug even without the bug 506815 patch. (iow, it isn't a regression so much as a "make this bug a lot easier to hit"ssion).

As to why this wouldn't happen with e10s enabled, I'm guessing that bug 506815 only really affects windows spawned in-process by the chrome process, due to how registration for mouseenter/mouseleave works in Windows.

...again, though, this is guesswork :S

	Loic
	Comment 3 • 8 years ago

For bug #2, see bug 1249757.

	Loic
	Comment 4 • 8 years ago

(In reply to Chris H-C :chutten from comment #2)
> I appreciate the ni?. I don't suppose you know anyone who knows the dropdown
> code on Windows? 

Yes, sorry, it's out of my scope. :)
I just NI you because you're the author of the patch which regressed. Feel free to re-NI to someone else.

	schwarz.ware Reporter
	Comment 5 • 8 years ago

Thank you for splitting into separate bug reports.
Only I guess the new title of this bug (Styled <select> dropdown list ...) is a little bit misleading. Because its not the styling of the <select> that causes the problem. Its the styling of the outer <TD> element, namely:

  TD { overflow: hidden; } TD:hover { overflow: visible; }

The strange behaviour seem to happen only in this combination of TD's overflow definition. If either the hidden or visible style definition is omitted, the <select> behaves as expected.

Further, I found out, that it has nothing to do with <TD>. It happens also if the <select> is placed inside a <DIV> having that particular overflow-style.

	schwarz.ware Reporter
	Comment 6 • 8 years ago

The testcase for my comment #5

	Chris H-C :chutten Assignee
	Comment 7 • 8 years ago

I had a chat with :jimm and :Gijs on #developers about this.

1) :jfkthame reported that the bug is present and much worse on Mac
2) The code controlling this is in nsIListControlFrame.cpp, and I think it has to do with capturing mouse events: https://dxr.mozilla.org/mozilla-central/source/layout/forms/nsListControlFrame.cpp#849
3) :jimm confirms that it doesn't reproduce with e10s on.

I can't quite nail down where in the code lies the trigger to close the options list when the mouse exits. I thought it would be nsListControlFrame::MouseMove (https://dxr.mozilla.org/mozilla-central/source/layout/forms/nsListControlFrame.cpp#1854) but no such luck.

	Chris H-C :chutten Assignee
	Comment 9 • 8 years ago

:jimm suggests that maybe :dbaron or :roc may be able to help us understand that stack. ni?

	David Baron :dbaron:
	Comment 10 • 8 years ago

That stack just says that we're in the middle of restyling the combobox in a way that requires recreating its frames.

So one question is whether we properly maintain the state (both just being open and other state) of the dropdown when recreating frames for a combobox -- in both the e10s and non-e10s versions of selects.  It seems like we should.  It should be simple to write a testcase that triggers that -- just put a combobox inside an element that dynamically (from JS) changes between overflow:visible and overflow:hidden every second.

(If it's not clear to you why that style change is happening, you could get an rr recording of this, start from the stack you gave, set a watchpoint on the data that ProcessRestyledFrames is pulling out of the change list, and reverse-continue to where it got put into the changelist, and look at what property changes generated the hint.  But it's not clear to me whether that's actually useful, given that it should be simple to write a testcase.)

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 11 • 8 years ago

Regression from 44, but only for non-e10s. Tracking for 46+. Not sure a fix would make it to 45 at this point (a week from release).

Chris are you able to take on this bug?

	Chris H-C :chutten Assignee
	Comment 12 • 8 years ago

I'm afraid I can't take this bug at the moment. But please find attached a simple test case as previously described.

Indeed, when the style changes on the parent element, the select is closed.

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 13 • 8 years ago

dbaron or jet, can you help find an owner for this bug, and bug 1249757?

	Jet Villegas (inactive)
	Comment 14 • 8 years ago

Matt: let's look at these 2 on your debugger

	Matt Woodrow (:mattwoodrow)
	Comment 15 • 8 years ago

I won't have time to debug this right now, swamped with other regressions.

It looks like we could back out the regressing changeset, it doesn't look to be too critical to ship.

	Chris H-C :chutten Assignee
	Comment 16 • 8 years ago

We should probably keep the removal of MouseTrailer as it removed a 200ms wakeup timer that likely contributed to Firefox's poor battery performance. Also, it is a step in the right direction: neither GTK nor cocoa use a timer to find out where the mouse is.

	Matt Woodrow (:mattwoodrow)
	Comment 17 • 8 years ago

I didn't mean a permanent backout, just a temporary one until this regression can be fixed.

	David Baron :dbaron:
	Comment 18 • 8 years ago

Until it has an owner who is willing to take responsibility for fixing regressions (see comment 12), I think a permanent backout might be more appropriate.  (Note that taking responsibility for doing it doesn't necessarily mean doing all of the work; it means making sure that it happens.)

This regression shouldn't have gotten dropped on the floor until it was too late to avoid shipping it.

	Jet Villegas (inactive)
	Comment 19 • 8 years ago

(In reply to David Baron [:dbaron] ⌚️UTC+8 from comment #18)
> Until it has an owner who is willing to take responsibility for fixing
> regressions (see comment 12), I think a permanent backout might be more
> appropriate. 

The MouseTracker code has been in the tree since the Netscape days:
https://github.com/ehsan/mozilla-cvs-history/blob/d91e4affc9addc181032000bdc126392d94d0745/widget/src/windows/nsToolkit.cpp

Let's put it back in until we've got a handle on these regressions. We might also try making this require e10s before we let it back in the tree.

	Chris H-C :chutten Assignee
	Comment 20 • 8 years ago

I have some time now to work on this. 

A fun note about e10s comboboxes, if focus starts outside of the combobox, it takes two clicks on the inline portion to dismiss the popup (but only one if you click anywhere else). Should I file a bug?

I think I'm getting somewhere. The previously-posted stack might be incidental, as it doesn't appear to be the reason for the popup to rollup. I'm tracking down the actual rollup to see what's causing it (perhaps the WM_ACTIVATE WA_INACTIVE with a 0 lParam might be the culprit? Or it might be a side-effect of my debugging.)

	Chris H-C :chutten Assignee
	Comment 21 • 8 years ago

(It was a side-effect of my debugging, feel free to ignore.)

The original question remains: the frame gets freed and rebuilt, and its state is not maintained (really, it ought not to, in general). The widget is hidden by the first Reflow() after the recreated frame is built. Who _should_ save mDroppedDown? The <select> (mDropdownFrame->mContent)? The nsIStatefulFrame (SaveState)? The widget? Its view? Should the mechanism by which the widget and view are reassigned to the recreated Frame be responsible? (and if so, what is it?)

	Chris H-C :chutten Assignee
	Comment 22 • 8 years ago

Conversations with :bz on #developers suggest that the correct place to save and restore this data is through the nsIStatefulFrame mechanism. 

Unfortunately, it appears as though, for the twiddle test page, the state saved in nsComboboxControlFrame::SaveState isn't the state that is restored in nsComboboxControlFrame::RestoreState. The stateKey is identical, and it doesn't seem to matter what state I'm saving (the mContentData of whether or not the select is dropped down, or simply a scroll position, or disabled state).

The code runs and appears correct, but something isn't going right.

	Chris H-C :chutten Assignee
	Comment 23 • 8 years ago

Oh bother. The hashset of states is keyed off of document and element. 

Both the ComboboxControlFrame and ListControlFrame are nsIStatefulFrames. Both of them represent the same document and element, so get the same stateKey. In frame tree order traversed by nsFrameManager::CaptureFrameState, the list control frame comes afterwards. Which means it overwrites whatever is set by the combobox control's SaveState.

So, some things:
1) The cascading SaveState call in ComboboxControlFrame needs to go. It's a waste of code and runtime.
2) Maybe nsILayoutHistoryState should assert when AddState is called with a statekey already present in the hash. This will stop erroneous uses of stateful frames like the one I was trying to do.
3) Which means we can cut some storage because ComboboxControlFrame will need to stop being stateful

I'm going to try to get the list control frame to be a little smarter about being stateful. Then it can ask the combobox (if present) for mDroppedDown and add it into the stateful state.

Clearing dbaron's ni? as I don't actually have any questions at the moment (beyond "Why, layout code, why?")

	Sylvestre Ledru [:Sylvestre]
	Comment 24 • 8 years ago

Too late for 45 indeed. We could take a patch in esr45 if it is really important.

	Chris H-C :chutten Assignee
	Comment 25 • 8 years ago

The nsListControlFrame for the combobox is already stateful. It doesn't need
the combobox to delegate the SaveState call. CaptureFrameState already recurses
for us.

Review commit: https://reviewboard.mozilla.org/r/38973/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38973/

	Chris H-C :chutten Assignee
	Comment 26 • 8 years ago

If multiple stateful frames are representing the same element on the same
document, they will have the same stateKey and will overwrite each others'
states in LayoutHistoryState. This is not intended behaviour, so an assert
would be prudent.

Review commit: https://reviewboard.mozilla.org/r/38975/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38975/

	Chris H-C :chutten Assignee
	Comment 27 • 8 years ago

If a style recalculation happens, the frames that make up a select dropdown are
rebuilt. To save the dropped-down state, we rely on ListControlFrame's stateful
frame. On restore, we re-show the combobox if it was previously-dropped-down.

Review commit: https://reviewboard.mozilla.org/r/38977/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38977/

	David Baron :dbaron:
	Comment 28 • 8 years ago

I'm a little bit disturbed that this patch is so unrelated to the patch causing the regression, since it seems like there's a risk of ending up with a big sequence of mostly-unrelated patches that all depend on each other and each need regressions fixed in order to ship them without regressions.  But this does seem like stuff that's worth fixing.

	David Baron :dbaron:
	Comment 29 • 8 years ago

Comment on attachment 8728555 [details]
MozReview Request: bug 1249664 - Test that the popup doesn't hide on frame reconstruction r?RyanVM

https://reviewboard.mozilla.org/r/38973/#review35719

	David Baron :dbaron:
	Comment 30 • 8 years ago

https://reviewboard.mozilla.org/r/38973/#review35721

I'd prefer commit messages of the form "Make nsComboboxControlFrame not implement nsIStatefulFrame" rather than "should not", since the commit messages should describe what's changing rather than describing a problem statement (which is then ambiguous about whether the problem is being fixed, created, or commented about).

	David Baron :dbaron:
	Comment 31 • 8 years ago

Comment on attachment 8728556 [details]
MozReview Request: bug 1249664 - Assert when additions to LayoutHistoryState overwrite. r?dbaron

https://reviewboard.mozilla.org/r/38975/#review35723

In the commit message, change "Subsequent additions to LayoutHistoryState overwrite" to something more like "Assert when LayoutHistoryState is overwritten."

	David Baron :dbaron:
	Comment 32 • 8 years ago

Comment on attachment 8728557 [details]
MozReview Request: bug 1249664 - Save dropped-down state in nsPresState r?dbaron

https://reviewboard.mozilla.org/r/38977/#review35727

"across reflows" is the wrong description of the patch; reflow doesn't cause frame reconstruction, you should refer to "across frame reconstruction"

You should be aware that this state saving and restoration is used both for frame reconstruction and for back/forward caching (but only when we're not using the bfcache, i.e., when CanSavePresentation returns false or the bfcache is too big... I think).  But I don't think any of the state saved by combobox frames (scroll position, dropped down state) is relevant across back/forward.  (State saved by the elements is relevant, but it seems that nsGenericHTMLFormElementWithState::GenerateStateKey makes that not conflict.)

::: layout/forms/nsListControlFrame.cpp:2491
(Diff revision 1)
> +  if (!mComboboxFrame)
> +    return NS_OK;

{} around return

::: layout/forms/nsListControlFrame.cpp:2494
(Diff revision 1)
> +  if (!*aState) {
> +    *aState = new nsPresState();
> +  }

So what happens if we're in a situation where ScrollFrameHelper::SaveState returns nullptr, but now this code creates a saved state.

When restoration comes around, we'll now be calling ScrollFrameHelper::RestoreState where I think we wouldn't have called it before.  What ensures that this works correctly?

::: layout/forms/nsListControlFrame.cpp:2498
(Diff revision 1)
> +  nsCOMPtr<nsISupportsPRBool> droppedDown(do_CreateInstance(NS_SUPPORTS_PRBOOL_CONTRACTID));
> +  NS_ENSURE_TRUE(droppedDown, NS_ERROR_FAILURE);
> +  droppedDown->SetData(mComboboxFrame->IsDroppedDown());

I'm really not happy about nsISupportsPRBool usage here.  Could you just add a new boolean member variable to nsPresState instead?

I suppose that means that I should look at the patch again after you do that.

::: layout/forms/nsListControlFrame.cpp:2501
(Diff revision 1)
> +  (*aState)->SetStateProperty(droppedDown);

I was worried that this would collide with the SetStateProperty in HTMLSelectElement, but we do some key magic in nsGenericHTMLFormElementWithState::GenerateStateKey that I think makes this OK.

::: layout/forms/nsListControlFrame.cpp:2516
(Diff revision 1)
> +    mComboboxFrame->ShowDropDown(droppedDown);

I'm a little bit worried about whether it's always safe to call ShowDropDown here, but I guess it seems ok.

	Chris H-C :chutten Assignee
	Comment 33 • 8 years ago

Comment on attachment 8728555 [details]
MozReview Request: bug 1249664 - Test that the popup doesn't hide on frame reconstruction r?RyanVM

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38973/diff/1-2/

	Chris H-C :chutten Assignee
	Comment 34 • 8 years ago

Comment on attachment 8728557 [details]
MozReview Request: bug 1249664 - Save dropped-down state in nsPresState r?dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38977/diff/1-2/

	Chris H-C :chutten Assignee
	Comment 35 • 8 years ago

We already restore the scroll-position state of the list control frame (via
nsHTMLScrollFrame), so the combobox control frame needs to add a suffix to
its state key so it doesn't overlap.

Review commit: https://reviewboard.mozilla.org/r/39259/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39259/

	Chris H-C :chutten Assignee
	Comment 36 • 8 years ago

Comment on attachment 8728556 [details]
MozReview Request: bug 1249664 - Assert when additions to LayoutHistoryState overwrite. r?dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38975/diff/1-2/

	Chris H-C :chutten Assignee
	Comment 37 • 8 years ago

I took a different approach with this rev. The previous approach wouldn't support the changes necessary for r+ (the piggybacking of both list and combobox's states onto a single nsPresState was just not going to work out cleanly) so I figured, since stateful frames are responsible for their states, they should be responsible for their stateKeys as well.

Hopefully this one works out better than the last.

	David Baron :dbaron:
	Comment 38 • 8 years ago

Comment on attachment 8728557 [details]
MozReview Request: bug 1249664 - Save dropped-down state in nsPresState r?dbaron

https://reviewboard.mozilla.org/r/38977/#review36271

	David Baron :dbaron:
	Comment 39 • 8 years ago

Comment on attachment 8729135 [details]
MozReview Request: bug 1249664 - Save the combobox's dropped-down state across frame reconstruction r?dbaron

https://reviewboard.mozilla.org/r/39259/#review36273

The review comments on the previous version of this patch are in https://reviewboard.mozilla.org/r/38977/ since you don't seem to be using a version control setup that uses MozReview propertly (I'm guessing that means git).  You probably shouldn't be using MozReview at all given that.

::: layout/forms/nsComboboxControlFrame.h:204
(Diff revision 1)
>    //nsIStatefulFrame
>    NS_IMETHOD SaveState(nsPresState** aState) override;
>    NS_IMETHOD RestoreState(nsPresState* aState) override;
> +  NS_IMETHOD GenerateStateKey(nsIContent* aContent,
> +                              nsIDocument* aDocument,
> +                              nsACString& aKey) override;

I don't follow this.  GenerateStateKey isn't a method on nsIStatefulFrame, which means that your grouping of the method is misleading, and more importantly that nothing will call your new GenerateStateKey method (unless I've missed something in one of the other patches).

The comment above means my second comment the last time around, in https://reviewboard.mozilla.org/r/38977/ , isn't yet fixed.

	David Baron :dbaron:
	Comment 40 • 8 years ago

Comment on attachment 8729135 [details]
MozReview Request: bug 1249664 - Save the combobox's dropped-down state across frame reconstruction r?dbaron

https://reviewboard.mozilla.org/r/39259/#review36275

Er, sorry about that, that first patch I reviewed a few days ago did add nsIStatefulFrame::GenerateStateKey...

::: layout/forms/nsComboboxControlFrame.cpp:1657
(Diff revision 1)
> -  if (!mListControlFrame)
> -    return NS_ERROR_FAILURE;
> -
> +  if (!*aState) {
> +    (*aState) = new nsPresState();
> +  }

I think you can just allocate the state eagerly now, although you might want to assert that *aState is null at the beginning.

::: layout/forms/nsComboboxControlFrame.cpp:1674
(Diff revision 1)
> +NS_IMETHODIMP
> +nsComboboxControlFrame::GenerateStateKey(nsIContent* aContent,
> +                                        nsIDocument* aDocument,
> +                                        nsACString& aKey)
> +{

Please add a comment explaining that this does what it does so that the state key is different from the listbox for the same element, which *sometimes* saves its scroll position.

so r=dbaron with those two things fixed

	Chris H-C :chutten Assignee
	Comment 41 • 8 years ago

Comment on attachment 8729135 [details]
MozReview Request: bug 1249664 - Save the combobox's dropped-down state across frame reconstruction r?dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39259/diff/1-2/

	Ryan VanderMeulen [:RyanVM]
	Comment 42 • 8 years ago

Tests? :)

	Pulsebot
	Comment 43 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/f47a76e5ddbe
https://hg.mozilla.org/integration/mozilla-inbound/rev/66183f2dff75
https://hg.mozilla.org/integration/mozilla-inbound/rev/cea8bcf0fc02

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

https://hg.mozilla.org/mozilla-central/rev/f47a76e5ddbe
https://hg.mozilla.org/mozilla-central/rev/66183f2dff75
https://hg.mozilla.org/mozilla-central/rev/cea8bcf0fc02

	Chris H-C :chutten Assignee
	Comment 45 • 8 years ago

Test forthcoming. Currently wrestling with mochitest :S

	Chris H-C :chutten Assignee
	Comment 46 • 8 years ago

Comment on attachment 8728555 [details]
MozReview Request: bug 1249664 - Test that the popup doesn't hide on frame reconstruction r?RyanVM

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38973/diff/2-3/

	Ryan VanderMeulen [:RyanVM]
	Comment 47 • 8 years ago

Comment on attachment 8728555 [details]
MozReview Request: bug 1249664 - Test that the popup doesn't hide on frame reconstruction r?RyanVM

Not a peer of this code, sorry. That said, as mentioned on IRC yesterday, I'm worried about the requestFlakyTimeout since that's usually a good path to intermittent failures [1]. Also, if the test passes on Linux/OSX, we should let it run there as well. We typically don't restrict tests to running on one platform unless there's a really good reason for it. Beyond that, I'll leave the real review to someone who better understands the source material :)

[1] https://developer.mozilla.org/en-US/docs/Mozilla/QA/Avoiding_intermittent_oranges

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 48 • 8 years ago

I don't think this is good for uplift to beta - since it sounds like there is some doubt about the approach here.  Once the test lands you may want to request uplift to aurora so that we can ship the fix as soon as possible. If you don't think uplift is a good idea then let's also wontfix this for aurora.

	David Baron :dbaron:
	Comment 49 • 8 years ago

I agree with Ryan's comments on the test (and in general I think you should only use skip on tests when they crash).  But it would be good to finish the test and land it as well.  Any progress on that?

However, I don't think that problems with writing an automated test means there's "disagreement on the approach" in terms of whether it's worth backporting.

	Chris H-C :chutten Assignee
	Comment 50 • 8 years ago

Unfortunately I just don't know what makes the test require the timeout. I think it just has to wait for the native widget to properly fill out the space and be interactive before it will correctly capture the simulated click.

I just don't think this part of the code is written to be easily testable.

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

Hi Chutten, should we consider uplifting this fix to Beta47?

	Chris H-C :chutten Assignee
	Comment 52 • 8 years ago

(In reply to Ritu Kothari (:ritu) from comment #51)
> Hi Chutten, should we consider uplifting this fix to Beta47?

Six weeks without identified regressions suggests it ought to be a good candidate. Sure.

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

(In reply to Chris H-C :chutten from comment #52)
> (In reply to Ritu Kothari (:ritu) from comment #51)
> > Hi Chutten, should we consider uplifting this fix to Beta47?
> 
> Six weeks without identified regressions suggests it ought to be a good
> candidate. Sure.

In that case, could you please nominate a patch for uplift to Beta47? Thanks!

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

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

	Chris H-C :chutten Assignee
	Comment 55 • 8 years ago

Comment on attachment 8729135 [details]
MozReview Request: bug 1249664 - Save the combobox's dropped-down state across frame reconstruction r?dbaron

Approval Request Comment
[Feature/regressing bug #]: bug 506815
[User impact if declined]: non-e10s Firefox users may have difficulty selecting options in an HTML select if it has been styled.
[Describe test coverage new/current, TreeHerder]: None
[Risks and why]: Minor. The fix uses existing mechanisms to save the dropdown's state across frame reconstruction.
[String/UUID change made/needed]: None.

	Lutz Schwarz
	Comment 56 • 8 years ago

I tested this nightly:

firefox-49.0a1.en-US.win32.installer.exe

My impression is that it is very hard now to reproduce the effect that the dropdowbn closes unexpectedly. When moving the mouse between the dropdown and the input field and back again a lot of times (slow and/or quickly) it sometimes happened that the dropdown closes. Nobody does this, of course. For me this would be ok.

But it seems that this fix comes with another regression:
The background-color of the dropdown doesn't have the same background-color as the input field (the <select>). In my case, with the nightly, the select's background was yellowish but the dropdown's background was white. In FF version 46 the background colors of both are the same.

	Chris H-C :chutten Assignee
	Comment 57 • 8 years ago

Did you ensure to turn multi-process Firefox off when verifying this patch? Multiprocess Firefox uses a different dropdown codepath. Firefox is defaulted to multi-process on Nightly and Developer Edition channels.

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

Comment on attachment 8729135 [details]
MozReview Request: bug 1249664 - Save the combobox's dropped-down state across frame reconstruction r?dbaron

Fixes a regression with drop down lists, Beta47+

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

https://hg.mozilla.org/releases/mozilla-beta/rev/0c124b78808e
https://hg.mozilla.org/releases/mozilla-beta/rev/1276ba0df4ba
https://hg.mozilla.org/releases/mozilla-beta/rev/0d955f56f084

	Lutz Schwarz
	Comment 60 • 8 years ago

(In reply to Chris H-C :chutten from comment #57)
> Did you ensure to turn multi-process Firefox off when verifying this patch?
> Multiprocess Firefox uses a different dropdown codepath. Firefox is
> defaulted to multi-process on Nightly and Developer Edition channels.

No I didn't. But when I do and turned off multi-process Firefox, then it worked all fine:

* The drop-down didn't hide unexpectedly when moving the mouse from the select-field onto the drop-down (and back again) no matter by which speed the mouse is moved.

* The drop-down didn't hide if moving the mouse outside of the drop-down nor if moving it outside of the select-field (and not clicking somewhere), which is expected behaviour (the drop-down closes in this cases if multi-process Firefox is on).

* The background-color of drop-down and select-field are the same.

	Chris H-C :chutten Assignee
	Comment 61 • 8 years ago

Styling a select is and always has been tricky. If having the background-color propagate like that is important to your use-case, maybe file a bug? Let me know what its number is and I'll make sure the appropriate flags are set.

	Lutz Schwarz
	Comment 62 • 8 years ago

Popular browser we check for our application are using the same background color for the drop-down of a select as is defined for the select itself. And not just this, also padding, font attributes (e.g. font-style) and text colors are propagated (may be more but I didn't check more).

The latest FF 46.0.1 like those before propagates all we need, e.g. background-color (which is important for us). But it did at least not propagate the padding (whereas Opera and Chrome do). Currently padding is not important for my use-case. But if it becomes, I file a bug and refer to your post, thank you.