Closed Bug 858459 Opened 7 years ago Closed 7 years ago

drop-down select menu closes without selecting in Firefox 20.0

Categories

(Core :: Layout, defect)

20 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla23
Tracking Status
firefox20 --- affected
firefox21 + verified
firefox22 + fixed
firefox23 + verified

People

(Reporter: info, Assigned: mats)

References

Details

(Keywords: regression, testcase)

Attachments

(4 files)

User Agent: Mozilla/5.0 (Windows NT 5.1; rv:20.0) Gecko/20100101 Firefox/20.0
Build ID: 20130326150557

Steps to reproduce:

Following link loads a request form for a hotel booking:
http://www.caesar-data.com/cgi-bin/quick_check.cgi?hotel_id=demohotel
The calender is included in this form since several years, and it was working correctly with all Firefox editions incl. Firefox 19. Ir workt with all newerst versions of IE, Opera and Chrome.
It does not longer works with Firefox 20.0


Actual results:

If you try to select a month with the drop-down menu, the drop-down menu will close without selecting anything.
I suppose a bug in connection with Javascript in Firefox 20.0


Expected results:

The selected month should appear.
Recent regression probably backported into FF20.

Regression range:
m-c
good=2013-03-09
bad=2013-03-10
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e6215e0357fa&tochange=9e6232e86000
Status: UNCONFIRMED → NEW
Ever confirmed: true
Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/4cfcf5473878
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20130308 Firefox/22.0 ID:20130308024456
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/d5ec1f00ccd2
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20130308 Firefox/22.0 ID:20130308031011
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=4cfcf5473878&tochange=d5ec1f00ccd2
Regressed by:
d5ec1f00ccd2	Mats Palmgren — Bug 635852. r=smaug
Component: Untriaged → Layout
Product: Firefox → Core
I'm investigating...
Assignee: nobody → matspal
OS: Windows XP → All
Hardware: x86 → All
Attached file testcase
This is a simplified testcase that has the same bug.
Attachment #734234 - Attachment mime type: text/plain → text/html
Keywords: testcase
(In reply to Mats Palmgren [:mats] from comment #3)
> I'm investigating...

Hey Mats, any update here ?
Yeah, I do understand the problem.  The site installs this event handler
on the document (note that they sniff UA string and run different stuff):

document.onclick=FSfncHideDateSelector

function FSfncHideDateSelector(TheEvent) {
  if(FSblnIsShown==false){return};
  if(FSobjSelectorRef){
   if(FSstrBrowser=="Gecko"){
    if(TheEvent){
      var ThisIcon="FSdsIcon_"+FSobjDateRef.name;
      var rel=TheEvent.target;
      while(rel) {
        if((rel.id=="FSdateSelector")||(rel.id=="FSmainTable")||(rel.id==ThisIcon)) {
          break
        } else {
          rel=rel.parentNode
        }
      }
    };
    if(!rel){
      FSobjSelectorRef.style.visibility="hidden";
      FSblnIsShown=false
    };
    return
   } else {
    // ... non-Gecko
   }
  } else {
   FSobjSelectorRef=false
  }
};


So, what happens when you click on that drop-down is that its onchange wipes the
entire "panel" it's in to "update" the month view, using .innerHTML so all the
previous content, that we're still handling click event for, is removed from
the document.  Now, because of bug 635852 we stop dispatching click to content
that has been removed from the document, but instead that leads to hitting
this code:
http://hg.mozilla.org/mozilla-central/annotate/f8d27fe5d7c0/layout/base/nsPresShell.cpp#l6830
with targetContent == null so we dispatch to the document instead - whic
runs the script above, which lands in "if(!rel)" and promptly hides the new "panel"
with the updated month view.

The origin of that code is bug 360731:
https://bugzilla.mozilla.org/attachment.cgi?id=246468&action=diff#mozilla/layout/base/nsPresShell.cpp_sec1
which is for dispatching events for back/forward buttons on your mouse, IUIC.
I've verified that removing that block breaks my back/forward mouse buttons.
In that case though the mCurrentEventFrame is the ViewportFrame (which have null
content) so I think we could limit it to that case.

https://tbpl.mozilla.org/?tree=Try&rev=0a76a7b0e4e8
Note the click event on the document.  Chrome doesn't do that.
Attached patch fix+testSplinter Review
Attachment #739616 - Flags: review?(bugs)
Comment on attachment 739616 [details] [diff] [review]
fix+test


>           if (!eventTarget) {
>             nsCOMPtr<nsIContent> targetContent;
>             if (mCurrentEventFrame) {
>               rv = mCurrentEventFrame->
>                      GetContentForEvent(aEvent, getter_AddRefs(targetContent));
>             }
>             if (NS_SUCCEEDED(rv) && targetContent) {
>               eventTarget = do_QueryInterface(targetContent);
>-            } else if (mDocument) {
>+            } else if (mDocument && GetCurrentEventFrame()) {
>               eventTarget = do_QueryInterface(mDocument);
>               // If we don't have any content, the callback wouldn't probably
>               // do nothing.
>               eventCBPtr = nullptr;

Do we actually end up to this last else if() block ever?
GetCurrentEventFrame() updates mCurrentEventFrame and returns it, so
usually it just returns mCurrentEventFrame which is handled already above.
> Do we actually end up to this last else if() block ever?

Yes, it's needed for back/forward mouse buttons at least, see last paragraph
in comment 6.

> mCurrentEventFrame which is handled already above.

I don't see how it's handled.  We use mCurrentEventFrame to GetContentForEvent
which results in a null targetContent, so we enter this block.

The bug is that we enter this block also when mCurrentEventFrame is null
which the added check prevents.
But with the patch there are events we don't handle?
I guess I need to run this in a debugger or something.
Duplicate of this bug: 862587
Reproduced this bug in Firefox ESR 17.0.5 on Windows 7 and on Mac OS X. This did not occur in ESR 10.
Comment on attachment 739616 [details] [diff] [review]
fix+test

I don't think we want to stop events go through presshell.
We should prevent dispatching the click in
nsEventStateManager::CheckForAndDispatchClick
Attachment #739616 - Flags: review?(bugs) → review-
I would prefer the suggested fix because I think we should eventually remove that
else-block.  Events that we want to target at the document, e.g. the back/forward
mouse buttons, should be explicitly targeted to it.
Attached patch wallpaper+testSplinter Review
That said, I guess we can wallpaper the specific case of mouse clicks
for now though.
Attachment #740667 - Flags: review?(bugs)
events should still work even after document.removeChild(document.documentElement) so we should, 
IMO, move towards dispatching events to nsINodes, not nsIContent.
Comment on attachment 740667 [details] [diff] [review]
wallpaper+test

function log858459() looks odd. Perhaps logEvent ?

Could you perhaps add a comment near the timeout that it is needed since we
want to check that certain event is _not_ dispatched.
Attachment #740667 - Flags: review?(bugs) → review+
> IMO, move towards dispatching events to nsINodes, not nsIContent.

I agree with that; making HandleEventWithTarget take a nsINode* and
changing mCurrentEventContent to nsINode etc.  It's just this crazy
else-block that I don't like.
Comment on attachment 740667 [details] [diff] [review]
wallpaper+test

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 635852
User impact if declined: minor web compat issues on some sites, probably rare
Testing completed (on m-c, etc.): not yet in Nightly, green on m-i
Risk to taking this patch (and alternatives if risky): there's a low risk that it will cause other web compat issues.  The alternative is to back out bug 635852 which I think would be zero risk wrt regressions.
String or IDL/UUID changes made by this patch: none
Attachment #740667 - Flags: approval-mozilla-beta?
Attachment #740667 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/cb2cfeb30fde
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
(In reply to Mats Palmgren [:mats] from comment #21)
> Comment on attachment 740667 [details] [diff] [review]
> wallpaper+test
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): bug 635852
> User impact if declined: minor web compat issues on some sites, probably rare
> Testing completed (on m-c, etc.): not yet in Nightly, green on m-i
> Risk to taking this patch (and alternatives if risky): there's a low risk
> that it will cause other web compat issues.  The alternative is to back out
> bug 635852 which I think would be zero risk wrt regressions.
> String or IDL/UUID changes made by this patch: none

Given the baketime this has had and the outlined risk that it may cause other webcompat issues, it does not fit well to rush this up in beta.

 Mats, is there an alternative approach to fix this issue without the risk of web compat regressions in case we had more time ?

We cannot backout bug 635852 since we have shipped with it Fx20 and due to its security nature.
The patch which landed should be reasonable safe, safer than the other approach.
Attachment #740667 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(In reply to Olli Pettay [:smaug] from comment #24)
> The patch which landed should be reasonable safe, safer than the other
> approach.

Thanks Olli ! I discussed this with :akeybl, and we are going to uplift this earlier than later in beta , given there is no alternative here.

We will backout if the web regressions are worse than what we currently have.

Olli, can you please help with the needed branch landing as :mats must be away & I am going to hold-off on beta build till this lands cleanly ?
I'll land soon. Compiling and testing first.
Comment on attachment 740667 [details] [diff] [review]
wallpaper+test

missed on actually giving an a+ here.

Olli, thanks for the landing !
Attachment #740667 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Adding verifyme to do some exploratory testing around drop-down select menus.
Keywords: verifyme
We did some exploratory testing with websites using drop-down select menus in Firefox 21.0b4 and did not find any regressions. It's not a strict verification of this bug but I feel with this testing and the in-testsuite coverage it's safe enough to call this verified for Firefox 21. That said, I think we should continue to track this post-release and monitor our usual feedback channels.
Are there any plans to fix this in ESR 17.0.5?
Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:23.0) Gecko/20130428 Firefox/23.0
Mozilla/5.0 (X11; Linux x86_64; rv:23.0) Gecko/20130428 Firefox/23.0

Verified as fixed on Windows 7 x64 and Ubuntu 12.04 x86_64 with latest Nightly 23.0a1 (Build ID: 20130429030926) using the provided testcase and also following the steps to reproduce in the description.
Additional real-world testcase:

http://jquerytools.org/demos/dateinput/customize.html

Click any of the three textboxes to display the dateinput calendar control. Select a value in either the month or the year dropdown. 

Actual results: 

The dateinput calendar is hidden.

Expected results:

The calendar should be redrawn to show the selected month / year. 

I am new to bugzilla and the firefox rapid release process. I see that the status-firefox21 tracking flag is set to verified, so I am thinking the fix will be in the next release. Is there  a good way to track the expected date for the next release?
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:23.0) gecko/20130503 Firefox/23.0

Verified on Mac OS, in addition to Ubuntu and Windows in comment 33 (latest Nightly).
Status: RESOLVED → VERIFIED
Can this be backported to Firefox ESR 17 since it is a functional regression introduced in a service pack?
(In reply to Peter Agar from comment #37)
> Can this be backported to Firefox ESR 17 since it is a functional regression
> introduced in a service pack?

The regression appeared in FF20, so ESR isn't affected. And the next ESR is based on FF24 which has already the fix.
You need to log in before you can comment on or make changes to this bug.