1514926 - Convert arrowscrollbox binding to a custom element

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, task, P5)

Description

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Per what's suggested in the meeting, we should be able to flatten arrowscrollbox and arrowscrollbox-clicktoscroll into one custom element, and move tabbrowser-arrowscrollbox to its (sole?) user.

Comment 1

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Bug 1514926 - Create BaseControlMixin, allowing BaseControl to inherit other native XULElement classes

Comment 2

[Tim Guan-tin Chien [:timdream] (please needinfo)]

I've moved https://phabricator.services.mozilla.com/D14925 here because I will need that to be able to inherit from BaseControl.

Other than that, here are the findings:

* The element is not used in XUL documents excepts tests.
* It is used in popup, menu, and tabbrowser bindings.

This is actually a nice use case of Shadow DOM, unfortunately because of bug 1515518 I will need to think of alternatives...

Comment 3

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #2)
> * It is used in popup, menu, and tabbrowser bindings.

Sorry, I should be more specific.

The IDs of the bindings are places-popup-arrow, places-popup-base, popup, and tabbrowser-tabs.

Comment 4

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #2)
> I've moved https://phabricator.services.mozilla.com/D14925 here because I
> will need that to be able to inherit from BaseControl.

... and I was wrong about this.

Comment 5

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Bug 1514926 - (Broken) Convert arrowscrollbox bindings to custom element

On top of bug 1516266. This converts the arrowscrollbox binding and
tabbrowser-arrowscrollbox binding to custom elements.

The debug browser will fail to launch with

Assertion failure: !content->IsActiveChildrenElement(), at /builds/worker/workspace/build/src/layout/base/RestyleManager.cpp:3033

Appearently because we are moving the <children> element in the connected callback.

Comment 6

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Also, trying to locate the children like |document.getBindingParent(this).children;| and move them manually into <scrollbox> also failed with the same assertion error.

https://searchfox.org/mozilla-central/rev/7a922172a94cfe24c7a48e0a581577895e1da8c4/toolkit/content/widgets/scrollbox.xml#349

Comment 7

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Actually, it failed with

Assertion failure: content->GetFlattenedTreeParentNodeForStyle() (Node not in the flattened tree still has a frame?), at /builds/worker/workspace/build/src/layout/base/PresShell.cpp:3946

Comment 8

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Unassigning. This is not actionable for now.

Comment 9

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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.

Comment 10

[alexander :surkov (:asurkov)]

Attachment: Bug 1514926 - convert arrowscrollbox binding to a custom element

Comment 11

[alexander :surkov (:asurkov)]

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.

Comment 12

[Brian Grinstead [:bgrins]]

Attachment: Bug 1514926 - Convert the arrowscrollbox binding to a Custom Element

Comment 13

[Brian Grinstead [:bgrins]]

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

Comment 14

[Brian Grinstead [:bgrins]]

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

Comment 15

[Brian Grinstead [:bgrins]]

Attachment: Bug 1514926 - Patch for debugging unexpected test_mousescroll.xul failure related to the [orient] attribute not being respected from the inner scrollbox within Shadow DOM, when the shadow DOM is initialized in the constructor

Comment 16

[Brian Grinstead [:bgrins]]

Attachment: orient-attribute-and-computed-style.png

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?

Comment 17

[Emilio Cobos Álvarez (:emilio)]

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...

Comment 18

[Brian Grinstead [:bgrins]]

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.

Comment 19

[Brian Grinstead [:bgrins]]

(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.

Comment 20

[alexander :surkov (:asurkov)]

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?

Comment 21

[Brian Grinstead [:bgrins]]

Attachment: with-patch-shadowdom.png

screenshot of dom tree of a menupopup with the patch applied

Comment 22

[Brian Grinstead [:bgrins]]

Attachment: withuot-patch-xbl.png

screenshot of dom tree of a menupopup without the patch applied

Comment 23

[Brian Grinstead [:bgrins]]

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.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5e221240fc1d
Convert the arrowscrollbox binding to a Custom Element r=dao

Comment 25

[Dorel Luca [:dluca]]

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

Comment 26

[Tim Nguyen :ntim]

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

Comment 27

[Brian Grinstead [:bgrins]]

(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

Comment 28

[Brian Grinstead [:bgrins]]

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

Comment 29

[Brian Grinstead [:bgrins]]

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

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/45b2d685bc5f
Convert the arrowscrollbox binding to a Custom Element r=dao

Comment 31

[Tim Nguyen :ntim]

Attachment: Bug 1514926 - Followup: remove now unused arrowscrollbox check from MayNeedToLoadXBLBinding. r=emilio

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6e8f0afa223c
Followup: remove now unused arrowscrollbox check from MayNeedToLoadXBLBinding. r=emilio

Comment 33

[Razvan Maries]

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