Closed
Bug 1075670
Opened 10 years ago
Closed 9 years ago
[e10s] event.screenX and event.screenY is wrong
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla40
People
(Reporter: alice0775, Assigned: handyman)
References
Details
Attachments
(14 files, 41 obsolete files)
158 bytes,
text/html
|
Details | |
151.90 KB,
image/jpeg
|
Details | |
402 bytes,
text/html
|
Details | |
256.11 KB,
text/plain
|
Details | |
5.27 KB,
patch
|
Details | Diff | Splinter Review | |
9.57 KB,
patch
|
Details | Diff | Splinter Review | |
1.68 KB,
patch
|
Details | Diff | Splinter Review | |
6.99 KB,
patch
|
Details | Diff | Splinter Review | |
7.39 KB,
patch
|
Details | Diff | Splinter Review | |
4.95 KB,
patch
|
Details | Diff | Splinter Review | |
1.22 KB,
patch
|
Details | Diff | Splinter Review | |
5.04 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
1.59 KB,
patch
|
Details | Diff | Splinter Review | |
32.26 KB,
patch
|
billm
:
review+
jlorenzo
:
qa-approval+
|
Details | Diff | Splinter Review |
Steps To Reproduce: 1. Start e10s 2. Open ScratchPad and make sure chrome privilege. 3. Pase the following code to ScratchPad var script = 'data:application/javascript,' + encodeURIComponent('addEventListener("mouseup", function(event) {sendSyncMessage("test_mouseup", {}, {event: event}); }, false);'); window.messageManager.loadFrameScript(script, true); window.messageManager.addMessageListener("test_mouseup", function(msg){handleEvent(msg.objects.event)} ); function handleEvent(event) { alert(event.screenX + ", " + event.screenY); } 4. Click on anywhere of contents area Actual Results: event.screenX and event.screenY is not screen coordinates. Expected Results: event.screenX and event.screenY should be screen coordinates.
Reporter | ||
Comment 1•10 years ago
|
||
Err
> 3. Pase the following code to ScratchPad
3. Paste the following code to ScratchPad
Comment 2•10 years ago
|
||
Not really events related. Events just use the coordinates from widget + offset to layout stuff. Are we not updating WidgetToScreenOffset() in the child process correctly?
Component: DOM: Events → DOM
Reporter | ||
Comment 3•10 years ago
|
||
Simple STR 1. Start e10s 2. Open attached html 3. Click contents area
Updated•10 years ago
|
Assignee: nobody → davidp99
I assume this is what's been trashing my mouse gestures. I drew out some data I was getting back from mousemove events screenX and screenY - red line is the raw points, green line is an approximation of the actual movement, got from throwing out large deltas. The offset of the bad co-ordinates seems linked to the content area's coordinates on the screen - ie, the closer the browser window is to the top left of the screen, the smaller the margin of error. Hope that's new/useful information. If you already knew this, please do enjoy the pretty colours.
Assignee | ||
Comment 5•10 years ago
|
||
Thanks all. Alice's test and the smeared image make it pretty clear what was happening. This patch uses a different widget function to generate screen coordinates. The patch uses some of my work in bug 1044182, which is part of bug 1092630, so the patch would need to wait for that to be checked in to land. The only interesting issue here is WidgetToScreenOffset, which doesn't report actual screen coordinates in remote processes. This is typically correct as remote tabs consider themselves to be located at the origin and the main process 'repairs' them when it gets messages. However, when directly talking to page documents and plugins, the content process has to do this itself (pages and plugins should and do assume no knowledge of main-process/content-process differences). So we need to handle that specially. RealWidgetToScreenOffset is created for that purpose. Its use should be contained to these two cases (ie page content and plugins).
Assignee | ||
Comment 6•10 years ago
|
||
(Posted the wrong bug's patch.)
Attachment #8520320 -
Attachment is obsolete: true
Comment 7•9 years ago
|
||
I don't see how the patch would fix the issue (nor it does locally). Better to use the testcase in non-maximized window to see the difference to non-e10s behavior.
Comment 8•9 years ago
|
||
The issue is that if the window is just moved, not resized, TabChild doesn't get any messages about the move.
Comment 9•9 years ago
|
||
(In reply to David Parks [:handyman] from comment #5) > The only interesting issue here is WidgetToScreenOffset, which doesn't > report actual screen coordinates in remote processes. This is typically > correct I don't actually see this ever being correct. > as remote tabs consider themselves to be located at the origin and > the main process 'repairs' them when it gets messages. Sounds like we have some cases to fix. WidgetToScreenOffset should really be about screen offset, not about something else. It is used in many cases and currently e10s gets lots of them wrong.
Comment 10•9 years ago
|
||
I think we need to update all the relevant TabChilds when http://mxr.mozilla.org/mozilla-central/source/widget/nsBaseWidget.cpp?rev=66f63e8af1ca&mark=1450-1451#1438 is called.
Assignee | ||
Comment 11•9 years ago
|
||
Update: smaug's comment 10 has proven a bit tricky. It'll need a bit more work. First, some of the wiring is missing in the current code -- resize and move behavior are handled quite differently. nsWebShellWindow::WindowMoved should notify the docShell, similar to how nsWebShellWindow::WindowResized works. From there, window-resize updates the TabParents indirectly through reflow callbacks. We obviously dont reflow on a window move... the TabParents need to be fished out of the DOM by finding the nsSubdocumentFrames. At least, that's my plan. Additionally, nsCocoaWindow::ReportMoveEvent (and ReportSizeEvent) are operating in the wrong coordinates. With move events, this leads to HiDPI coordinates in Gecko (bad). (mBounds should be scaled by 1/BackingScaleFactor().) This would not be a simple fix -- its a pretty central thread. Just fixing 'move' by itself, however, is probably reasonable (since it isn't quite right now anyway).
Assignee | ||
Comment 12•9 years ago
|
||
Updating patch for bitrot.
Attachment #8520762 -
Attachment is obsolete: true
Assignee | ||
Comment 13•9 years ago
|
||
Created the "MozUpdateWindowPos" chrome event to notify tabs of a window move. Tabs register for this event on the windowroot of their content.
Assignee | ||
Comment 14•9 years ago
|
||
These two patches seem to fix the bug but I'm still looking into test failures: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e714830824e1
Assignee | ||
Comment 15•9 years ago
|
||
Quick update: The Linux e10s mochitest-1 failure appears to be Linux-only (although TBPL doesn't run the test elsewhere, they pass for me on mac and win).
Assignee | ||
Comment 16•9 years ago
|
||
Turns out window.mozInnerScreenX/Y had the same math issue. When they were both out of sync, the tests (incorrectly) pass. With both fixed, the tests are green again. TBPL: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c73ab5655a28
Attachment #8544708 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8544713 -
Flags: review?(bugs)
Assignee | ||
Updated•9 years ago
|
Attachment #8545345 -
Flags: review?(bugs)
Comment 17•9 years ago
|
||
Comment on attachment 8545345 [details] [diff] [review] 1. Properly report screen coords to Javascript, even with a PuppetWidget We should fix puppet widget to not have fake screen coordinates. Otherwise this will fix just a small subset of the issues and we'll need to go through all the other cases. I have no idea why PuppetWidget has fake screen coordinates.
Attachment #8545345 -
Flags: review?(bugs) → review-
Comment 18•9 years ago
|
||
Comment on attachment 8544713 [details] [diff] [review] 2. Update window position in TabParent when window widget is moved > TabParent::SetOwnerElement(Element* aElement) > { >+ // If we held previous content then unregister for its events. >+ nsCOMPtr<nsIContent> frame = do_QueryInterface(mFrameElement); This is useless. Element inherits nsIContent. So just use mFrameElement everywhere. >+ if (frame && frame->OwnerDoc() && frame->OwnerDoc()->GetWindow()) { OwnerDoc() never ever returns null. In general in DOM code only Get* methods may return null. >+ nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(frame->OwnerDoc()->GetWindow()); No need for the do_QueryInterface. >+ if (frame && frame->OwnerDoc() && frame->OwnerDoc()->GetWindow()) { ditto about OwnerDoc() >+ nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(frame->OwnerDoc()->GetWindow()); useless QI
Attachment #8544713 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 19•9 years ago
|
||
Fixes the calculations by changing the PuppetWidget behavior, as suggested. Also repairs mac plugin calculations. To see the effect of the plugin coordinate change, use the test plugin in bug 1044182 (save the plugin to a file and then open it locally... I dealt with this by comparing values in an e10s window vs. a non-e10s window). Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf83020a099d
Attachment #8545345 -
Attachment is obsolete: true
Attachment #8554684 -
Flags: review?(bugs)
Assignee | ||
Comment 20•9 years ago
|
||
with smaug's changes in comment 18.
Attachment #8544713 -
Attachment is obsolete: true
Comment 21•9 years ago
|
||
You don't need to change TabParent::MapEventCoordinatesForChildProcess?
Assignee | ||
Comment 22•9 years ago
|
||
You mean because it does the adaptation that shifts event coordinates to be widget-relative? I initially thought I would... but no. The deal is that the content process event coordinate stuff circumvents the usual coordinate adaptation, using refPoints to establish a screen-relative position. The refPoints are based on 'real' screen coordinates (not content-process screen coordinates) so they produce the correct results -- they are the position of the tab's origin, in actual screen coordinates. You can see what it does with, for example, mouse position, by running the testcase in this bug.
Comment 23•9 years ago
|
||
Ah. This code has been so nicely inconsistent :/
Comment 24•9 years ago
|
||
Waiting for tryserver results before reviewing. Especially b2g results will be interesting.
Assignee | ||
Comment 25•9 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #23) > Ah. This code has been so nicely inconsistent :/ Indeed. I imagine the event-refPoint-for-content-proc stuff could be ripped out with this change (the normal math should work now... I'm pretty sure) but that can be a job for another day.
Comment 26•9 years ago
|
||
Comment on attachment 8554684 [details] [diff] [review] 1. Make PuppetWidget::WidgetToScreenOffset report proper screen coords "// This is actually relative to window-chrome." Could you fix that comment in plugins code > switch (sourceSpace) { > case NPCoordinateSpacePlugin: >- screenPoint = sourcePoint + pluginFrame->GetContentRectRelativeToSelf().TopLeft() + >- chromeSize + pluginPosition + windowPosition; >+ screenPoint = sourcePoint + pluginPosition; I can understand removal of chromeSize and windowPosition, but why also pluginFrame->GetContentRectRelativeToSelf().TopLeft() > switch (destSpace) { > case NPCoordinateSpacePlugin: >- destPoint = screenPoint - pluginFrame->GetContentRectRelativeToSelf().TopLeft() - >- chromeSize - pluginPosition - windowPosition; >+ destPoint = screenPoint - pluginPosition; ditto Note, I'm not familiar with that code. >diff --git a/widget/PuppetWidget.h b/widget/PuppetWidget.h This seems to be ok.
Assignee | ||
Comment 27•9 years ago
|
||
smaug had asked about the contentRect calculation in plugin coord code. This test case shows the failure if it is missing -- the plugin's 100px border isn't counted properly without it. This is just the testcase from bug 1044182 with a 100px border added.
Assignee | ||
Comment 28•9 years ago
|
||
NIing smaug with no question -- since he was reviewing this but requested a plugin reviewer for the nsPluginInstanceOwner code. The plugin screen values can be tested with the test case in the previous comment. The big picture is that the change to PuppetWidget::WidgetToScreenOffset in this patch changes the calculations in ConvertPointPuppet since the pluginFrame coordinate system is different (i.e. it's 'top-level' coordinate system is no longer tab-relative but is genuinely screen relative).
Attachment #8554684 -
Attachment is obsolete: true
Attachment #8554684 -
Flags: review?(bugs)
Flags: needinfo?(bugs)
Attachment #8554861 -
Flags: review?(joshmoz)
Attachment #8554861 -
Flags: review?(joshmoz) → review+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 29•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/94a2a4a7c84c https://hg.mozilla.org/integration/mozilla-inbound/rev/47de36ef3ab4 Should this have a test?
Flags: in-testsuite?
Keywords: checkin-needed
Comment 30•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/94a2a4a7c84c https://hg.mozilla.org/mozilla-central/rev/47de36ef3ab4
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Comment 31•9 years ago
|
||
Looks like this is still a bit wrong. We're getting too small X coordinates.
Flags: needinfo?(bugs)
This is breaking the context menu and tooltips in nightly. We should back it out if it won't be fixed by tonight. Do you think you can fix it by then?
Flags: needinfo?(davidp99)
Comment 33•9 years ago
|
||
I think context menu is broken (also) because its implementation relies on wrong coordinates.
Backed out. remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/1b4d06590778 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/a0e5f5eee758
Status: RESOLVED → REOPENED
Flags: needinfo?(davidp99)
Resolution: FIXED → ---
Assignee | ||
Comment 35•9 years ago
|
||
Tooltips and context menus are indeed broken. Tooltips were using non-HiDPI-capable math... but the WidgetToContent displacement was always (0,0) before, so it didn't show up.
Assignee | ||
Comment 36•9 years ago
|
||
Removed mapScreenCoordinatesFromContent, since content and chrome now use same screen coords.
Comment on attachment 8556205 [details] [diff] [review] 4. Remove mapScreenCoordinatesFromContent as it is no longer needed Review of attachment 8556205 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, this looks great. ::: toolkit/content/widgets/browser.xml @@ +872,1 @@ > this.startScroll(data.scrolldir, pos.x, pos.y); Please just pass data.screenX/Y directly into startScroll.
Attachment #8556205 -
Flags: review+
These patches basically fix the problems I was seeing. However, the Y coordinates are still off by about 20 or 30 pixels. This might be a Linux-specific issue. It sounds like this is bug 1126902. I'd recommend asking smaug for review on the tooltip patch. Maybe he can also help with bug 1126902. Then I think we can re-land. I'm not sure how to test this stuff. Somehow we'd need to be able to find the coordinates that the context menu or the autocomplete or tooltip popups are shown.
https://hg.mozilla.org/mozilla-central/rev/1b4d06590778 https://hg.mozilla.org/mozilla-central/rev/a0e5f5eee758
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 41•9 years ago
|
||
This doesn't actually change any behavior -- its just removal of code that is made even more redundant by the patches in this bug.
Assignee | ||
Comment 42•9 years ago
|
||
The previous patches exposed some holes in the math that affects Linux behavior. This patch does two things: 1) fixes popup calcs to be client-relative (this only really matters on Linux but could have an effect on Windows -- its been tested on Windows as well -- other platforms do not need to distinguish between widget and widget-client relative). 2) fixes GTKs nsWindow::WidgetToScreenOffset to use the correct (ie screen-relative, not parent-widget relative) GTK function. This makes a difference to tooltips.
Assignee | ||
Comment 43•9 years ago
|
||
de-bitrot r+ed in comment 28
Attachment #8554861 -
Attachment is obsolete: true
Assignee | ||
Comment 44•9 years ago
|
||
de-bitrot r+ed in comment 18
Attachment #8554690 -
Attachment is obsolete: true
Assignee | ||
Comment 45•9 years ago
|
||
de-bitrot
Attachment #8556187 -
Attachment is obsolete: true
Attachment #8559453 -
Flags: review?(bugs)
Assignee | ||
Comment 46•9 years ago
|
||
de-bitrot r+ed in comment 37
Attachment #8556205 -
Attachment is obsolete: true
Assignee | ||
Comment 47•9 years ago
|
||
de-bitrot
Attachment #8557387 -
Attachment is obsolete: true
Attachment #8559455 -
Flags: review?(bugs)
Assignee | ||
Comment 48•9 years ago
|
||
PuppetWidget didn't compensate for the case where the OS had a titlebar or other chrome that made nsWindow::GetClientOffset something other than the origin. This didn't matter when the PuppetWidget content was defined to be at the screen origin (since content displacement was always 0,0). The PuppetWidget position now "translates past" the client offset. The PuppetWidget itself therefore has no client offset. Tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=332655649907
Attachment #8557393 -
Attachment is obsolete: true
Attachment #8559497 -
Flags: review?(bugs)
Comment 49•9 years ago
|
||
Comment on attachment 8559453 [details] [diff] [review] 3. Repair tooltip math for HiDPI displays Oh, somewhat old code from bug 171229
Attachment #8559453 -
Flags: review?(bugs) → review+
Comment 50•9 years ago
|
||
(In reply to Olli Pettay [:smaug] (review overload. No more review requests before Feb 8, please) from comment #49) > Comment on attachment 8559453 [details] [diff] [review] Please test cases when content page is zoomed-in/out
Comment 51•9 years ago
|
||
Comment on attachment 8559455 [details] [diff] [review] 5. Clean up redundant code in chrome proc related to content offset Ah, yes, I was wondering why GetChromeDisplacement was added in bug 1092630. Looked like a hack.
Attachment #8559455 -
Flags: review?(bugs) → review+
Comment 52•9 years ago
|
||
Comment on attachment 8559497 [details] [diff] [review] 6. Make PuppetWidget respect ClientOffset (I wouldn't be surprised if some coordinate handling tweaks will be still need in some cases when dealing with some ancient code like embedding/* but such cases should be handled in followup bugs.)
Attachment #8559497 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 53•9 years ago
|
||
Looks good. Tests are in comment 38. All 6 patches are there.
Keywords: checkin-needed
Comment 54•9 years ago
|
||
Part 1 failed to apply cleanly: renamed 1075670 -> final.1.patch applying final.1.patch patching file widget/PuppetWidget.h Hunk #2 FAILED at 119 1 out of 2 hunks FAILED -- saving rejects to file widget/PuppetWidget.h.rej patch failed, unable to continue (try -v) patch failed, rejects left in working dir errors during apply, please fix and refresh final.1.patch David, could you take a look, thanks!
Flags: needinfo?(davidp99)
Keywords: checkin-needed
Assignee | ||
Comment 55•9 years ago
|
||
Fixing bitrot
Attachment #8559450 -
Attachment is obsolete: true
Flags: needinfo?(davidp99)
Comment 56•9 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/bf710daef92b remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/cd6cf15b27cc remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/8b72b1201661 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/d3ac4fd82311 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/4d34eb58c46d remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/4d68750b4e28
Comment 57•9 years ago
|
||
(In reply to Jim Mathies [:jimm] from comment #56) > remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/bf710daef92b > remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/cd6cf15b27cc > remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/8b72b1201661 > remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/d3ac4fd82311 > remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/4d34eb58c46d > remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/4d68750b4e28 sorry had to back this out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=6519415&repo=mozilla-inbound
Flags: needinfo?(davidp99)
Comment 58•9 years ago
|
||
I'd suggest a new set of patches merged to mc tip and pushed to try. :) Also, the commit info in the current set was missing reviewer info.
Assignee | ||
Comment 59•9 years ago
|
||
de-bitrot
Attachment #8562367 -
Attachment is obsolete: true
Flags: needinfo?(davidp99)
Comment 66•9 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/88cd9db3cb3f remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/e5811f696398 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/7f226041aed8 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/dcd9f101e330 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/f5944c276519 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/17a3125a2f05
Comment 67•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/88cd9db3cb3f https://hg.mozilla.org/mozilla-central/rev/e5811f696398 https://hg.mozilla.org/mozilla-central/rev/7f226041aed8 https://hg.mozilla.org/mozilla-central/rev/dcd9f101e330 https://hg.mozilla.org/mozilla-central/rev/f5944c276519 https://hg.mozilla.org/mozilla-central/rev/17a3125a2f05
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
status-firefox38:
--- → fixed
Resolution: --- → FIXED
Comment 68•9 years ago
|
||
Hi David, With these patches, we can't see the bluetooth setting page. Please check https://bugzilla.mozilla.org/show_bug.cgi?id=1133518
Flags: needinfo?(davidp99)
Comment 69•9 years ago
|
||
If it helps, I have these patches reverted on my github branch (and verified it fixes the issues this bug caused on b2g): https://github.com/Cwiiis/gecko-dev/tree/patches-i-want
Comment 70•9 years ago
|
||
Backed out directly on m-c using the backout patch gsvelto created. https://hg.mozilla.org/mozilla-central/rev/4bb425001d8a
Assignee | ||
Comment 72•9 years ago
|
||
On Flame, the chrome process offset is (0,45). It was being calculated as (-480, 45). This undoes the change that switched to the bad math.
Attachment #8567320 -
Flags: review?(bugs)
Assignee | ||
Comment 73•9 years ago
|
||
Tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=54dea11d2378
Assignee | ||
Comment 76•9 years ago
|
||
de-bitrot. r+ed in comment 51.
Attachment #8563528 -
Attachment is obsolete: true
Assignee | ||
Comment 77•9 years ago
|
||
de-bitrot. r+ed in comment 52.
Attachment #8563530 -
Attachment is obsolete: true
Comment 78•9 years ago
|
||
(In reply to David Parks [:handyman] from comment #72) > Created attachment 8567320 [details] [diff] [review] > 7. Change function used to calculate chrome process offset > > On Flame, the chrome process offset is (0,45). It was being calculated as > (-480, 45). This undoes the change that switched to the bad math. Why do we get 480? GetChildProcessOffset() shouldn't return that (well, 480). Or is the iframe actually placed at (480, 45)?
Comment 79•9 years ago
|
||
Comment on attachment 8567320 [details] [diff] [review] 7. Change function used to calculate chrome process offset >+static nsIntPoint >+GetChromeDisplacement(nsFrameLoader *aFrameLoader) >+{ >+ if (!aFrameLoader) >+ return nsIntPoint(); always {} with 'if' But I guess we can take this for now, and figure out the issue in a followup...however, I'm worried that we have then wrong screenX/Y on b2g.
Attachment #8567320 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 80•9 years ago
|
||
I don't know much about how B2G defines 'browser' (in the PBrowser sense) but the situation with the Bluetooth display was that the home screen and the bluetooth app are the same PBrowser instance but with different primary frames. Somehow, the primary frame in the Bluetooth case is translated... but the resulting touch positions (which use this math) are properly handled. I took that to mean that GetChildProcessOffset defines something slightly different in B2G. But maybe its just a number of bugs. This would be good to know.
Attachment #8567320 -
Attachment is obsolete: true
Comment 82•9 years ago
|
||
(In reply to David Parks [:handyman] from comment #80) > but the situation with the Bluetooth display was that the home screen and > the bluetooth app are the same PBrowser instance but with different primary > frames. That makes no sense. You have PBrowser (well, TabChild) for top level child process Window objects.
Comment 84•9 years ago
|
||
The homescreen and the bluetooth app are not in the same process, so I'm not sure how comment 80 is relevant. Some of the bluetooth functionality is actually handled by the system app, so not even OOP.
Flags: needinfo?(fabrice)
Comment 85•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b823c6c95030 https://hg.mozilla.org/integration/mozilla-inbound/rev/e6d6f0c11133 https://hg.mozilla.org/integration/mozilla-inbound/rev/ced9055e0bcc https://hg.mozilla.org/integration/mozilla-inbound/rev/a3e8329610d9 https://hg.mozilla.org/integration/mozilla-inbound/rev/065b859e6525 https://hg.mozilla.org/integration/mozilla-inbound/rev/83199cfc333f https://hg.mozilla.org/integration/mozilla-inbound/rev/4ca74b217fe8
Keywords: checkin-needed
Comment 86•9 years ago
|
||
Backed out for mochitest-e10s crashes. https://hg.mozilla.org/integration/mozilla-inbound/rev/ae8722530027 https://treeherder.mozilla.org/logviewer.html#?job_id=6922318&repo=mozilla-inbound
status-firefox38:
affected → ---
Target Milestone: mozilla38 → ---
Assignee | ||
Comment 88•9 years ago
|
||
Added NULL check for PrimaryFrameOfOwningContent.
Attachment #8568167 -
Attachment is obsolete: true
Assignee | ||
Comment 89•9 years ago
|
||
Here we go again. Tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b2fff3771bc2
Assignee | ||
Comment 90•9 years ago
|
||
Comment on attachment 8568681 [details] [diff] [review] 7. Change function used to calculate chrome process offset Tests are looking good. smaug, the only change to the patch is the null check of contentFrame, which caused the earlier Linux bc1 failures.
Attachment #8568681 -
Flags: review?(bugs)
Updated•9 years ago
|
Attachment #8568681 -
Flags: review?(bugs) → review+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 91•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/13619f8fa672 https://hg.mozilla.org/integration/mozilla-inbound/rev/f705f5063169 https://hg.mozilla.org/integration/mozilla-inbound/rev/bd7a5d213692 https://hg.mozilla.org/integration/mozilla-inbound/rev/a68d5051107f https://hg.mozilla.org/integration/mozilla-inbound/rev/23bb7b239c78 https://hg.mozilla.org/integration/mozilla-inbound/rev/45b61c78ee2d https://hg.mozilla.org/integration/mozilla-inbound/rev/659c40243282
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/13619f8fa672 https://hg.mozilla.org/mozilla-central/rev/f705f5063169 https://hg.mozilla.org/mozilla-central/rev/bd7a5d213692 https://hg.mozilla.org/mozilla-central/rev/a68d5051107f https://hg.mozilla.org/mozilla-central/rev/23bb7b239c78 https://hg.mozilla.org/mozilla-central/rev/45b61c78ee2d https://hg.mozilla.org/mozilla-central/rev/659c40243282
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Comment 93•9 years ago
|
||
Backed out for causing bug 1139010. That's twice this has been backed out for on-device regressions now. Please don't request checkin again until this has been verified to work correctly on a real device. Regression tests would sweeten the deal. https://hg.mozilla.org/mozilla-central/rev/44bcd21e59fe
Status: RESOLVED → REOPENED
status-firefox39:
fixed → ---
Depends on: 1139010
Resolution: FIXED → ---
Target Milestone: mozilla39 → ---
I would like to request that we have a QA run of the patches before landing next time around please. I'm willing to help facilitate that to occur. Please needinfo me before landing.
Assignee | ||
Comment 95•9 years ago
|
||
This resolves the B2G issues from before. It does not fix screenX/screenY on B2G -- there is a serious problem with -moz-transform-origin when used with -moz-transform: scale(2) which makes this hard and a bigger problem with gonk touch coordinate calculations that I'm still trying to track down. I've dropped the old patch #7 (Change function used to calculate chrome process offset) since this now works with the bad values that come from that function. But the bugs in that function are the direct result of the -moz-transform-origin issue and are half of the reason the B2G screen coords are still screwed up.
Attachment #8568681 -
Attachment is obsolete: true
Updated•9 years ago
|
Assignee | ||
Comment 96•9 years ago
|
||
Comment on attachment 8576356 [details] [diff] [review] 7. Make composition bounds calculation widget-relative We are going to try to go with this temporarily, despite event.screenX/Y still being incorrect on Flame. This patch should fix any B2G regressions from the previous patches. kats, you worked on related code in bug 1043644 so you might have some insight here. The gist of the patches in this bug is that PuppetWidget is no longer defined to be positioned at the origin and some math that assumed it was needed to be fixed. You might also have an idea whether or not this change should also be applied to nsLayoutUtils::CalculateCompositionSizeForFrame and nsLayoutUtils::CalculateRootCompositionSize (which I speculate based solely on your patches in bug 1043644).
Attachment #8576356 -
Flags: review?(bugmail.mozilla)
Comment 97•9 years ago
|
||
Comment on attachment 8576356 [details] [diff] [review] 7. Make composition bounds calculation widget-relative Review of attachment 8576356 [details] [diff] [review]: ----------------------------------------------------------------- tn and/or botond would probably be better reviewers for this, they should be able to answer your questions.
Attachment #8576356 -
Flags: review?(tnikkel)
Attachment #8576356 -
Flags: review?(bugmail.mozilla)
Attachment #8576356 -
Flags: review?(botond)
Comment 98•9 years ago
|
||
Comment on attachment 8576356 [details] [diff] [review] 7. Make composition bounds calculation widget-relative Review of attachment 8576356 [details] [diff] [review]: ----------------------------------------------------------------- I'll defer to Timothy for the review, but to answer your question about whether or not this change should be applied to nsLayoutUtils::CalculateCompositionSizeForFrame and nsLayoutUtils::CalculateRootCompositionSize: I don't believe so, because those functions return a size rather than a rect, while this change only affects the origin of the rect.
Attachment #8576356 -
Flags: review?(botond)
Updated•9 years ago
|
Attachment #8576356 -
Flags: review?(tnikkel) → review+
Assignee | ||
Comment 99•9 years ago
|
||
Great, thanks kats, botond and tn. nhirata, you wanted to play with this before it was checked in. Note that the STR in comment 3 (no actual test for this) still will not give the right screen values for events on B2G but there should not be regressions.
Flags: needinfo?(nhirata.bugzilla)
Hi David, do I just need to apply the one patch? Or do I need to apply all the attached patches?
Flags: needinfo?(davidp99)
Assignee | ||
Comment 101•9 years ago
|
||
You'll need all seven -- they build on one another so they need to be applied in numerical order.
Flags: needinfo?(davidp99)
Assignee | ||
Comment 108•9 years ago
|
||
de-bitrot all. r+ed at comment 98
Attachment #8576356 -
Attachment is obsolete: true
Comment 109•9 years ago
|
||
Kats, can you help david figure this out? There seems to be something wrong with the b2g coordinate system and APZC
Flags: needinfo?(bugmail.mozilla)
Assignee | ||
Comment 110•9 years ago
|
||
Naoki's crash stack
Where are we even crashing?
Needinfo Blake to ask Naoki what's going on here. There doesn't seem to be any evidence that this crash (which appears to be related to a bad event somehow) is related to David's patch. We also don't know how common it is. David did a lot of manual testing and didn't reproduce anything. Without further information, I'd like to land this patch tomorrow.
Flags: needinfo?(mrbkap)
Clearing ni as it seems things are under control here and the patches will be landing. If there are still issues that you are having trouble with I'd be happy to help but please provide specific scenarios of bad behavior that i can reproduce with these patches applied.
Flags: needinfo?(bugmail.mozilla)
Assignee | ||
Comment 114•9 years ago
|
||
de-bitrot, of course!
Attachment #8578898 -
Attachment is obsolete: true
Assignee | ||
Comment 120•9 years ago
|
||
de-bitrot. Tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c6f316bcd64c NI to billm for potential submission.
Attachment #8578907 -
Attachment is obsolete: true
Flags: needinfo?(wmccloskey)
The try push was not looking good and one of the failures was in a test I recently added so I took a look. Some of this stuff probably got broken by bug 1126089. 1. Because of the changes from bug 1126089, we have to be careful not to null out mFrameElement too soon because then the message manager won't work. That's why the unload.js test was failing. 2. We were also crashing b2g reftests in some cases because ActorDestroy assumes mFrameElement is non-null. 3. I think we also were not always unregistering the event listener in SetOwnerElement correctly. We have to make sure to do it while mFrameElement is still part of the document. Otherwise we don't have a way to get to the window. Olli, this patch applies on top of part 2 (which doesn't seem to be titled correctly). Here's a new try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f0026c74d05 Please ignore if the crashes aren't fixed.
Flags: needinfo?(wmccloskey)
Attachment #8583595 -
Flags: review?(bugs)
Updated•9 years ago
|
Attachment #8583595 -
Flags: review?(bugs) → review+
The try push looks good. There are some consistent b2g failures, but they're failing consistently in the m-c push this was based on. I'll land when the tree re-opens (so, like, next year). I suspect the crash here may have been fixed by the patch I posted, so I don't think we need to look into that any more.
Flags: needinfo?(nhirata.bugzilla)
Flags: needinfo?(mrbkap)
I haven't crashed on the patch that Bill landed ( tested on the mozilla-inbound build 20150326204640 for flame-kk-eng Now that I think about it, I should probably test Nexus 5 L as well.
Comment 125•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/02fabf032cbe
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Comment 126•9 years ago
|
||
I think we need to back this patch out, since it seems to break the FirefoxOS Gallery app. (See bug 1149183) Comment #109 says there seems to be something wrong with the B2G coordinate system. And that's what I'm seeing. With this patch landed, I'm getting touch events that contain Touch objects that have negative values for screenY or sometimes for screenX on my Flame device running nightly. The Gallery app is the only one in Gaia that uses screenX and screenY, so it is broken by this patch. I should probably modify Gallery to use clientX and clientY instead, but, negative screen coordinates for touch events seem pretty clearly wrong, so let's revert this for now. Here are some things I notice that might help to debug this: When I get a touch event with a negative screenY, I find that clientY - screenY is always equal to 569 or 570. This is on a Flame, where the screen height in CSS pixels is 569.5. Once, when I tested, I was seeing positive screenY values that were the same as clientY, but in that case, the screenX values were negative and clientX - screenX was always equal to 320, which is the width of the screen in CSS pixels. During this test, when I was getting negative screenX values, I tried rotating the device into landscape orientation. When I did that clientX - screenX became 569 or 570, matching the new screen width.
Comment 127•9 years ago
|
||
I'm attaching the small gaia patch I used to diagnose the negative screenX/screenY coordinate issue. This adds some console.log() statements to shared/js/gesture_detector.js and to the Gallery app. The logging output for the gallery output is specific to bug 1149183. It prints touch event coordinates (via the gesture detector) when the user tries to drag the crop handles in the gallery app editing mode. The logging output in the gesture detector prints stuff out any time it sees negative screen coordinates, especially when panning. This output demonstrates that the bug is not specific to the gallery app's cropping code. You can see the same thing when you drag the exposure slider in the image editor. And you can see the same thing if you take a photo with the camera, preview it in the camera, and then zoom in (with a pinch) and pan around with a single finger drag. All of those other things still work because they don't use the screenX and screenY coordinates, but this logging patch demonstrates that the negative coordinates are occurring in those cases too.
Comment 128•9 years ago
|
||
David, can the b2g team give the e10s team a hand in diagnosing and fixing this? This bug blocks a series of patches for e10s that have been waiting to land. The e10s team has little experience with debugging b2g, so it would be great if the b2g team could lend us a hand in figuring out where we're going wrong in this patch.
Flags: needinfo?(dflanagan)
Maybe kats can help now that we have something more concrete that's broken.
Flags: needinfo?(bugmail.mozilla)
Assignee | ||
Comment 130•9 years ago
|
||
The test that is attached to this bug also shows this fairly well. By clicking around the screen and looking at the coordinates, you can see that it establishes a pretty non-linear coordinate system. I believe I'd traced this to geometry that establishes coordinate system axes for widget touch input but I'm not 100% on that.
Comment 131•9 years ago
|
||
(In reply to Jim Mathies [:jimm] from comment #128) > David, can the b2g team give the e10s team a hand in diagnosing and fixing > this? This bug blocks a series of patches for e10s that have been waiting to > land. The e10s team has little experience with debugging b2g, so it would be > great if the b2g team could lend us a hand in figuring out where we're going > wrong in this patch. Jim: that's a totally reasonable request. I work on media apps, however, so noone on my team has any expertise in the APZ or events code. Last time I was involved in bug like this, Kats fixed it in no time, so let's ask him :-) Kats: can you work on this or can you suggest someone who can?
Flags: needinfo?(dflanagan)
Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/06529960107e
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to Bill McCloskey (:billm) from comment #129) > Maybe kats can help now that we have something more concrete that's broken. Yeah I'll take a look. I can repro bug 1149183 on my local build (latest m-c code which doesn't have the backout) and will debug from there.
Flags: needinfo?(bugmail.mozilla)
My initial investigation for the gallery app case seems to indicate that when the TabParent::SendUpdateDimensions call is issued, the chromeOffset [1] is y=-854 (on Flame). This chromeOffset gets stored in the TabChild and used when computing the WidgetToScreen offset, resulting in the negative screen coordinates. The chromeoffset is y=-854 initially I believe because of the CSS transform at [2]. This transform is removed once the app is open and stable, but the chromeOffset is never updated and retains the stale value from app initialization. Continuing to dig... [1] http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabParent.cpp?rev=8bc7561d7557#933 [2] https://github.com/mozilla-b2g/gaia/blob/ee1fba22db03606b8c01fdf947e3746a25a2da46/apps/system/style/window.css#L552
If I remove the guard at http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabParent.cpp?rev=8bc7561d7557#919 it makes things work correctly. The guard doesn't account for the chromeOffset value changing
I threw up a patch for review on bug 1149183. I suggest waiting until that lands, and then re-landing these patches.
Comment 137•9 years ago
|
||
In the same way Naoki requested in comment 94, I would like to that we have an on-device automation run before the next landing. I can help with that. Please needinfo me before landing.
This is the final patch including the fix from kats in bug 1149183. There's a try run here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=22e742e9fab8 I can understand the concern about this patch, but this is a very high priority for us, so please try to get the automation run done soon.
Flags: needinfo?(jlorenzo)
Attachment #8586346 -
Flags: review+
Comment 139•9 years ago
|
||
Gaia UI smoketests are currently running against 3e6e4cfe95801ef207c5d8287bdf115f65f72949 + attachment 8586346 [details] [diff] [review]. Leaving the NI to announce the results.
Comment 140•9 years ago
|
||
Comment on attachment 8586346 [details] [diff] [review] patch for checkin This patch looks good from the smoketests standpoint even though, I encountered a couple of issues while them. I made sure this wasn't related to the patch. Here are the details below: ===================== Smoketest results passed: 40 failed: 6 FAILED TESTS ------- test_browser_play_video.py => The browser keeps loading. I repro'd the issue on 20150401010204 which passed in the lab[1]. There is likely something wrong in my set up. test_homescreen_delete_app.py => I built a userdebug build due to the issue described in bug 1150009. The test fails to install the app during the setUp() phase. This is not related to this patch, I got the issue on a userdebug build which doesn't contain the patch. test_homescreen_delete_app_packaged.py => Idem test_homescreen_launch_app.py => Idem test_homescreen_launch_app_packaged.py => Idem test_music_artist_mp3.py => Due to bug 1149886 [1] http://jenkins1.qa.scl3.mozilla.com/view/UI/job/flame-kk-319.mozilla-central.nightly.ui.functional.smoke/134/HTML_Report/
Flags: needinfo?(jlorenzo)
Attachment #8586346 -
Flags: qa-approval+
https://hg.mozilla.org/mozilla-central/rev/0160303649ae
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: mozilla39 → mozilla40
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•