Remove the "scrollbar-base" binding

RESOLVED FIXED in Firefox 60

Status

()

RESOLVED FIXED
a year ago
a year ago

People

(Reporter: bgrins, Assigned: bgrins)

Tracking

(Blocks: 1 bug)

unspecified
mozilla60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)

Details

(Whiteboard: [xbl-in-content])

Attachments

(2 attachments)

+++ This bug was initially created as a clone of Bug #1431246 +++

Spinning this off as per https://bugzilla.mozilla.org/show_bug.cgi?id=1431246#c12:

There's a scrollbar-base binding that is both attached to <scrollcorner> elements and extended by the "scrollbar" binding. It attaches <handler>s for "contextmenu", "click", "dblclick", and "command" to stopPropagation (and preventDefault for "contextmenu" and "click".

However, this appears to be unnecessary given the existence of `IsEventStoppedFromAnonymousScrollbar`.

- https://dxr.mozilla.org/mozilla-central/rev/b0baaec09caf3e1b30ec6b238f5c46ef9b3188be/toolkit/content/widgets/scrollbar.xml#14-21
Comment hidden (mozreview-request)
Attachment #8945213 - Flags: review?(bzbarsky) → review?(bugs)
Are the scrollbars used also elsewhere, outside native anonymous content?
tree.xml and autocomplete.xml at least use <xul:scrollbar>

Comment 4

a year ago
mozreview-review
Comment on attachment 8945213 [details]
Bug 1432950 - Remove the scrollbar-base binding to prevent XBL JS from running on scrollbars in content

https://reviewboard.mozilla.org/r/215440/#review221088

I'd be rather surprised if this doesn't break <xul:tree> somehow.
(Feel free to explain why this is fine and ask review again.)
Attachment #8945213 - Flags: review?(bugs) → review-
This doesn't remove the scrollbar binding entirely, but rather the scrollbar-base binding that it extends. So it effectively gets rid of the <handler> tags running on <xul:scrollbar> and <xul:scrollcorner>. There's more context in Bug 1431246, Comments 4-12 but AFAICT those event handlers aren't needed since nsXULElement::IsEventStoppedFromAnonymousScrollbar special cases the same events originating from those two elements.

Try appears green with this patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2656fed9dbfaaac297e1568b24108f3d8e4c130d.

I did some manual testing on <textarea> to make sure this didn't regress Bug 70698, and I can do more manual testing specifically on <xul:tree> if you think that could be a problem.
IsEventStoppedFromAnonymousScrollbar is only about native anonymous content, the scrollbar in <tree> isn't native anonymous.
(In reply to Olli Pettay [:smaug] from comment #6)
> IsEventStoppedFromAnonymousScrollbar is only about native anonymous content,
> the scrollbar in <tree> isn't native anonymous.

Ah I see that now - thanks. I'll look more into it, I wonder why this uses a <xul:scrollbar> directly instead of normal overflow.
Because the scrollbar in <tree> doesn't really scroll anything, but just ends up updating the view.
One extra benefit to getting rid of the handlers (in addition to one less binding) is that there won't be any more JS running on scrollbars, so we shouldn't have to create the extra global for XBL execution on pages when a user interacts with the scrollbar. I don't think we have data on it, but I have to assume scrollbars are the most likely cause of in-content XBL running JS (maybe aside from <input> / <textarea> key handling).

One maybe easy way (although I haven't tested it) to get that benefit without solving <tree> would be if we had a new binding for scrollbars in trees that included the handlers.  So instead of:

- scrollbar-base [bindToUntrustedContent]
-- scrollbar [bindToUntrustedContent]

We could make it like this:

- scrollbar [bindToUntrustedContent]
-- scrollbar-tree
Yeah, separating those use-cases makes a lot of sense to me. The bindings are small enough that it seems like it would be fine to also have no inheritance relationship at all, if that were to make anything easier.
One of the three places <xul:scrollbar> is used directly (autocomplete-treerows) will be removed in Bug 1427363
Depends on: 1427363
Comment hidden (mozreview-request)
Attachment #8945213 - Flags: review?(bzbarsky)
Referring to Comment 9, I did some testing with this patch. Doing:

./mach run --profile $(mktemp -d) "data:text/html,<div style='height:1000px'</div>"

Will start printing out the memory used by this page on an interval. I see something like:

Total bytes: 262568
Total bytes: 262568
Total bytes: 262568
...

Then when I click on the scrollbar it goes up to:

Total bytes: 330872

Which lines up with the numbers I was seeing on a single binding in the measurements at https://bugzilla.mozilla.org/show_bug.cgi?id=1426492#c10.

With the patch applied the usage doesn't jump after clicking on the scrollbar. It doesn't save too much, but it's better than nothing.
Whiteboard: [xbl-in-content]
Comment hidden (mozreview-request)

Comment 15

a year ago
mozreview-review
Comment on attachment 8945213 [details]
Bug 1432950 - Remove the scrollbar-base binding to prevent XBL JS from running on scrollbars in content

https://reviewboard.mozilla.org/r/215440/#review225794
Attachment #8945213 - Flags: review?(bugs) → review+

Comment 16

a year ago
mozreview-review
Comment on attachment 8945213 [details]
Bug 1432950 - Remove the scrollbar-base binding to prevent XBL JS from running on scrollbars in content

https://reviewboard.mozilla.org/r/215440/#review226004

::: toolkit/content/widgets/tree.xml:41
(Diff revision 3)
>            <children/>
>          </xul:treerows>
>          <xul:textbox anonid="input" class="tree-input" left="0" top="0" hidden="true"/>
>        </xul:stack>
>        <xul:hbox xbl:inherits="collapsed=hidehscroll">
> -        <xul:scrollbar orient="horizontal" flex="1" increment="16" style="position:relative; z-index:2147483647;"/>
> +        <xul:scrollbar orient="horizontal" flex="1" increment="16" style="position:relative; z-index:2147483647;" oncontextmenu="event.stopPropagation();event.preventDefault();" onclick="event.stopPropagation();event.preventDefault();" ondblclick="event.stopPropagation();" oncommand="event.stopPropagation();"/>

nit: please put each on* attribute on its own line and add a space after the first semi-colon
Attachment #8945213 - Flags: review?(dao+bmo) → review+
Comment hidden (mozreview-request)
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED

Comment 18

a year ago
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7080e442652d
Remove the scrollbar-base binding to prevent XBL JS from running on scrollbars in content;r=dao,smaug

Comment 19

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7080e442652d
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.