Closed
Bug 1439472
Opened 7 years ago
Closed 7 years ago
browser/components/extensions/test/browser/test-oop-extensions/browser_ext_popup_select.js fails after bug 1193394
Categories
(WebExtensions :: General, defect)
WebExtensions
General
Tracking
(firefox60 fixed)
RESOLVED
FIXED
mozilla60
Tracking | Status | |
---|---|---|
firefox60 | --- | fixed |
People
(Reporter: arai, Assigned: arai)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 3 obsolete files)
browser/components/extensions/test/browser/test-oop-extensions/browser_ext_popup_select.js
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c9653afa89f067d4a8146ffab41835e9153808fd&group_state=expanded&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=running&filter-resultStatus=pending&filter-resultStatus=runnable&selectedJob=163038978
> 15:55:04 ERROR - 427 INFO TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/test-oop-extensions/browser_ext_popup_select.js | Select popup has the correct y origin - Got 100, expected 95
> 15:55:04 INFO - Stack trace:
> 15:55:04 INFO - chrome://mochikit/content/browser-test.js:test_is:1271
> 15:55:04 INFO - chrome://mochitests/content/browser/browser/components/extensions/test/browser/test-oop-extensions/browser_ext_popup_select.js:testPanel:66
> 15:55:04 INFO - GECKO(8920) | JavaScript error: resource://gre/modules/SelectParentHelper.jsm, line 226: TypeError: currentBrowser.messageManager is null
Assignee | ||
Comment 1•7 years ago
|
||
reproduced locally.
taking.
Assignee: nobody → arai.unmht
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•7 years ago
|
||
I was wrong. I haven't reproduced the same issue.
this test fails on non-windows, and I've checked on macOS
> [browser_ext_popup_select.js]
> skip-if = os != 'win'
I'll check on windows.
Assignee | ||
Comment 3•7 years ago
|
||
reproduced locally on windows with debug build.
Comment hidden (obsolete) |
Assignee | ||
Comment 5•7 years ago
|
||
waiting 1 second before synthesizing mouse click on menu inside browser action popup solved the issue.
So, I think it's clicking the menu while the animation (moving down) is still ongoing.
https://searchfox.org/mozilla-central/rev/0c0ddaa7e859a2b76a56a0e2e9c0de88af166812/browser/components/extensions/test/browser/browser_ext_popup_select.js#49
Assignee | ||
Comment 6•7 years ago
|
||
by increasing transition-duration for the arrow panel, the issue reproduced on macOS.
https://searchfox.org/mozilla-central/rev/0c0ddaa7e859a2b76a56a0e2e9c0de88af166812/toolkit/content/xul.css#423-489
so, I think it has to wait for the transition end.
Assignee | ||
Comment 7•7 years ago
|
||
Here's the patch that waits for all animations.
I'll ask review once I've confirmed the fix on try (currently closed)
Assignee | ||
Comment 8•7 years ago
|
||
Comment on attachment 8952305 [details] [diff] [review]
Wait for all transitions for arrow panel before operating on the content of the panel.
this fails.
looks like we need to wait a bit more.
(waiting +1 sec solves the issue)
I'll check what's going on there.
Attachment #8952305 -
Attachment is obsolete: true
Assignee | ||
Comment 9•7 years ago
|
||
waiting for the next event tick after animations also solves
https://treeherder.mozilla.org/#/jobs?repo=try&revision=38a22adec8a8ba9973b26bd7f00b000915f1b132&selectedJob=163623132
looks like something (layout?) is not yet updated at the moment
Assignee | ||
Comment 10•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=216031247d7c6d9885e34766da153ca2dd448821
On successful case:
boxObject.screenY = 71, elemRect.bottom = 29, popupRect.top = 100
and on failure case:
boxObject.screenY = 71, elemRect.bottom = 29, popupRect.top = 95
So, looks like the return value of getOuterScreenRect might not be updated
when the animation finished promise is resolved.
Assignee | ||
Updated•7 years ago
|
Version: 53 Branch → unspecified
Assignee | ||
Comment 11•7 years ago
|
||
So, the original issue here is that, the promise returned by awaitExtensionPanel didn't wait for the sliding down animation of the popup panel,
so the "select" element's position depends on the animation.
Then, even if we wait for the animations' `finished` promises, the rect returned by popup.getOuterScreenRect() might not be updated at the point of the promises resolution.
Just waiting for the next event tick after the animation solves it.
Attachment #8953027 -
Flags: review?(gijskruitbosch+bugs)
Comment 12•7 years ago
|
||
Comment on attachment 8953027 [details] [diff] [review]
Wait for all transitions for arrow panel before operating on the content of the panel.
Review of attachment 8953027 [details] [diff] [review]:
-----------------------------------------------------------------
Paolo or :zombie is a better reviewer here because this is webextension / panel code.
Attachment #8953027 -
Flags: review?(tomica)
Attachment #8953027 -
Flags: review?(paolo.mozmail)
Attachment #8953027 -
Flags: review?(gijskruitbosch+bugs)
Comment 13•7 years ago
|
||
Comment on attachment 8953027 [details] [diff] [review]
Wait for all transitions for arrow panel before operating on the content of the panel.
I think that what needs to happen here is that if the panel is located inside a PanelView, we should wait for the ViewShown event. However, we shouldn't wait if the ViewShown event has already been dispatched.
I don't think we have a way to know the latter with certainty. I'll file a bug.
Attachment #8953027 -
Flags: review?(tomica)
Attachment #8953027 -
Flags: review?(paolo.mozmail)
Assignee | ||
Comment 14•7 years ago
|
||
Thank you for reviewing :)
When is the ViewShown event supposed to be dispatched?
Does it reflect the animation state?
(I skimmed the code around the event dispatch, but I couldn't find the reference to the animation)
Flags: needinfo?(paolo.mozmail)
Comment 15•7 years ago
|
||
(In reply to Tooru Fujisawa [:arai] from comment #14)
> When is the ViewShown event supposed to be dispatched?
> Does it reflect the animation state?
Yes, it is always dispatched when the transition has finished and the contents are fully in view, so mouse events on the content can be simulated safely.
Flags: needinfo?(paolo.mozmail)
Comment 16•7 years ago
|
||
Also note that bug 1439358 is making some changes around how the events are handled, but it will not change this invariant for ViewShown.
Comment 17•7 years ago
|
||
(In reply to :Paolo Amadini from comment #15)
> (In reply to Tooru Fujisawa [:arai] from comment #14)
> > When is the ViewShown event supposed to be dispatched?
> > Does it reflect the animation state?
>
> Yes, it is always dispatched when the transition has finished and the
> contents are fully in view, so mouse events on the content can be simulated
> safely.
Is this true for main views and the panel transition? Because I'm not convinced it is...
Assignee | ||
Comment 18•7 years ago
|
||
(In reply to :Paolo Amadini from comment #15)
> (In reply to Tooru Fujisawa [:arai] from comment #14)
> > When is the ViewShown event supposed to be dispatched?
> > Does it reflect the animation state?
>
> Yes, it is always dispatched when the transition has finished and the
> contents are fully in view, so mouse events on the content can be simulated
> safely.
Apparently that's not true.
Rhe event is dispatched before the transition finishes,
I've checked it by extending the transition duration to 5s.
The ViewShown event is dispatched almost the same time as WebExtPopupLoaded,
but transition is still ongoing at that point.
do you think we should fix the ViewShown event to be dispatched after transition?
Flags: needinfo?(paolo.mozmail)
Assignee | ||
Comment 19•7 years ago
|
||
Just to confirm, is the `transition` you mentioned the sliding down one? or perhaps it's sliding left/right one?
What I want to wait is the sliding down one for the arrow panel, that changes the y position of the panel.
Comment 20•7 years ago
|
||
Yes, this wasn't true for the main view, but if you apply the patches from bug 1440333 it should now work for the main view too. Since it now relies on "popupshown" for the main view, I believe it should take the sliding down transition into account. For the sliding transition, it was already taken into account for the ViewShown event.
Flags: needinfo?(paolo.mozmail)
Comment 21•7 years ago
|
||
You can also query "PanelView.forNode(node).active" to see if you need to wait for the ViewShown event, or if the view is already displayed. This is necessary in your case because you don't have the reference to the view before you start the process that will show it, as the view is created dynamically. The "node" is the <panelview> element, not the <panel> or <panelmultiview>.
Assignee | ||
Comment 22•7 years ago
|
||
Thanks, waiting for ViewShown works :)
Attachment #8953027 -
Attachment is obsolete: true
Attachment #8954269 -
Flags: review?(paolo.mozmail)
Assignee | ||
Comment 23•7 years ago
|
||
Paolo, can you please take time to review this patch so that we can land bug 1193394 ?
Flags: needinfo?(paolo.mozmail)
Comment 24•7 years ago
|
||
Comment on attachment 8954269 [details] [diff] [review]
Wait for ViewShown event of extension panel before synthesizing mouse event.
Review of attachment 8954269 [details] [diff] [review]:
-----------------------------------------------------------------
Thank you very much for working on this and bug 1193394!
::: browser/components/extensions/test/browser/head.js
@@ +222,5 @@
> win.document, "WebExtPopupLoaded", true,
> event => event.detail.extension.id === extension.id);
>
> + let panelview = getPanelViewForNode(browser);
> + let viewShownPromise;
nit: let viewShownPromise = null, so it's clearer that we're intentionally using a non-Promise value in some cases.
Attachment #8954269 -
Flags: review?(paolo.mozmail) → review+
Updated•7 years ago
|
Flags: needinfo?(paolo.mozmail)
Assignee | ||
Comment 25•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c928c9f81cace6bc4dda2fcd61875106910e93a
Bug 1439472 - Wait for all transitions for arrow panel before operating on the content of the panel. r=Paolo
Assignee | ||
Comment 26•7 years ago
|
||
(In reply to Tooru Fujisawa [:arai] from comment #25)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> 1c928c9f81cace6bc4dda2fcd61875106910e93a
> Bug 1439472 - Wait for all transitions for arrow panel before operating on
> the content of the panel. r=Paolo
sorry, I landed wrong version.
will back it out and land the correct one.
Assignee | ||
Comment 27•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b63bfe468d99d862d1610c936ecc344578abaf3
Backed out changeset 1c928c9f81ca (bug 1439472)
https://hg.mozilla.org/integration/mozilla-inbound/rev/a512c178c19ba8867d636188c5e56c943710a7d5
Bug 1439472 - Wait for ViewShown event of extension panel before synthesizing mouse event. r=Paolo
Comment 28•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Comment 29•7 years ago
|
||
backout bugherder |
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 30•7 years ago
|
||
Backed out changeset a512c178c19b (bug 1439472) for causing bug 1442101. a=backout
push that caused the backout: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=a512c178c19ba8867d636188c5e56c943710a7d5&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=running&filter-resultStatus=pending&filter-resultStatus=runnable
failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=running&filter-resultStatus=pending&filter-resultStatus=runnable&fromchange=d443027a615ce256f2ddd67449ff918d9fad8743&selectedJob=165091340&filter-searchStr=Windows%207%20debug%20Mochitests%20with%20e10s%20test-windows7-32%2Fdebug-mochitest-browser-chrome-e10s-7%20M-e10s(bc7)
https://treeherder.mozilla.org/logviewer.html#?job_id=165091340&repo=mozilla-inbound
backout: https://hg.mozilla.org/mozilla-central/rev/bddc2ca6492179e4b287c3c05f249bba1350e8ef
Updated•7 years ago
|
Flags: needinfo?(arai.unmht)
Comment 31•7 years ago
|
||
If the failure is only happening on Windows debug, and still happens after bug 1442187, then feel free to skip the test on Windows debug and file a bug to re-enable it. I'm not particularly concerned about that test running on debug builds as long as it passes on opt.
Assignee | ||
Comment 32•7 years ago
|
||
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #31)
> If the failure is only happening on Windows debug, and still happens after
> bug 1442187, then feel free to skip the test on Windows debug and file a bug
> to re-enable it. I'm not particularly concerned about that test running on
> debug builds as long as it passes on opt.
thanks, I'll double check if the issue doesn't happen on opt.
Flags: needinfo?(arai.unmht)
Assignee | ||
Comment 33•7 years ago
|
||
(In reply to Tooru Fujisawa [:arai] from comment #32)
> (In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're
> blocked) from comment #31)
> > If the failure is only happening on Windows debug, and still happens after
> > bug 1442187, then feel free to skip the test on Windows debug and file a bug
> > to re-enable it. I'm not particularly concerned about that test running on
> > debug builds as long as it passes on opt.
>
> thanks, I'll double check if the issue doesn't happen on opt.
at least for past tries, the issue happens only on debug
https://docs.google.com/spreadsheets/d/1iH50biQLuUSMSwMGd5LZFchDgmbAFjlEYdM92nDiPAc/edit#gid=1861398285
Assignee | ||
Comment 34•7 years ago
|
||
here's try, based on bug 1442218 patch and others
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9efa87286c5f1799128df4744858a84354525294
Assignee | ||
Comment 35•7 years ago
|
||
Here's the patch that skips the test on debug.
Assignee | ||
Comment 36•7 years ago
|
||
Comment on attachment 8955623 [details] [diff] [review]
Skip browser/components/extensions/test/browser/browser_ext_popup_select.js on debug build.
I'll file a followup bug later and fill the bug number
Attachment #8955623 -
Flags: review?(kmaglione+bmo)
Updated•7 years ago
|
Attachment #8955623 -
Flags: review?(kmaglione+bmo) → review+
Assignee | ||
Comment 37•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b5cffe82ff05e7cfbdfb839b11f017bd5db9a2f
Bug 1439472 - Skip browser/components/extensions/test/browser/browser_ext_popup_select.js on debug build. r=kmag
Assignee | ||
Updated•7 years ago
|
Attachment #8954269 -
Attachment is obsolete: true
Comment 38•7 years ago
|
||
bugherder |
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
Resolution: --- → FIXED
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•