translate3d: dropdown selector selects wrong element

UNCONFIRMED
Unassigned

Status

()

UNCONFIRMED
3 years ago
3 years ago

People

(Reporter: mozillabmw, Unassigned)

Tracking

({testcase})

36 Branch
x86_64
Linux
testcase
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

3 years ago
Created attachment 8581558 [details]
firefox36 screenshot of original page with debug overlay showing the offset

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:36.0) Gecko/20100101 Firefox/36.0
Build ID: 2015022000

Steps to reproduce:

opened http://zq1.de/bug/firefoxbug (which is a minimal reproducer extracted from the OpenStack horizon dashboard page which is using bootstrap) in Firefox 31.5, 35, 36
on openSUSE and CentOS and Debian with an icewm or twm  windowmanager (bug does not occur with KDE or i3)
selected an entry (m1.xlarge) in the dropdown selector


Actual results:

another entry was selected (m1.large)


Expected results:

the clicked entry should have been selected

Comment 1

3 years ago
WFM with FF36 on Win 7, maybe only Linux issue.
Component: Untriaged → Layout: Form Controls
Keywords: testcase
Works on Mac too, not surprisingly, since even on Linux it works with some windowmanagers...
Flags: needinfo?(tnikkel)
The testcase has translate3d(0px,0px,0px), did you intend one of them to be non-zero? I couldn't reproduce on mac or linux, with the original testcase or changing the y translate to 50px.
Flags: needinfo?(tnikkel) → needinfo?(mozillabmw)
(Reporter)

Comment 4

3 years ago
(In reply to Timothy Nikkel (:tn) from comment #3)
> The testcase has translate3d(0px,0px,0px), did you intend one of them to be
> non-zero?

This is from a recent addition to the highly popular bootstrap CSS framework:
https://github.com/twbs/bootstrap/pull/13649/files

so yes, 0,0,0 is intentional there.

As mentioned before, on Linux it is only reproducible with some windowmanagers.
Those simple ones (icewm, twm) that do not link libGL
Flags: needinfo?(mozillabmw)
What happens if instead of the translate3d you put opacity: 0.9 or make it absolute position with z-index: 1?
Flags: needinfo?(mozillabmw)
(Reporter)

Comment 6

3 years ago
It works fine (so no bug), when I only put either of
opacity: 0.9
z-index: 1
transform: translate(0, 0, 0)

into the
<style type="text/css"> 
      .modal-dialog { ... }
</style>
Flags: needinfo?(mozillabmw)
Karl suggested that the offset looks like the difference between the windows bounds with and without decorations (titlebar, border etc). Which seems like a very good guess. We do use a different code path when computing event coords when a transform is involved. So I'm guessing either nsWindow::WidgetToScreenOffset (http://mxr.mozilla.org/mozilla-central/source/widget/gtk/nsWindow.cpp#1846) or nsBaseWidget::GetBounds (http://mxr.mozilla.org/mozilla-central/source/widget/nsBaseWidget.cpp#1341) returns a value that doesn't take into account decorations when it should or vice-versa, because the window manager is providing the wrong values.
(Reporter)

Comment 8

3 years ago
(In reply to Timothy Nikkel (:tn) from comment #7)
> Karl suggested that the offset looks like the difference between the windows
> bounds with and without decorations (titlebar, border etc).

indeed, switching firefox to fullscreen by pressing F11, makes the offset go away.
So perhaps those window managers don't properly implement gdk_window_get_origin.
(Reporter)

Comment 10

3 years ago
I used firefox -d "gdb --args"
and
run
^C
break nsBaseWidget::GetBounds
cont
display mBounds
cont

to see what values were going through there, but found that those values in gdk_window_get_origin were always x=0 y=0
and GetBounds had some more variation, but it also looked the same for working and non-working cases.

1: mBounds = {<mozilla::gfx::BaseRect<int, nsIntRect, nsIntPoint, nsIntSize, nsIntMargin>> = {x = 0, y = 0, width = 1440, height = 782}, <No data fields>}
1: mBounds = {<mozilla::gfx::BaseRect<int, nsIntRect, nsIntPoint, nsIntSize, nsIntMargin>> = {x = 0, y = 0, width = 0, height = 0}, <No data fields>}
1: mBounds = {<mozilla::gfx::BaseRect<int, nsIntRect, nsIntPoint, nsIntSize, nsIntMargin>> = {x = 11, y = 126, width = 106, height = 98}, <No data fields>}

where the first one seems to be the full window and the last one the expanded dropdown element
(In reply to Bernhard M. Wiedemann from comment #10)
> to see what values were going through there, but found that those values in
> gdk_window_get_origin were always x=0 y=0

Those values should not be 0,0 if there are window decorations on the window. They should be the screen position of the top-left of the browser window minus window decorations. So if this testing was done with one of the window managers that shows the bug that indicates why the bug is happening.
(Reporter)

Comment 12

3 years ago
Maybe gdb was distorting results by interrupting firefox.
I did another round of testing with a patched gtk2
that outputs gdk_window_get_origin coords
and found that in both working and non-working cases
it gives the absolute coordinates from the top-left of the screen
to the top-left of the dropped-down widget

http://www.zq1.de/bug/firefoxbug-icewm-8-141.png
http://www.zq1.de/bug/firefoxbug-gnome-8-199.png

The dropdown seems to be a separate window without decorations
(e.g. gimp only captures this part)
The value of gdk_window_get_origin for the main Firefox window, not the dropdown window is what I'm suggesting might be incorrect.
(Reporter)

Comment 14

3 years ago
here is what it returns for the main Firefox window
firefox icewm:
IA__gdk_window_get_origin x=0 y=20
firefox in gnome (with gnome-menu-bar on top)
IA__gdk_window_get_origin x=0 y=66

so looks good.
Also, if this was wrong, there should be more off, than just the dropdown in translate3d mode
Hmm, okay then I'm not sure. If you're looking to debug this more you can trace through nsLayoutUtils::GetEventCoordinatesRelativeTo and see if/when things go wrong.
(Reporter)

Comment 16

3 years ago
nsLayoutUtils::GetEventCoordinatesRelativeTo was a good starting point.
In there is an if condition that only gets true when translate3d (or translatez) is used.

from there, I traced the differences into nsIFrame::GetTransformMatrix
and toplevel->GetClientBounds(toplevelScreenBounds)
via nsWindow::GetClientBounds
to  nsWindow::GetClientOffset()
and in there, in the broken case, the 2nd if condition is true and return nsIntPoint(0, 0); happens
while in the working case, proper offset coords are returned below it

I could see that mShell and window are not NULL,
thus the gdk_property_get must be the one returning zero
Thank you.

Hmm, so it seems those wm's don't respond to _NET_FRAME_EXTENTS. Maybe we can work around that. If that "if" fails returning the different between WidgetToScreenOffset and the TopLeft() of GetBounds should be the client offset for toplevel windows (which we should only have here).
I can make a patch to do comment 17 if you'd like to try it out.
Flags: needinfo?(mozillabmw)
(Reporter)

Comment 19

3 years ago
I would be interested in testing it, though I'm not convinced where the best place is for fixing this bug (e.g. windowmanagers could be extended to tell about their frame size)

I already built and ran a patched firefox for debugging, so should be easy now.
Flags: needinfo?(mozillabmw)
Oh, you have your own patch that's working? If you upload it here we can get it committed to Firefox.

I'm not sure but I'm guessing responding to _NET_FRAME_EXTENTS is not required, so a workaround in Firefox seems reasonable.
Created attachment 8586622 [details] [diff] [review]
maybe patch

I think this might work. I haven't compiled it.
Flags: needinfo?(mozillabmw)
(Reporter)

Comment 22

3 years ago
In my firefox-36 src, I had to use WidgetToScreenOffset to make it build
but then it worked great.
Flags: needinfo?(mozillabmw)
(Reporter)

Comment 23

3 years ago
Did some more testing and found that the patch might have unwanted side-effects.
From my debug prints, I can see that the old 0,0 fallback was also hit on other windowmanagers
and outside the translate3d codepath.
The visible effect is that the newly popped up dropdown is first off to the bottom-right
by a factory of 2 (in my example, that is roughly x=8 y=105)
... so not quite as easy.
Created attachment 8587006 [details] [diff] [review]
maybe patch

Looks like popup window coordinates are parent relative, but mIsTopLevel is confusingly true for them.
Flags: needinfo?(mozillabmw)
(Reporter)

Comment 25

3 years ago
This one works even better. In the modern-windowmanager case,
there are no bad calls to the fallback left
(only 3 calls before the main window comes up, and they return 0,0 anyway)
and in the icewm case it still works fine.
Flags: needinfo?(mozillabmw)
Attachment #8586622 - Attachment is obsolete: true
Attachment #8587006 - Flags: review?(karlt)
Comment on attachment 8587006 [details] [diff] [review]
maybe patch

(In reply to Timothy Nikkel (:tn) from comment #24)
> Looks like popup window coordinates are parent relative, but mIsTopLevel is
> confusingly true for them.

They are actually relative to the screen or root window, which is why we
needed https://hg.mozilla.org/mozilla-central/rev/a35b6341335c

Most popup windows do not need to be queried but mNoAutoHide popup windows,
especially those with eBorderStyle_title, may have decorations, so excluding
all eWindowType_popup windows isn't really the right solution.

Removing review request until this is better understood.
Attachment #8587006 - Flags: review?(karlt)
(In reply to Bernhard M. Wiedemann from comment #23)
> Did some more testing and found that the patch might have unwanted
> side-effects.
> From my debug prints, I can see that the old 0,0 fallback was also hit on
> other windowmanagers
> and outside the translate3d codepath.
> The visible effect is that the newly popped up dropdown is first off to the
> bottom-right
> by a factory of 2 (in my example, that is roughly x=8 y=105)
> ... so not quite as easy.

I haven't been able to reproduce with metacity or kwin.
Was this in gnome?
Which popup behaved this way?

I wonder whether the problem might be due to GetClientOffset() being called
before the window has been moved to the position specified by mBounds.

Would you be able to add logging, please, to find the state of mIsShown and
mNeedsMove when things go wrong?
Flags: needinfo?(mozillabmw)
Depends on: 1180008
(Reporter)

Comment 28

3 years ago
I cannot find time to compile firefox atm, but so far I reproduced the bug with icewm and twm (maybe fvwm would also show it)
https://bugzilla.opensuse.org/show_bug.cgi?id=919656#c18
suggests that those old WMs do not implement version 1.3 of the window-manager spec, as more modern WMs do.
Flags: needinfo?(mozillabmw)
You need to log in before you can comment on or make changes to this bug.