Unassigning. This is not actionable for now.

Note that the patch may work without hitting assertion as soon as we have removed all <arrowscrollbox> usage inside XBL <content>

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

At the time of writing these bindings are

• tabbrowser-tabs
• places-popup-base
• places-popup-arrow
• popup

Dependencies should be set when bugs for these bindings are filed.

here's updated to trunk patch. It uses shadow DOM which requires to move styling into the widget or wait for bug 1505489. Also, it appears both approaches (shadow DOM and the light DOM original approach) will require adjustments in keyboard handling in popup code (see bug 1555497 for details), so bug 1555497 might be a stopper for this one.

Latest try push https://treeherder.mozilla.org/#/jobs?repo=try&revision=1dcbdf037d5bec3be4c9a5fa0915128a7ddae705&selectedJob=265046095

Talos looks good: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=43fc59024f8d6dbcf6fa9112521f42922417ec70&newProject=try&newRevision=1c2c41150d15468e5dd6277adf7f65749c45b04e&framework=1&showOnlyImportant=1

Emilio, I've got a weird one for you here. The toolkit/content/tests/chrome/test_mousescroll.xul test is failing with this change, and it seems at least somewhat related to shadow dom and/or the prototype cache.

In this test the lower arrowscrollbox is supposed to be vertically oriented, and we are setting the attribute as expected. However it's not set. Oddly, if I add this to xul.css it does work:

scrollbox[orient=vertical] {
  -moz-box-orient: vertical;
}

It also works if I move the call to this.init() from the constructor and into the connectedCallback, as detailed in the comment in this patch: https://phabricator.services.mozilla.com/D47465.

Do you have any idea what's going on here? Is there a quick fix we could make to support this? Or should we just not be creating shadow DOM at all in the constructor if it's possible that the element will be loaded via prototype cache?

I'm confused, the orient attribute doesn't change the CSS value at all, so you should get -moz-box-orient: horizontal either way (unless you override it via css): https://searchfox.org/mozilla-central/rev/23f836a71cfe961373c8bd0d0219ec60a64b3c8f/layout/xul/nsBoxFrame.cpp#398

So it's probably not that... To be clear, does unsetting and re-setting the value work as a work-around?

I'd have to do some proper debugging here, I'm not familiar with the prototype cache at all...

Just to follow up on Comment 17. Emilio and I looked into this and:

emilio> I'm moderately sure this is just a XUL layout bug
emilio> should be reproducible without shadow dom too
emilio> Stack of the frame fixing up its internal state to no longer be horizontal: https://www.irccloud.com/pastebin/JiaUzNaL/stacks

bgrins> I could just move it into connectedCallback for now. we sort of interchangeable create DOM in constructor (usually with SD) or the first connectedCallback (usually if it doesn't use SD). I'm wondering now if we should only ever do it on first connectedCallback (after DOMContentLoaded)
bgrins> I'll see how far i can get with that

I'm not yet sure if moving to connectedCallback will cause new issues, and would still like to figure out if this is something that could be fixed at the source.

(In reply to Brian Grinstead [:bgrins] from comment #18)

Just to follow up on Comment 17. Emilio and I looked into this and:

emilio> I'm moderately sure this is just a XUL layout bug
emilio> should be reproducible without shadow dom too
emilio> Stack of the frame fixing up its internal state to no longer be horizontal: https://www.irccloud.com/pastebin/JiaUzNaL/stacks

bgrins> I could just move it into connectedCallback for now. we sort of interchangeable create DOM in constructor (usually with SD) or the first connectedCallback (usually if it doesn't use SD). I'm wondering now if we should only ever do it on first connectedCallback (after DOMContentLoaded)
bgrins> I'll see how far i can get with that

I'm not yet sure if moving to connectedCallback will cause new issues, and would still like to figure out if this is something that could be fixed at the source.

The current patch definitely seems to be triggering a real bug in the XUL element which is worth figuring out, but it seems like if I just remove the call to delayConnectedCallback this starts working without reworking how initialization works. I think I'll just do that for now - delayConnectedCallback is primarily a mechanism to allow our CEs to play nicely with XBL and we have very few bindings left so I don't believe it's necessary here.

on a11y failure (https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=269113099&repo=try&lineNumber=3496):
TEST-UNEXPECTED-FAIL | accessible/tests/mochitest/hittest/test_menu.xul | wrong state bits for 'mi_file1.2.1' !got '0', expected 'invisible'
TEST-UNEXPECTED-FAIL | accessible/tests/mochitest/hittest/test_menu.xul | state bits should not be present in ID 'mi_file1.2.1' !got 'offscreen', expected '0'

the test fails when menu (including submenu) is closed and it checks whether menuitems are invisible (https://searchfox.org/mozilla-central/source/accessible/tests/mochitest/hittest/test_menu.xul#56-66).

Normally, when accessible states are computed for the menuitem, then it is reported hidden as soon as a11y reaches menupopup element, which has a hidden view (https://searchfox.org/mozilla-central/source/accessible/generic/Accessible.cpp#338). After the conversion (with patch applied), a11y returns early claiming that menuitem is scrolled off (https://searchfox.org/mozilla-central/source/accessible/generic/Accessible.cpp#372).

Originally the frame traversal looks this way:

0:22.42 GECKO(11992) Visibility: <menuitem id='mi_file1.2.1'>
0:22.42 GECKO(11992) parentFrame: <scrollbox id=''> 000002886F6F2C60, content: 000002887A81EDC0, size: (0, 0, 7265, 5280)
0:22.42 GECKO(11992) parentFrame: <scrollbox id=''> 000002886F6F2D28, content: 000002887A81EDC0, size: (0, 600, 7265, 2040)
0:22.42 GECKO(11992) parentFrame: <arrowscrollbox id=''> 000002886F6F2A48, content: 000002887A81EB80, size: (180, 180, 7265, 3240)
0:22.42 GECKO(11992) parentFrame: <menupopup id='mp_file1.2'> 000002886F6F1FC0, content: 000002886F6BE4C0, size: (6840, -180, 7625, 3600)
0:22.42 GECKO(11992) view is hidden -> invisible

and broken build version looks this way:
0:21.56 GECKO(10500) Visibility: <menuitem id='mi_file1.2.1'>
0:21.56 GECKO(10500) parentFrame: <scrollbox id=''> 000002681A976C60, content: 000002681F044C10, size: (0, 0, 7265, 5280)
0:21.56 GECKO(10500) parentFrame: <scrollbox id=''> 000002681A976D28, content: 000002681F044C10, size: (0, 1260, 7265, 720)
0:21.56 GECKO(10500) scrolled off 12 pix -> offscreen

so scrollable frame of a scrollbox element indeed has lesser height than it's used to be 720 vs 2040 and different y coordinate 600 vs 1260.

Emilio, any ideas why scrollable frame inside a menupoup may have weird rect?

screenshot of dom tree of a menupopup with the patch applied

screenshot of dom tree of a menupopup without the patch applied

I'm seeing a difference visually in the test with/without the patch so this might just be surfacing a bug. Clearing needinfo while we figure it out.

Backed out changeset 5e221240fc1d (bug 1514926) for mochitest failures in browser/tools/mozscreenshots/primaryUI/browser_primaryUI.js

Log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=269551469&repo=autoland&lineNumber=5632

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&revision=5e221240fc1d537da612973800bbe24efa50736b&selectedJob=269551469

Backout:
https://hg.mozilla.org/integration/autoland/rev/8f8e340e1ff69608a842844702247148fb3e5d02

Maybe something here: https://searchfox.org/mozilla-central/source/browser/tools/mozscreenshots/mozscreenshots/extension/configurations/Tabs.jsm#85 needs updating ?

(In reply to Tim Nguyen :ntim from comment #26)

Maybe something here: https://searchfox.org/mozilla-central/source/browser/tools/mozscreenshots/mozscreenshots/extension/configurations/Tabs.jsm#85 needs updating ?

My guess is this event needs to listen on the arrowScrollbox. I have a try push out at https://treeherder.mozilla.org/#/jobs?repo=try&revision=19e75a94c7b4aa17b9e262f30165d67358c15e7a

This event: https://searchfox.org/mozilla-central/rev/35cc00a25c4471993fdaa5761952bd3afd4f1731/browser/tools/mozscreenshots/mozscreenshots/extension/configurations/Tabs.jsm#141

Yeah, this looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=19e75a94c7b4aa17b9e262f30165d67358c15e7a&selectedJob=269559490. Going to re-land.

https://hg.mozilla.org/mozilla-central/rev/45b2d685bc5f
https://hg.mozilla.org/mozilla-central/rev/6e8f0afa223c