858459 - drop-down select menu closes without selecting in Firefox 20.0

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[info]

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.

Comment 1

[Loic]

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

Comment 2

[Alice0775 White]

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

Comment 3

[Mats Palmgren (inactive)]

I'm investigating...

Comment 4

[mjh563]

Attachment: testcase

This is a simplified testcase that has the same bug.

Comment 5

[bhavana bajaj [:bajaj]]

(In reply to Mats Palmgren [:mats] from comment #3)
> I'm investigating...

Hey Mats, any update here ?

Comment 6

[Mats Palmgren (inactive)]

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

Comment 7

[Mats Palmgren (inactive)]

Attachment: An approximation of what the site does

Note the click event on the document.  Chrome doesn't do that.

Comment 8

[Mats Palmgren (inactive)]

Attachment: fix+test

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 10

[Mats Palmgren (inactive)]

> 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.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

But with the patch there are events we don't handle?
I guess I need to run this in a debugger or something.

Comment 13

[Peter Agar]

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 14

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 15

[Mats Palmgren (inactive)]

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.

Comment 16

[Mats Palmgren (inactive)]

Attachment: wallpaper+test

That said, I guess we can wallpaper the specific case of mouse clicks
for now though.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

events should still work even after document.removeChild(document.documentElement) so we should, 
IMO, move towards dispatching events to nsINodes, not nsIContent.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 19

[Mats Palmgren (inactive)]

> 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 20

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cb2cfeb30fde

Comment 21

[Mats Palmgren (inactive)]

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

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cb2cfeb30fde

Comment 23

[bhavana bajaj [:bajaj]]

(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.

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

The patch which landed should be reasonable safe, safer than the other approach.

Comment 25

[bhavana bajaj [:bajaj]]

(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 ?

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

I'll land soon. Compiling and testing first.

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/eb4232ef382c

Comment 28

[Olli Pettay [:smaug][[e-mail]]]

https://hg.mozilla.org/releases/mozilla-beta/rev/023a7866916b

Comment 29

[bhavana bajaj [:bajaj]]

Comment on attachment 740667 [details] [diff] [review]
wallpaper+test

missed on actually giving an a+ here.

Olli, thanks for the landing !

Comment 30

[u279076]

Adding verifyme to do some exploratory testing around drop-down select menus.

Comment 31

[u279076]

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.

Comment 32

[Peter Agar]

Are there any plans to fix this in ESR 17.0.5?

Comment 33

[Maria Grazia Alastra [:mga]]

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.

Comment 34

[bobd]

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?

Comment 35

[Mats Palmgren (inactive)]

https://wiki.mozilla.org/Releases

Comment 36

[Virgil Dicu [:virgil] [QA]]

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).

Comment 37

[Peter Agar]

Can this be backported to Firefox ESR 17 since it is a functional regression introduced in a service pack?

Comment 38

[Loic]

(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.