Status

()

task
P5
normal
RESOLVED FIXED
Last year
19 days ago

People

(Reporter: Paolo, Assigned: timdream)

Tracking

(Blocks 1 bug)

unspecified
mozilla66
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox66 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Reporter

Description

Last year
The "scrollbox" binding just contains a box, while the "scrollbox" element provides access to some native functions through the ScrollBoxObject object.

We could remove the binding by updating the consumers to use just a simple box, although we may have to update some styling accordingly.
Reporter

Updated

Last year
Depends on: 1454358

Updated

Last year
Priority: -- → P5
Moving to Core:XUL per https://bugzilla.mozilla.org/show_bug.cgi?id=1455336
Component: XP Toolkit/Widgets: XUL → XUL
Depends on: 1458584
The element cannot be removed because some of the implementations have moved to XULScrollElement in bug 1454358. We have to keep the tag name for this.

What's the right approach here instead? Should we have all the consumer construct the <box> element on their own? Should we convert the remaining tiny binding to custom element?
Reporter

Comment 3

6 months ago
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #2)
> The element cannot be removed because some of the implementations have moved
> to XULScrollElement in bug 1454358. We have to keep the tag name for this.

If I remember correctly, callers that rely on XULScrollElement can use DOM functions instead, sometimes reusing existing JavaScript variants of the XULScrollElement methods.

> What's the right approach here instead? Should we have all the consumer
> construct the <box> element on their own? Should we convert the remaining
> tiny binding to custom element?

This bug is about getting rid of XULScrollElement, but it doesn't exclude converting the binding in a different bug.
I am a bit confused. Removing the XULScrollElement class that was just implemented in C++?

Anyway, given that we are not going to remove the tag name, and we will not be able to remove the one and only <box> element in anon content, I am going to try to convert the remaining binding into custom element, with <content> into Shadow DOM.

It would be the first instance of Shadow DOM usage in chrome, we will be able to see what breaks.
Assignee: nobody → timdream
Status: NEW → ASSIGNED
Summary: Consider removing the "scrollbox" element → Convert scrollbox binding into custom element and put anonymous content in Shadow DOM
This does what's intended, moving scrollbox to a custom element implementation

The debug browser will fail to launch with

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

and the non-debug browser will not render the tabs at all.

With the inspector I can see non-XBL instance of <scrollbox> can be constructed correctly. We'll need to figure out how
XBL anonymous content and Shadow DOM slots could work together.

Depends on D14925
The other approach I am exploring right now is to remove the anonymous content altogether.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4510107ce3633d22237da1170f6819a266f2eac&selectedJob=217805141

Looks like it's ... manageable? Correcting _scrollbox.boxObject.height and add padding into it should make the failures go away.

I am running mozscreenshots right now to see if there are uncaught changes.

https://screenshots.mattn.ca/compare/?oldProject=try&oldRev=1b90cc2ca758a0c05323b4da4bf175fae6041ea8&newProject=try&newRev=5cee57b9320767b9c8ed5f57deb4256119a79d63
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #6)
> Created attachment 9032307 [details]
> Bug 1454357 - (Broken) Convert scrollbox to custom element
> 
> This does what's intended, moving scrollbox to a custom element
> implementation
> 
> The debug browser will fail to launch with
> 
> Assertion failure: !content->IsActiveChildrenElement(), at
> /builds/worker/workspace/build/src/layout/base/RestyleManager.cpp:3033
> 
> and the non-debug browser will not render the tabs at all.
> 
> With the inspector I can see non-XBL instance of <scrollbox> can be
> constructed correctly. We'll need to figure out how
> XBL anonymous content and Shadow DOM slots could work together.

Even if we end up being able to remove the anonymous node in this case (as per Comment 7), I'd like to get a bug on file about this issue since it's going to prevent us from using SD in cases where it's not possible to remove the anonymous content (stuff like <wizard> as an example).
(In reply to Brian Grinstead [:bgrins] from comment #8)
> Even if we end up being able to remove the anonymous node in this case (as
> per Comment 7), I'd like to get a bug on file about this issue since it's
> going to prevent us from using SD in cases where it's not possible to remove
> the anonymous content (stuff like <wizard> as an example).

Yeah. I am fairly sure I can come up with a test case for this.
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #9)
> Yeah. I am fairly sure I can come up with a test case for this.

Please see test case and bug at bug 1515518.
Attachment #9032306 - Attachment is obsolete: true
Attachment #9032307 - Attachment is obsolete: true
The patch in comment 7 breaks bug 490178, i.e. <richlistbox dir="reverse">. For some reasons without the <box> as anonymous content, the direction of the list doesn't really update sometimes.

For that, I am more leaning toward removing the feature altogether since it is not used.

I am still looking at other test failures.
It also breaks the assumed frame tree expected by XULScrollElement::ScrollByIndex() I think. I am trying to figure out a fix.
This undo the change implemented in bug 490178 (f8f1a9113a1c) because there is no user of this feature.

For some reason when scrollbox has its inner box removed, XUL doesn't really respect the direction set, and the rendering of the items (now the direct children of scrollbox) will be incorrect.
With all the previous efforts, the scrollbox binding now does nothing but to create an inner box element holding the scrolling content.

It turned out that inner box be easily removed. The padding set by the document sheets can be moved to the srollbox element directly.

The only gatcha is XULScrollElement::ScrollByIndex() -- it can now reach the child item frames directly from the scrolled frame.

Depends on D15169
Comment hidden (obsolete)
Reporter

Comment 17

6 months ago
Note that I'm removing the last consumer of XULScrollElement::ScrollByIndex in bug 1454360.
Depends on: 1454360
Cool, if so removing the method makes more sense.
Reporter

Updated

6 months ago
Depends on: 1516258
Reporter

Comment 19

6 months ago
Since you're touching the scrollbox styling anyways, you may also consider converting scrollbox uses to simple boxes and remove styling dependencies on the "scrollbox" tag. I'm removing XULScrollElement in bug 1516258, although "richlistbox" was already the only remaining consumer after bug 1454360.
Sure, I'll wait for dependencies to settle.
Reporter

Updated

6 months ago
Depends on: 1516763

Rebased.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=69acae3335f7555dea62348289f587f6c8f70bfe

Let's uncomplicate things by breaking up the dependency —- I believe this and bug 1516258 can land independently (and easily rebased on top of).

No longer depends on: 1516258

Comment 22

6 months ago
Pushed by tchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3e99aed6c14d
Remove the scrollbox binding r=NeilDeakin

Comment 23

6 months ago
bugherder
Status: ASSIGNED → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66

Updated

6 months ago
Depends on: 1519160
Component: XUL → XUL Widgets
Product: Core → Toolkit

Updated

19 days ago
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.