Closed Bug 1454357 Opened 7 years ago Closed 6 years ago

Remove scrollbox binding

Categories

(Toolkit :: UI Widgets, task, P5)

Product:
Toolkit
Component:
UI Widgets
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla66
Tracking Flags:
Tracking Status
firefox66 --- fixed

People

(Reporter: Paolo, Assigned: timdream)

References

Details

Attachments

(2 files, 2 obsolete files)

Bug 1454357 - Remove the feature to set list direction on a rishlistbox
47 bytes, text/x-phabricator-request
Details | Review
Bug 1454357 - Remove the scrollbox binding
47 bytes, text/x-phabricator-request
Details | Review 
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.
Depends on: 1454358
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?
(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
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.
Depends on: 1516258
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.
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
Pushed by tchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3e99aed6c14d
Remove the scrollbox binding r=NeilDeakin

https://hg.mozilla.org/mozilla-central/rev/3e99aed6c14d

Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox66: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
Depends on: 1519160
Component: XUL → XUL Widgets
Product: Core → Toolkit
Type: enhancement → task

«toolkit.tabbox.switchByScrolling",true» not completely working correctly. Moving between tabs with the mouse wheel works only in the visibility zone, when there are a lot of open bookmarks and goes beyond the navigation panel, the function stops working. It is necessary that the mouse wheel works on all tabs and it doesn’t matter how much is open in the navigation bar.
Please fix it.

Forgot to add, I'm using FF 71 x64

(In reply to veron from comment #24)

«toolkit.tabbox.switchByScrolling",true» not completely working correctly. Moving between tabs with the mouse wheel works only in the visibility zone, when there are a lot of open bookmarks and goes beyond the navigation panel, the function stops working. It is necessary that the mouse wheel works on all tabs and it doesn’t matter how much is open in the navigation bar.
Please fix it.

This is tracked in Bug 1601184.

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: