Closed Bug 1439472 Opened 6 years ago Closed 6 years ago

browser/components/extensions/test/browser/test-oop-extensions/browser_ext_popup_select.js fails after bug 1193394

Categories

(WebExtensions :: General, defect)

Product:
WebExtensions
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox60 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: arai, Assigned: arai)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

Skip browser/components/extensions/test/browser/browser_ext_popup_select.js on debug build.
1.11 KB, patch
kmag
: review+
Details | Diff | Splinter Review 
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
reproduced locally.
taking.
Assignee: nobody → arai.unmht
Status: NEW → ASSIGNED
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.
reproduced locally on windows with debug build.
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
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.
Here's the patch that waits for all animations.
I'll ask review once I've confirmed the fix on try (currently closed)
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
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
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.
Version: 53 Branch → unspecified
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 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 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)
Depends on: 1440333
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)
(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)
Also note that bug 1439358 is making some changes around how the events are handled, but it will not change this invariant for ViewShown.
(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...
(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)
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.
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)
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>.
Thanks, waiting for ViewShown works :)
Attachment #8953027 - Attachment is obsolete: true
Attachment #8954269 - Flags: review?(paolo.mozmail)
Paolo, can you please take time to review this patch so that we can land bug 1193394 ?
Flags: needinfo?(paolo.mozmail)
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+
Flags: needinfo?(paolo.mozmail)
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
(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.
https://hg.mozilla.org/mozilla-central/rev/a512c178c19b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
for causing bug 1442101
https://hg.mozilla.org/mozilla-central/rev/bddc2ca64921
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Flags: needinfo?(arai.unmht)
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.
(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)
(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
Here's the patch that skips the test on debug.
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)
Attachment #8955623 - Flags: review?(kmaglione+bmo) → review+
Blocks: 1442822
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
Attachment #8954269 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/1b5cffe82ff0
Status: REOPENED → RESOLVED
Closed: 6 years ago → 6 years ago
Resolution: --- → FIXED
Flags: qe-verify-
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: