190767 - <select> menu does not work

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Thomas]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.3b) Gecko/20030125
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.3b) Gecko/20030125

The menu with "Aujourd'hui", "Ce soir", "Demain" etc does not work.
This function works fine with Mozilla 20030121 but not greater!

Reproducible: Always

Steps to Reproduce:
1. Select any links
2.
3.

Actual Results:  
No effect when select a link

Comment 1

[Boris Zbarsky [:bzbarsky]]

form controls

Comment 2

[Ruslan Ismailov]

Such behaviour is a resilt of fremeset definition:

<FRAME name=VerwTxtBelMenu marginWidth=0 marginHeight=0
src="VerwGfxBelFrameDefsFR_files/VerwGfxBelMenuFR.htm" frameBorder=NO scrolling=no>

If I remove scrolling=no, everything would be fine.

Comment 3

[viipale+bugzilla]

I noticed the same bug on another website, using Mozilla or Phoenix builds
2003-01-25 on Windows 2000. The website is private, so I can't give the url. A
testcase is available in here: http://194.29.195.131/mozillatest/
The select box in the leftmost frame has scrolling="no", the right one has
scrolling="yes". The left one doesn't work.

The bug occurs only if you select an option from the dropdown menu by clicking
the mouse. If you select it with the Enter key, it works fine.

Also tried other form elements, but this seems to effect select boxes only.

Comment 4

[Boris Zbarsky [:bzbarsky]]

We need a regression date range.

Comment 5

[Boris Zbarsky [:bzbarsky]]

This seems to have regressed between 2003-01-21-08 and 2003-01-21-22.  Of the
checkins in that range, the one for bug 185889 is most suspicious-looking. 
Could someone try a local backout of that and see whether that helps?

Comment 6

[Ruslan Ismailov]

I undone patch for bug 185889 and error gone. So I think comment #5 is correct.

Comment 7

[Ruslan Ismailov]

I found a source of this bug, but I not sure that I understand, what is happened.

In file layout/html/base/src/nsPresShell.cpp there is new code (after patch)

          if (!targetElement) {
            NS_IF_RELEASE(mCurrentEventContent);
            mCurrentEventFrame = nsnull;

last assigment seems to be bad. Let me prove this, code before patch for bug
185889 was:

while (targetElement &&
             !targetElement->IsContentOfType(nsIContent::eELEMENT)) {
        nsIContent* temp = targetElement;
        temp->GetParent(*getter_AddRefs(targetElement));
        targetFrame = nsnull;}

So if targetFrame become nsnull only if while stuff worked at least 1 time. So
setting nsnull to mCurrentEventFrame is incorrect. If at wery start
!targetElement was true, than new code produce mCurrentEventFrame = nsnull, when
old code does not. 

Comment 8

[Ruslan Ismailov]

Attachment: Fix frames scroll=no events case

Well, I see only 1 correct solution: move mCurrentEventFrame = nsnull; inside
while (only by that way we could obtain correct calculation).

Comment 9

[John Keiser (jkeiser)]

Comment on attachment 112820 [details] [diff] [review]
Fix frames scroll=no events case

This was done deliberately; the change was to target the event at the text
frame but make JS handlers fire on content.

Comment 10

[John Keiser (jkeiser)]

BTW, I do appreciate the patch, and the fact that you are trying to get at the
root cause.  The problem you are seeing is most likely the strange disparity in
that we try to leave the *frame* the same as it was initially while the
*content* event (JS handlers) is fired at the nearest element parent.  Changing
this, for example, should break selection--try clicking and dragging.  We keep
the original frame around so that the recipient of the event knows exactly where
the event was targeted.

Comment 11

[Ruslan Ismailov]

John, could you point me to a testcase for selection break, because testcase in
bug 185889 working fine. Even more, what could be selected, if           

mCurrentEventFrame->GetContentForEvent(mPresContext, aEvent,
                                                 getter_AddRefs(targetElement));

will not return targetElement? (Well, in "normal" case, when targetElement was
defined by this, selection worked).

Or there should be another way to resolve case, if mCurrentEventFrame have no
content.

Comment 12

[Ruslan Ismailov]

Attachment: Proposed fix v.1.1

O'k. In case when mCurrentEventFrame have no content for event, trying to do
mCurrentEventFrame = nsnull lead to bug. This case is not covered by comment
#10  and herefore I suggest to check it before doing something. This patch
covered only this exact case and newer lead problems when *content* exists.

Comment 13

[Bill Gerba]

There is similar behavior with the <SELECT> dropdown at the top of
www.ucomics.com, which does not have frames, with the last (at least 2) weeks of
1.3 beta on i686 Linux build.

Comment 14

[Ruslan Ismailov]

I think http://www.ucomics.com is another bug, Linux-specific (under windows
everythink o'k). It was reported as bug 185216. 

This bug with scrolling=yes appeared under Win and Linux.

Comment 15

[Ruslan Ismailov]

Attachment: Patch v. 1.2

I found more explicit way to patch this case. Sourse of trouble is in
mCurrentEventContent variable, wich could be not existant in this case. And if
we have no other content, than mCurrentEventFrame = nsnull; is incorrect. So I
propose to insert only 1 checking for this.

This patch seems to be correct way to obtain all functionality and resolve
unparity from comment #10.

Comment 16

[John Keiser (jkeiser)]

Have you tested that patch?  NS_IF_RELEASE() should *always* set
mCurrentEventContent to null, in which case your if() will never occur.
             NS_IF_RELEASE(mCurrentEventContent);
-            mCurrentEventFrame = nsnull;
+            // ifmCurrentEventContent does not exist than
+            // let mCurrentEventFrame doing all business. Bug 190767.
+            if (mCurrentEventContent) {mCurrentEventFrame = nsnull;}

The theory of this patch is sound, however, and a good idea.  However, what
should happen when no mCurrentEventContent is not there is, we shouldn't even do
this whole reparenting thing--just wrap the whole deal in an
if(mCurrentEventContent).  Same effect but it should be clearer.

We need to investigate *why* mCurrentEventContent is null, however.  Since this
happens when scrolling=no but not when scrolling=yes, there is a more
fundamental problem.  Minimal testcase follows.

Comment 17

[John Keiser (jkeiser)]

Attachment: Minimal Testcase

This uses body style="overflow: hidden", which is the same deal as frame with
scrolling=no.

Comment 18

[Ruslan Ismailov]

Well, patch is working and I use it. Even more, bug 191154 have another example
of same situation (I think I made a dupe aften some additional investiogation). 

Maybe I should move if (mCurrentEventContent) {mCurrentEventFrame = nsnull;}
before NS_IF_RELEASE? 

Comment 19

[Ruslan Ismailov]

Attachment: Patch v 1.3

Well, I see source of trouble with unexisted content in line 6031:

if (!mCurrentEventContent) {
  // fallback, call existing codes
    mDocument->GetRootContent(&mCurrentEventContent);

But were is quarantee than this one give real content?

Comment 20

[John Keiser (jkeiser)]

OK, looking through the code, some of what I did there is useless. 
mCurrentEventContent is going to be null always in that function--or should be.
 The reason mCurrentEventContent can be null is that we don't always fill in
mCurrentEventContent (in fact, I can't find where we fill it in at *all* in this
case.  What you're seeing is most likely left over from the previous event
(which is also bad!).  I was mistakenly thinking we had landed my patch for bug
148542, which would clean that up.

As to line 6031, that should only run for key events, not mouse events--are you
sure that is getting called during this particular mouse event?  That would
indicate bigger problems.

For some reason this shows up fixed in my debug build.  I am trying to figure
out what I did to make that happen.  We need to determine the real problem
here--why are we getting an event that isn't targeted at an element or an
ancestor of an element for this case?

Comment 21

[John Keiser (jkeiser)]

OK, I have done some analysis and found that the real problem here is that the
select is getting a blur / focus on mousedown.  This closes the dropdown and
prevents the selection from being made, and causes subsequent problems to
happen.  I have traced this back to gLastFocusedDocument not being set, which is
because NS_GOTFOCUS is not being fired, and that is where the problem stands
tonight.  I must go to bed.  This is obviously going to be tightly related to
bug 191154.

Comment 22

[John Keiser (jkeiser)]

Ruslan: OK, now I understand why your patch fixes this bug.  When there are no
scrollbars, a focus event is targeted at a window frame which has no content at
all.  What we need to do is wrap an if(targetElement) around the whole while
loop and the if (!targetElement) stuff so that we just don't try to retarget the
event if the frame has no content.  We also need to look into giving this stray
frame some content :)

The reason for the stuff inside the if (!targetElement) block is to handle
documents consisting of just a textnode, in case such exist--we still don't want
to fire events at them.

Comment 23

[Ruslan Ismailov]

If I understand, it was patch v 1.1 ;-). 

Comment 24

[John Keiser (jkeiser)]

That's precisely the one.  Guess we skipped over that one before I got to look.
 Testing.

Comment 25

[John Keiser (jkeiser)]

Comment on attachment 112852 [details] [diff] [review]
Proposed fix v.1.1

r=jkeiser, but I want to check this in--I will correct the spelling and stuff.

Thanks for the excellent work.

Comment 26

[Brian Ryner (not reading)]

Comment on attachment 112852 [details] [diff] [review]
Proposed fix v.1.1

looks ok to me. sr=bryner.

Comment 27

[Asa Dotzler [:asa]]

Comment on attachment 112852 [details] [diff] [review]
Proposed fix v.1.1

a=asa (on behalf of drivers) for checkin to 1.3beta.

Comment 28

[John Keiser (jkeiser)]

Fix checked in.

Comment 29

[viipale+bugzilla]

Confirming fixed. Phoenix build 2003-02-01 on WinXP.