Closed Bug 1514926 Opened 2 years ago Closed 10 months ago

Convert arrowscrollbox binding to a custom element

Categories

(Toolkit :: XUL Widgets, task, P5)

task

Tracking

()

RESOLVED FIXED
mozilla71
Tracking Status
firefox71 --- fixed

People

(Reporter: timdream, Assigned: bgrins)

References

(Blocks 2 open bugs)

Details

Attachments

(5 files, 4 obsolete files)

47 bytes, text/x-phabricator-request
Details | Review
499.48 KB, image/png
Details
230.43 KB, image/png
Details
265.82 KB, image/png
Details
47 bytes, text/x-phabricator-request
Details | Review
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.
Priority: -- → P5
Attachment #9033043 - Attachment is obsolete: true
Attachment #9033043 - Attachment is obsolete: false
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...
(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.
Depends on: 1516266
Attachment #9033043 - Attachment is obsolete: true
(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.
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.
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
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

Unassigning. This is not actionable for now.

Assignee: timdream → nobody
Status: ASSIGNED → NEW
Attachment #9033363 - Attachment is obsolete: true

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.

Depends on: 1539665
Summary: Considering simply arrowscrollbox bindings → Consider simplying arrowscrollbox bindings
Summary: Consider simplying arrowscrollbox bindings → Convert arrowscrollbox binding to a custom element
Type: enhancement → task

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.

Depends on: 1505489
Blocks: 1566675
Depends on: 1555497
Attachment #9088501 - Attachment description: Bug 1514926 - Convert arrowscrollbox binding to a Custom Element with Shadow DOM → Bug 1514926 - Convert thee arrowscrollbox binding to a Custom Element
Attachment #9088501 - Attachment description: Bug 1514926 - Convert thee arrowscrollbox binding to a Custom Element → Bug 1514926 - Convert the arrowscrollbox binding to a Custom Element

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?

Flags: needinfo?(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...

Flags: needinfo?(emilio)

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.

Attachment #9069966 - Attachment is obsolete: true
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Attachment #9096964 - Attachment is obsolete: true
Blocks: 1585409

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?

Flags: needinfo?(emilio)

screenshot of dom tree of a menupopup with the patch applied

Attached image withuot-patch-xbl.png

screenshot of dom tree of a menupopup without the patch applied

Attachment #9098289 - Attachment is patch: false
Attachment #9098289 - Attachment mime type: text/plain → image/png
Attachment #9098290 - Attachment is patch: false
Attachment #9098290 - Attachment mime type: text/plain → image/png

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.

Flags: needinfo?(emilio)
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5e221240fc1d
Convert the arrowscrollbox binding to a Custom Element r=dao

(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

Flags: needinfo?(bgrinstead)
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/45b2d685bc5f
Convert the arrowscrollbox binding to a Custom Element r=dao
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/6e8f0afa223c
Followup: remove now unused arrowscrollbox check from MayNeedToLoadXBLBinding. r=emilio
Status: ASSIGNED → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla71
See Also: → 1456093
Regressions: 1589089
Regressions: 1600773
Regressions: 1601184
Regressions: 1607181
Regressions: 1592798
No longer regressions: 1592798
Blocks: 1625464
You need to log in before you can comment on or make changes to this bug.