1470880 - Figure out failure in "test_arrowpanel.xul" and remove "resources" from the "arrowpanel" binding

Status: RESOLVED FIXED

(Core :: XBL, task, P3)

Description

[:Paolo Amadini]

The following lines are required in the "arrowpanel" binding to address a failure in "test_arrowpanel.xul" apparently related to animation timing:

<resources>
  <stylesheet src="data:text/css,"/>
</resources>

This may be fixed as part of the conversion of panels to Custom Elements, but should be investigated if we want to remove support for <resources> earlier.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: patch with which I expected to be able to repro the bug.

So I took a look at this. I would've expected to see the tests pass with this change, instead I got:

toolkit/content/tests/chrome/test_arrowpanel.xul
  FAIL animated popup not hidden yet
SimpleTest.ok@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:275:7
nextTest@chrome://mochitests/content/chrome/toolkit/content/tests/chrome/test_arrowpanel.xul:204:5
onpopuphidden@chrome://mochitests/content/chrome/toolkit/content/tests/chrome/test_arrowpanel.xul:1:29

I assume that is the test that fails on mac without the <resources>? Or did I miss something?

Comment 2

[:Paolo Amadini]

On Mac without <resources>, the test hangs at "PASS state is showing", then after clicking anywhere to close the popup I get:

FAIL state is showing during transitionend - got "hiding", expected "showing"
FAIL popupshown not fired yet
FAIL animated popup not hidden yet

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Ah, I should be able to repro this with the patch in bug 1007283 or something like that.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Patch to make the test pass on Linux

And removing the <resources> does make the test fail, at least on an opt build, yay.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1470880 - Stop relying on [side] in arrowpanel selectors in order to remove last usage of xbl resources. r=bgrins

The reason why this fixes it is a bit subtle, let me try to explain.

XBL has this mechanism where all attributes in the binding <content> element
get auto-propagated to the bound element (the <panel> in this case).

This doesn't seem to be a very used feature looking at:

https://searchfox.org/mozilla-central/search?q=%3Ccontent&case=false&regexp=false&path=xml

The panel binding uses it to add the side attribute:

https://searchfox.org/mozilla-central/rev/d80f0a570736dce76a2eb184fb65517462089e8a/toolkit/content/widgets/popup.xml#264

The key here is that this attribute addition is silent (aNotify=false):

https://searchfox.org/mozilla-central/rev/d80f0a570736dce76a2eb184fb65517462089e8a/dom/xbl/nsXBLBinding.cpp#341

This means that the presence of this attribute is not supposed to change the
rendering of the page. It'd also be unsafe to notify at the point at which we
create XBL bindings.

So the way this happens is:

• We compute the initial style of the <panel> element (which doesn't have a
  side attribute, and thus doesn't match the rules, and has a computed
  opacity of 1).
• The XBL service silently sets the side attribute. This should cause a
  style change, but it doesn't since it's silent, so we remain with the
  opacity of 1.
• We open the popup, and the XBL binding listens for the popupshowing event
  and adds the animate attribute. The style system notices, and eventually
  we compute the new style. Issue is, it has again an opacity of 1, so we
  don't fire the transition.

Same with transform and such of course.

So far so good, but then, why does it work if there's a <resources> element
with an empty stylesheet? Fun that you ask!

We explicitly re-resolve the style of the element if there are any stylesheets:

https://searchfox.org/mozilla-central/rev/d80f0a570736dce76a2eb184fb65517462089e8a/dom/xbl/nsXBLService.cpp#551

And thus grab the correct initial opacity of zero, and trigger the transition.

Given arrow panels always have a side attribute (and same for the bookmarks
thing), making their style not depend on the silent attribute additions from
<content> fixes the bug.

We could fix the bug with an alternative patch (re-resolving style if there's a
<content> element with attributes in the binding). This wouldn't be completely
sound anyway in presence of combinators, and given that it'd remain being
unsound anyway, we should probably just remove that feature.

Also, the simplification of the stylesheets seems worth it anyway.

I'll follow-up removing the <resources> implementation, and we should probably
investigate removing the <content> attribute propagation, since it's the
really unsound thing here.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9509ae2baf1c
Simplify arrowpanel selectors in order to remove last usage of xbl resources. r=bgrins

Comment 7

[Bogdan Tara[:bogdan_tara | bogdant]]

Backed out changeset 9509ae2baf1c (Bug 1470880) for components/customizableui/test/browser* failures

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&searchStr=windows%2C10%2Cx64%2Casan%2Cmochitests%2Ctest-windows10-64-asan%2Fopt-mochitest-browser-chrome-e10s-7%2Cm%28bc7%29&tochange=54bfe0a47b678837214f7b46978d98b225fdfd2e&fromchange=abcd7426fb63f078b3d5e2566c28baa1dd923616&selectedJob=241918339

Backout link: https://hg.mozilla.org/integration/autoland/rev/54bfe0a47b678837214f7b46978d98b225fdfd2e

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=241918339&repo=autoland&lineNumber=2598

23:26:31 INFO - TEST-START | browser/components/customizableui/test/browser_customizemode_uidensity.js
23:28:01 INFO - TEST-INFO | started process screenshot
23:28:01 INFO - TEST-INFO | screenshot: exit 0
23:28:01 INFO - Buffered messages logged at 23:26:31
23:28:01 INFO - Entering test bound test_compact_mode_menuitem
23:28:01 INFO - TEST-PASS | browser/components/customizableui/test/browser_customizemode_uidensity.js | Normal mode menuitem should be active by default -
23:28:01 INFO - Buffered messages logged at 23:26:32
23:28:01 INFO - TEST-PASS | browser/components/customizableui/test/browser_customizemode_uidensity.js | UI Density should be set to compact on compact menuitem hover -
23:28:01 INFO - TEST-PASS | browser/components/customizableui/test/browser_customizemode_uidensity.js | UI Density pref should still be set to normal on compact menuitem hover -
23:28:01 INFO - TEST-PASS | browser/components/customizableui/test/browser_customizemode_uidensity.js | UI Density should be reset when no longer hovering the compact menuitem -
23:28:01 INFO - Buffered messages finished
23:28:01 INFO - TEST-UNEXPECTED-FAIL | browser/components/customizableui/test/browser_customizemode_uidensity.js | Test timed out -
23:28:01 INFO - GECKO(524) | MEMORY STAT | vsize 19406223MB | vsizeMaxContiguous 65365277MB | residentFast 1546MB
23:28:01 INFO - TEST-OK | browser/components/customizableui/test/browser_customizemode_uidensity.js | took 90134ms
23:28:01 INFO - Not taking screenshot here: see the one that was previously logged
23:28:01 INFO - TEST-UNEXPECTED-FAIL | browser/components/customizableui/test/browser_customizemode_uidensity.js | Found a tab after previous test timed out: about:blank -
23:28:01 INFO - checking window state
23:28:01 INFO - TEST-START | browser/components/customizableui/test/browser_drag_outside_palette.js
23:28:01 INFO - GECKO(524) | JavaScript error: resource:///modules/CustomizeMode.jsm, line 2108: TypeError: positionManager is undefined
23:28:01 INFO - Not taking screenshot here: see the one that was previously logged
23:28:01 INFO - Buffered messages logged at 23:28:01
23:28:01 INFO - Entering test bound
23:28:01 INFO - Actual placements: back-button, forward-button, stop-reload-button, home-button, customizableui-special-spring1, urlbar-container, customizableui-special-spring2, downloads-button, library-button, sidebar-button, fxa-toolbar-menu-button
23:28:01 INFO - Expected placements: back-button, forward-button, stop-reload-button, customizableui-special-spring1, urlbar-container, customizableui-special-spring2, downloads-button, library-button, sidebar-button, fxa-toolbar-menu-button
23:28:01 INFO - Buffered messages finished
23:28:01 INFO - TEST-UNEXPECTED-FAIL | browser/components/customizableui/test/browser_drag_outside_palette.js | Area nav-bar should have 10 items. - Got 11, expected 10
23:28:01 INFO - Stack trace:
23:28:01 INFO - chrome://mochikit/content/browser-test.js:test_is:1325
23:28:01 INFO - chrome://mochitests/content/browser/browser/components/customizableui/test/head.js:placementArraysEqual:139
23:28:01 INFO - chrome://mochitests/content/browser/browser/components/customizableui/test/head.js:assertAreaPlacements:133
23:28:01 INFO - chrome://mochitests/content/browser/browser/components/customizableui/test/browser_drag_outside_palette.js:null:17
23:28:01 INFO - chrome://mochikit/content/browser-test.js:Tester_execTest/<:1116
23:28:01 INFO - chrome://mochikit/content/browser-test.js:Tester_execTest:1144
23:28:01 INFO - chrome://mochikit/content/browser-test.js:nextTest/<:1005

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Those tests are relying on the transition not firing.

Before: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c2f02e79609107e6b8899a07b22e2315fc19fdb0
After: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0ded40653f5cfa74e42e9fbe64a6d7bf71c7c882

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1470880 - Fix specificity of the selectors for bottom-sided panels. r=bgrins

I could try to dig out what part of the test is racy, but I'd rather preserve
the behavior of not firing the transition in the test for now.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Wow, I got bitten by CSS specificity, that's ironic.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9d97144e67e9
Stop relying on [side] in arrowpanel selectors in order to remove last usage of xbl resources. r=bgrins

Comment 12

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/9d97144e67e9