Closed Bug 1470880 Opened 2 years ago Closed 9 months ago

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

Categories

(Core :: XBL, task, P3)

task

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox68 --- fixed

People

(Reporter: Paolo, Assigned: emilio)

References

Details

Attachments

(2 files, 2 obsolete files)

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.
See Also: → 1485554
Blocks: 1474069
Flags: needinfo?(emilio)
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?
Flags: needinfo?(emilio) → needinfo?(paolo.mozmail)
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
Flags: needinfo?(paolo.mozmail)

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

Flags: needinfo?(emilio)

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

Attachment #9030098 - Attachment is obsolete: true
Flags: needinfo?(emilio)
Flags: needinfo?(emilio)

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.

Flags: needinfo?(emilio)
Blocks: 1546046
Assignee: nobody → emilio
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9509ae2baf1c
Simplify arrowpanel selectors in order to remove last usage of xbl resources. r=bgrins

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

Flags: needinfo?(emilio)

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.

Attachment #9060258 - Attachment description: Bug 1470880 - Don't animate a customize mode panel to preseve behavior and make a test happy. r=bgrins → Bug 1470880 - Fix specificity of the selectors for bottom-sided panels. r=bgrins

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

Attachment #9060258 - Attachment is obsolete: true
Attachment #9059756 - Attachment description: Bug 1470880 - Simplify arrowpanel selectors in order to remove last usage of xbl resources. r=florian → Bug 1470880 - Stop relying on [side] in arrowpanel selectors in order to remove last usage of xbl resources. r=bgrins
Pushed by ealvarez@mozilla.com:
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
Type: enhancement → task
Status: NEW → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
You need to log in before you can comment on or make changes to this bug.