Closed Bug 1378794 Opened 7 years ago Closed 7 years ago

When temporary panels are destroyed, the ViewHide event may be dispatched on the wrong panelview

Categories

(Firefox :: Toolbars and Customization, defect, P1)

Product:
Firefox
Component:
Toolbars and Customization
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 56
Iteration:
56.2 - Jul 10
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: mikedeboer, Assigned: mikedeboer)

References

Details

(Whiteboard: [photon-structure])

Attachments

(1 file)

Bug 1378794 - Dispatch the 'ViewHide' event on the correct, current panelview when a temporary panel is destroyed.
59 bytes, text/x-review-board-request
Gijs
: review+
Details 
I noticed this issue whilst working intensively with temporary panels in bug 1354532: the 'ViewHide' event is always dispatched on the `viewNode` that was set in the very beginning, but meanwhile another subview may have already been shown in the panel before it's destroyed.
This causes the `viewNode` to receive the 'ViewHide' event for the second time and the current viewpanel to never receive it.
Flags: qe-verify-
Comment on attachment 8883957 [details]
Bug 1378794 - Dispatch the 'ViewHide' event on the correct, current panelview when a temporary panel is destroyed.

https://reviewboard.mozilla.org/r/154938/#review159930

::: browser/components/customizableui/content/panelUI.js:538
(Diff revision 1)
> -          viewNode.dispatchEvent(evt);
> +          let evt = new CustomEvent("ViewHiding", {detail: currentView});
> +          currentView.dispatchEvent(evt);

Doesn't the panelmultiview code do this itself? Seems like that would make more sense, but rs=me to make it not do the wrong thing, at least.
Attachment #8883957 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8883957 [details]
Bug 1378794 - Dispatch the 'ViewHide' event on the correct, current panelview when a temporary panel is destroyed.

https://reviewboard.mozilla.org/r/154938/#review159930

> Doesn't the panelmultiview code do this itself? Seems like that would make more sense, but rs=me to make it not do the wrong thing, at least.

Perhaps, but this may have been put here way back when (Australis era) because relying on the XBL destructor was too unreliable?
Pushed by mdeboer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f3fc2ea88f60
Dispatch the 'ViewHide' event on the correct, current panelview when a temporary panel is destroyed. r=Gijs
Backed out for failing browser/components/extensions/test/browser/browser_ext_popup_corners.js:

https://hg.mozilla.org/integration/autoland/rev/8d3213b771bda968d122e0c5544470c1b4f60f02

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=f3fc2ea88f607ce82ed46e329d44350bfb9c8ce2&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=112281980&repo=autoland

[task 2017-07-06T16:04:58.763459Z] 16:04:58     INFO - Test stand-alone browserAction popup
[task 2017-07-06T16:04:58.769672Z] 16:04:58     INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_popup_corners.js | Expect widget not to be overflowed - 
[task 2017-07-06T16:04:58.771861Z] 16:04:58     INFO - Buffered messages logged at 16:04:57
[task 2017-07-06T16:04:58.774915Z] 16:04:58     INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_popup_corners.js | Panel and view borderTopLeftRadius should be the same - 
[task 2017-07-06T16:04:58.778158Z] 16:04:58     INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_popup_corners.js | Panel and body borderTopLeftRadius should be the same - 
[task 2017-07-06T16:04:58.781913Z] 16:04:58     INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_popup_corners.js | Panel and view borderTopRightRadius should be the same - 
[task 2017-07-06T16:04:58.784199Z] 16:04:58     INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_popup_corners.js | Panel and body borderTopRightRadius should be the same - 
[task 2017-07-06T16:04:58.786514Z] 16:04:58     INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_popup_corners.js | Panel and view borderBottomRightRadius should be the same - 
[task 2017-07-06T16:04:58.790143Z] 16:04:58     INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_popup_corners.js | Panel and body borderBottomRightRadius should be the same - 
[task 2017-07-06T16:04:58.793237Z] 16:04:58     INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_popup_corners.js | Panel and view borderBottomLeftRadius should be the same - 
[task 2017-07-06T16:04:58.796786Z] 16:04:58     INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_popup_corners.js | Panel and body borderBottomLeftRadius should be the same - 
[task 2017-07-06T16:04:58.801484Z] 16:04:58     INFO - Buffered messages finished
[task 2017-07-06T16:04:58.806170Z] 16:04:58     INFO - TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/browser_ext_popup_corners.js | uncaught exception - TypeError: currentView is undefined at panelRemover@chrome://browser/content/customizableui/panelUI.js:539:11
[task 2017-07-06T16:04:58.808166Z] 16:04:58     INFO - hidePopup@chrome://global/content/bindings/popup.xml:137:13
[task 2017-07-06T16:04:58.810279Z] 16:04:58     INFO - hidePanelForNode@resource:///modules/CustomizableUI.jsm:1689:7
[task 2017-07-06T16:04:58.813496Z] 16:04:58     INFO - hidePanelForNode@resource:///modules/CustomizableUI.jsm:3693:5
[task 2017-07-06T16:04:58.816649Z] 16:04:58     INFO - closeBrowserAction@chrome://mochitests/content/browser/browser/components/extensions/test/browser/head.js:263:3
[task 2017-07-06T16:04:58.818701Z] 16:04:58     INFO - testPopupBorderRadius@chrome://mochitests/content/browser/browser/components/extensions/test/browser/browser_ext_popup_corners.js:75:11
[task 2017-07-06T16:04:58.820739Z] 16:04:58     INFO - Async*Tester_execTest/<@chrome://mochikit/content/browser-test.js:763:21
[task 2017-07-06T16:04:58.827555Z] 16:04:58     INFO - TaskImpl_run@resource://gre/modules/Task.jsm:331:42
[task 2017-07-06T16:04:58.829731Z] 16:04:58     INFO - TaskImpl@resource://gre/modules/Task.jsm:280:3
[task 2017-07-06T16:04:58.832860Z] 16:04:58     INFO - asyncFunction@resource://gre/modules/Task.jsm:252:14
[task 2017-07-06T16:04:58.834751Z] 16:04:58     INFO - Task_spawn@resource://gre/modules/Task.jsm:166:12
[task 2017-07-06T16:04:58.836942Z] 16:04:58     INFO - Tester_execTest@chrome://mochikit/content/browser-test.js:758:9
[task 2017-07-06T16:04:58.843018Z] 16:04:58     INFO - Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:670:7
[task 2017-07-06T16:04:58.845479Z] 16:04:58     INFO - SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:795:59
[task 2017-07-06T16:04:58.851450Z] 16:04:58     INFO - 
[task 2017-07-06T16:04:58.853398Z] 16:04:58     INFO - Stack trace:
[task 2017-07-06T16:04:58.855216Z] 16:04:58     INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:simpletestOnerror:1652
[task 2017-07-06T16:04:58.858131Z] 16:04:58     INFO - chrome://global/content/bindings/popup.xml:hidePopup:137
[task 2017-07-06T16:04:58.860047Z] 16:04:58     INFO - resource:///modules/CustomizableUI.jsm:hidePanelForNode:1689
[task 2017-07-06T16:04:58.861865Z] 16:04:58     INFO - resource:///modules/CustomizableUI.jsm:hidePanelForNode:3693
[task 2017-07-06T16:04:58.864455Z] 16:04:58     INFO - chrome://mochitests/content/browser/browser/components/extensions/test/browser/head.js:closeBrowserAction:263
[task 2017-07-06T16:04:58.866494Z] 16:04:58     INFO - chrome://mochitests/content/browser/browser/components/extensions/test/browser/browser_ext_popup_corners.js:testPopupBorderRadius:75
[task 2017-07-06T16:04:58.872553Z] 16:04:58     INFO - GECKO(1092) | JavaScript error: chrome://browser/content/customizableui/panelUI.js, line 539: TypeError: currentView is undefined
Flags: needinfo?(mdeboer)
Sorry about that, I clearly hadn't thought about the non-Photon case. Running the fix through Try now.
Flags: needinfo?(mdeboer)
Pushed by mdeboer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a041f3412d61
Dispatch the 'ViewHide' event on the correct, current panelview when a temporary panel is destroyed. r=Gijs
https://hg.mozilla.org/mozilla-central/rev/a041f3412d61
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: