115253 - [FIX]Comboxes can't be opened in DIV with scrolls !

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect, P1)

Description

[Stephane Barbaray]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:0.9.6+)
Gecko/20011213
BuildID:    2001121313

Try to click on the comboxes they won't open at all, this will work only if the
Div container have overflow not set to scroll or auto

Reproducible: Always
Steps to Reproduce:
1. wait for comboboxes to appear instead of "Veuillez patienter" (please wait in
french)
2. Try to open any of the comboboxes

Actual Results:  nothing!!!

Expected Results:  combo's list shown!

This never worked since at least this summer

Comment 1

[Boris Zbarsky [:bzbarsky]]

seen on linux too.

Comment 2

[rods (gone)]

This probably doesn't have anything to do with what you changed, but could you 
take a look?

Comment 3

[Stephane Barbaray]

Well... the only things that we changed since the last summer is the overflow
set to scroll instead of none and a fixed height in the div, so it could have
never ever worked because I never tried it like that before! Furthermore setting
actually the 'overflow' back to 'none' will make the thing working again but I'd
need those combos in a scrolled div finally... :->

Comment 4

[John Keiser (jkeiser)]

Hmmm.  No funky assertions showing up on a debug build.  This will take some
printfs I think.

Comment 5

[Boris Zbarsky [:bzbarsky]]

*** Bug 116091 has been marked as a duplicate of this bug. ***

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: testcase from bug 116091

Comment 7

[Shawn Neumann]

Any luck with this one?  We're having problem with a web app that won't let us
open select boxes if they're DIV as describe initially.

Comment 8

[John Keiser (jkeiser)]

Attachment: Illuminating Testcase

Comment 9

[John Keiser (jkeiser)]

The problem that we're having here is that screenX and screenY in the mouse
event are actually *different* when the mouse enters the scrollbar.  This
happens anytime you have a <select> inside of a scrolling <div>.  (Just simple
text did not cause this to happen.)

The exact amount of the difference, in fact, is the (x,y) position of the <div>
within the window (or possibly the <select>, not sure which).  This (x,y) is
being subtracted from the real screenX / screenY somewhere.

This causes the combobox not to open because it does not detect that the click
was in the combobox.  All click and mouse move events, in fact, will be wrong.

rods, bz, does this ring any bells?

Comment 10

[Boris Zbarsky [:bzbarsky]]

jkeiser, as I move my mouse over that testcase screenX/Y vary continously....
the only discontinuous coordinates I see are layerX/Y.

And yes, I _do_ see the select not opening.

Comment 11

[John Keiser (jkeiser)]

Hmm, I still see the discontinuity.  Please note that it's only about a
discontinuity of 60 (twips?).  It is most noticeable in the Y; the X is so close
to the left edge that it is imperceptible in this case.  When I first moved the
mouse around just now, in fact, I didn't see it (though I may just not have
looked hard, it's hard to spot), but I did see it soon thereafter.  Try moving
it around for a while, then bringing the mouse down into the combobox slowly
from above.

And it is definitely the cause of the problem in the code, at least on my system
(i.e. if I make that function return true, it will drop down, though that breaks
other things).  The code gets all the way up to this check and then this test
fails just before actually dropping down the combobox.

Could be Linux-only, but then that would mean there was a separate problem on
another platform with exactly the same symptoms.  That seems unlikely to me.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Ah, OK.  I see it too....  It sounds like something isn't walking up the views
properly to get the absolute offset?

Comment 13

[John Keiser (jkeiser)]

I am not familiar with how (or where) we calculate that stuff, but here's a
hypothesis about how it usually works:

1. We walk up the views to find the position relative to the document.
2. We add the document's relative position to the browser.
3. We add the browser's relative position to the screen.

At a guess, I'd say (1) is stopping at the DIV (or the select?) and the other 2
steps are still working correctly.

Comment 14

[John Keiser (jkeiser)]

Just did some testing; it appears that it is the DIV's top/left position that is
"subtracted" from screenX/Y, not the select's.

If you don't have a select in there, though, it doesn't do this.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Want something even cooler?  In a debug build I do _not_ see the discontinuity.
 The numbers are correct.

However, when I click on the select and drop it down using alt-down the popup is
just shifted up a bit in the opt build (as expected).  In the debug build, the
popup is outside the browser window, down and to the right.

Comment 16

[John Keiser (jkeiser)]

This is a big thing, but there are other big things on the list too.  1.1.

Comment 17

[Andrew Schultz]

*** Bug 166706 has been marked as a duplicate of this bug. ***

Comment 18

[Jeremey Barrett]

Is there a fix for this on the horizon? 1.1 is out (see comment #16) and it's
still broken.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

OK.  So the root cause of the problem was the following code in the "we have a
parent widget" branch:

-	 viewOffset.x += po.x;
-	 viewOffset.y += po.y;

and then if we have a widget

-	   aAbsoluteTwipsRect.x += po.x - bounds.x;
-	   aAbsoluteTwipsRect.y += po.y - bounds.y;

followed by

-      aAbsoluteTwipsRect.x += viewOffset.x;
-      aAbsoluteTwipsRect.y += viewOffset.y;

The net effect of this was to count "po" twice in the origin calculation.  This
caused the y-coord we calculated for the rect's origin to be wrong, with the
ensuing chaos.

So I fixed that.... then moved to nsCOMPtr's for the nsIWidget, then figured
out that the nsIScrollableView code was completely wrong (it always used
containingView, which is never updated after it's first gotten!) and utterly
unnecessary.

Then I decided I might as well rewrite the whole thing from scratch.  The basic
idea of the new code is to get the closest enclosing view for the frame, then
to walk up the view hierarchy till we get to a view with a widget. Once we
reach such a beast, grab its screen position and break out.

This shouldn't regress bug 35291 (I wish that had a testcase or something!),
since we start by checkin the widget of the frame's containing view, not the
widget of its parent (as the code did pre-35291)

Comment 20

[Boris Zbarsky [:bzbarsky]]

Alternate approach would be to leave the code as-is except for:

-	   aAbsoluteTwipsRect.x += po.x - bounds.x;
-	   aAbsoluteTwipsRect.y += po.y - bounds.y;
+	   aAbsoluteTwipsRect.x -= bounds.x;
+	   aAbsoluteTwipsRect.y -= bounds.y;

Comment 21

[Boris Zbarsky [:bzbarsky]]

I meant the "we have a parent view" branch in comment 19.

Comment 22

[Jeremey Barrett]

My vote: fix it, then fix it Right (if the first fix isn't Right). :)

Comment 23

[John Keiser (jkeiser)]

Comment on attachment 97917 [details] [diff] [review]
Proposed patch

The patch looks good for what it does, but I am rather surprised that
GetPosition() works relative to screen coordinates rather than scrolled
coordinates.  Are you sure this is the case?  I mean, besides the fact that
this patch works :)

Comment 24

[Boris Zbarsky [:bzbarsky]]

I'm pretty sure that that's the case.  But I'll double-check with roc... (as a
note, when I tried some scrolled divs everything was peachy as long as I did not
add in scroll positions.  When I did, things broke).

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 97917 [details] [diff] [review]
Proposed patch

r/sr=roc+moz

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

GetPosition() suffices because of the structure of views in scrolling elements.
There is a scroll port view with a widget, which contains a scrolled view. When
the element is scrolled down by N units, the scrolled view is at an offset of -N
to the scroll port view. GetPosition() picks this up and gets you into the
coordinate space of the scroll port before you look at its widget.

Comment 27

[John Keiser (jkeiser)]

Comment on attachment 97917 [details] [diff] [review]
Proposed patch

r=jkeiser

Comment 28

[Boris Zbarsky [:bzbarsky]]

taking

Comment 29

[Boris Zbarsky [:bzbarsky]]

fixed for 1.2b

Comment 30

[Boris Zbarsky [:bzbarsky]]

*** Bug 177469 has been marked as a duplicate of this bug. ***

Comment 31

[Boris Zbarsky [:bzbarsky]]

*** Bug 96756 has been marked as a duplicate of this bug. ***

Comment 32

[Intermittent Failures Robot]

1 failures in 3319 pushes (0.0 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• mozilla-central: 1

Platform and build breakdown:

• linux1804-32-shippable-qr: 1
  • opt: 1

For more details, see:
https://treeherder.mozilla.org/intermittent-failures/bugdetails?bug=115253&startday=2021-12-06&endday=2021-12-12&tree=all

Comment 33

[Intermittent Failures Robot]

1 failures in 3317 pushes (0.0 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• autoland: 1

Platform and build breakdown:

• linux1804-64-asan-qr: 1
  • opt: 1

For more details, see:
https://treeherder.mozilla.org/intermittent-failures/bugdetails?bug=115253&startday=2022-05-09&endday=2022-05-15&tree=all