Closed Bug 1432950 Opened 5 years ago Closed 5 years ago
Remove the "scrollbar-base" binding
+++ 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
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 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
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.
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 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+
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Pushed by firstname.lastname@example.org: 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
You need to log in before you can comment on or make changes to this bug.