Implement scrollbars without XBL

RESOLVED FIXED in Firefox 61

Status

()

P5
normal
RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: bgrins, Assigned: timdream)

Tracking

(Blocks: 4 bugs)

unspecified
mozilla61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

(Whiteboard: [xbl-in-content])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

a year ago
Scrollbars are one of the relatively few XBL bindings that get created in content documents. Here are some notes from conversations with roc and botond about our options for replacing the scrollbar / scrollbar-base bindings (https://searchfox.org/mozilla-central/source/toolkit/content/widgets/scrollbar.xml):

> I don't think there's anything particularly difficult.
> As you'll see looking at the scrollbar XBL,
> there are some features that get turned on or off depending
> on platform, like which buttons are visible, but IIRC it should
> be reasonably straightforward to replicate everything, you'll just
> need to be careful with testing across platforms.
> 
> One question is whether to replicate the XBL setup using native anonymous
> content or go to a model where there's a single scrollbar element and frame 
> which does everything including render the thumb and buttons etc. The latter is 
> more work but would give more performance benefits, especially on pages with 
> lots of overflow:auto scrollbars (including pages with lots of text inputs). 
> Another option between the two would be to have the scrollbar frame lazily 
> create its anonymous content the first time it's unhidden.

And:

> I assume that the proposed refactoring would preserve the ability for us to
> build a custom frame type with custom logic for scrollbars (like nsSliderFrame today).
> The only important consideration that I can think of for async
> scrolling, is that the scrollbar thumb continue getting its own
> display item (and if appropriate, its own layer) so that it can be
> moved by the compositor during async scrolling. I *believe* that's
> orthogonal to whether we use multiple frames for the scrollbar
> components, or a single frame (since a single frame can build multiple
> display items), and so you should feel free to choose either of
> those strategies.
(Reporter)

Comment 1

a year ago
Just a note that the "scrollbar-base" binding is attached to scrollcorner elements with OSX native scrollbars (https://dxr.mozilla.org/mozilla-central/rev/e4107773cffb1baefd5446666fce22c4d6eb0517/toolkit/themes/osx/global/nativescrollbars.css#76) and Windows "xul scrollbars" (unsure exactly when those are and aren't used):

* https://dxr.mozilla.org/mozilla-central/rev/e4107773cffb1baefd5446666fce22c4d6eb0517/toolkit/themes/osx/global/nativescrollbars.css#76
* https://dxr.mozilla.org/mozilla-central/source/toolkit/themes/windows/global/xulscrollbars.css#68

It's possible a first step would be to preventDefault / stopPropagation as appropriate from C++ for the scrollbar/scrollcorner elements and then delete scrollbar-base altogether:

      <handler event="contextmenu" preventdefault="true" action="event.stopPropagation();"/>
      <handler event="click" preventdefault="true" action="event.stopPropagation();"/>
      <handler event="dblclick" action="event.stopPropagation();"/>
      <handler event="command" action="event.stopPropagation();"/>
(Reporter)

Comment 2

a year ago
Neil, is it possible we could remove the "thumb" binding at https://searchfox.org/mozilla-central/source/toolkit/content/widgets/scrollbar.xml#12? It's bound to the "thumb" element at https://searchfox.org/mozilla-central/source/toolkit/content/minimal-xul.css#91. I'm not sure exactly what `extends="xul:button"` is doing - is this something we could delete and/or move into XUL <thumb> directly instead of through XBL?
Flags: needinfo?(enndeakin)
That line causes the element to get a nsButtonBoxFrame instead of nsBoxFrame.

But you should be able to easily remove the need for a binding by adding a mapping for <thumb> in nsCSSFrameConstructor::FindXULTagData, and also adding to the whitelist at nsXBLPrototypeBinding::CheckTagNameWhiteList
Flags: needinfo?(enndeakin)
(Reporter)

Comment 4

a year ago
(In reply to Brian Grinstead [:bgrins] from comment #1)
> It's possible a first step would be to preventDefault / stopPropagation as
> appropriate from C++ for the scrollbar/scrollcorner elements and then delete
> scrollbar-base altogether:
> 
>       <handler event="contextmenu" preventdefault="true"
> action="event.stopPropagation();"/>
>       <handler event="click" preventdefault="true"
> action="event.stopPropagation();"/>
>       <handler event="dblclick" action="event.stopPropagation();"/>
>       <handler event="command" action="event.stopPropagation();"/>

It's not clear to me why we need these currently. There's a function `nsXULElement::IsEventStoppedFromAnonymousScrollbar` that targets these events (and additionally eDragStart and eMouseAuxClick) in `GetEventTargetParent` with the comment: "// Don't propagate these events from native anonymous scrollbar.".

I've removed the binding locally and don't see click events go through to the document when I click on the scrollbar. Or are these <handler>s needed for some other reason?
Flags: needinfo?(bzbarsky)
(Reporter)

Comment 5

a year ago
If I remove the binding *and* return false in IsEventStoppedFromAnonymousScrollbar then I do see the click event go through to the page when I click on the scrollbar slider (but not on the thumb). If either the binding is applied or the current IsEventStoppedFromAnonymousScrollbar impl is in place, then I don't see the click event go through. I'm not sure how to see <scrollcorner>s to test but I assume it's the same behavior looking at the code.
(Reporter)

Updated

a year ago
Depends on: 1431522
Do default actions still happen?  For example, does bug 70698 regress?
Flags: needinfo?(bzbarsky)
(Reporter)

Comment 7

a year ago
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #6)
> Do default actions still happen?  For example, does bug 70698 regress?

If I'm reading that test case correctly, then the STR is:

1) Load `data:text/html,<textarea>lots of text causing wrapping to happen and a scrollbar to show up</textarea>`
2) Select the text from the urlbar
3) Drag the selected text onto the scrollbar of the textarea
4) Something bad happens with the scrollbar (either it disappears or the pasted text appears on it)

I've only tested this on OSX, but I can't reproduce this in any configuration (both with the binding removed and with return false in IsEventStoppedFromAnonymousScrollbar as in Comment 5).

The only weird behavior I see, and I see this even in a Nightly build, is that if I move the scrollbar thumb up to the top of the slider and then drop the text onto the thumb itself, the textarea gets scrolled to the bottom. I'll attach a gif since it's a little hard to explain in text.
(Reporter)

Comment 8

a year ago
As mentioned in Comment 7
(Reporter)

Comment 9

a year ago
I believe removing scrollbar-base would have another benefit: that's the only script that runs in scrollbars, which means we wouldn't need to create an extra JS global for XBL script execution that happens once those handlers runs. It's not a ton of memory, but it is noticeable as in https://bugzilla.mozilla.org/show_bug.cgi?id=1426492#c7
> 3) Drag the selected text onto the scrollbar of the textarea

No, middle-click on the scrollbar of the textarea.  Only testable on Linux.
Comment hidden (mozreview-request)
(Reporter)

Comment 12

a year ago
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #10)
> > 3) Drag the selected text onto the scrollbar of the textarea
> 
> No, middle-click on the scrollbar of the textarea.  Only testable on Linux.

I did some manual testing in a debug build and here's what I found:

1) m-c tip: Middle click on the textarea scrollbar does nothing
2) with the <handler>s removed: Middle click on the textarea scrollbar does nothing
3) with the <handler>s removed and IsEventStoppedFromAnonymousScrollbar returning false: Middle click on the textbox scrollbar pastes contents of the clipboard to inside the textarea wherever the cursor is.

I couldn't reproduce disappearing scrollbars, text being pasted in the wrong place, or assertion failures in any of the cases. But since (2) seems to behave the same as (1) and the try push with that change (https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba79fb07ce9fe45bd73bd88d2be3fd6a7829cd3f) doesn't show anything obvious I will spin removing that binding off into another bug where we can do further testing if needed.
(Reporter)

Updated

a year ago
Depends on: 1432950
(Reporter)

Updated

a year ago
Whiteboard: [xbl-in-content]
(Reporter)

Updated

a year ago
Depends on: 1442507
Comment hidden (mozreview-request)
(Reporter)

Updated

a year ago
Attachment #8943784 - Attachment is obsolete: true
(Reporter)

Updated

a year ago
Depends on: 1442695

Updated

a year ago
Blocks: 1443836
(Reporter)

Comment 14

a year ago
Now that scrollbar-base and thumb are gone, maybe we can already implement the first approach: "replicate the XBL setup using native anonymous content". AFAICT there aren't cases where we have a scrollbar that doesn't get the binding / anonymous content attached based on these selectors: https://searchfox.org/mozilla-central/search?q=scrollbar.xml%23scrollbar&path=.
(Reporter)

Updated

a year ago
Blocks: 1446829

Updated

a year ago
Depends on: 77790
I would like to give it a shot after bug 1450017.
(Reporter)

Comment 16

a year ago
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #15)
> I would like to give it a shot after bug 1450017.

Great! By the way, I have a work in progress patch that replicates the XBL anon content entirely in NAC at https://hg.mozilla.org/try/rev/55782b48d733a93304e0e23ffe91241b63ca7b3b, but it doesn't support non-anonymous scrollbars. I guess whatever you figure out for non-anonymous resizers in bug 1450017 should apply here as well.
(Reporter)

Comment 17

a year ago
Also FYI I'm working to get more details on a longer term plan to implement the entire scrollbar in a single frame. Specifically:

* How much work will it be?
* How can we support forced overlay scrollbars without CSS styling (i.e. devtools Responsive Design Mode and geckoview)?
* How can we support styled native scrollbars for the devtools dark theme and possibly other dark scrollable areas in chrome? I talked with Tantek about this and we think the CSS scrollbars spec can help here.

Regardless, I think proceeding with 'replicate the XBL anon content in NAC' approach in this bug is right - it's a way to make quicker progress on removing the in-content XBL and I don't think it'll make the longer term plan any harder.
Comment hidden (obsolete)
Comment hidden (obsolete)
My currently is that I can create the children by implementing nsScrollbarFrame::CreateAnonymousContent(). Still figuring it out...
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #20)
> My currently is 

* My current idea is

The patch posted is what I've worked out today. The markup is created correctly but the attributes is not copied correctly, I think. I will move things into nsScrollbarFrame (the Brian's changes in nsGfxScrollFrame).

Comment 23

a year ago
I’m quoting my comment from bug 77790 here, so that it may be addressed while this bug is being worked on:

(In reply to ExE Boss from bug 77790 comment #218)
> With the XBL implementation of scrollbars being removed in bug 1431246 with
> plans to replace it using a native anonymous content implementation, it
> might be a good idea to figure out how to implement this during the planning
> phase, rather than trying to shoehorn it into a finished implementation,
> which would then likely result in this getting stalled for another 17 years
> or more.
(In reply to ExE Boss from comment #23)
> I’m quoting my comment from bug 77790 here, so that it may be addressed
> while this bug is being worked on:
> 
> (In reply to ExE Boss from bug 77790 comment #218)
> > With the XBL implementation of scrollbars being removed in bug 1431246 with
> > plans to replace it using a native anonymous content implementation, it
> > might be a good idea to figure out how to implement this during the planning
> > phase, rather than trying to shoehorn it into a finished implementation,
> > which would then likely result in this getting stalled for another 17 years
> > or more.

+1

Being able to style scrollbars would be an advantage for APZ as well, because it would mean that some sites that currently feel the need to reimplement scrollbars in JS to get custom styling (and consequently get janky main-thread scrolling) could use native scrollbars with APZ scrolling instead.
(In reply to ExE Boss from comment #23)
> I’m quoting my comment from bug 77790 here, so that it may be addressed
> while this bug is being worked on:

Thank you for bringing this to my attention. The approach I am aiming here currently does not make this feature any harder or easier to implement (maybe easier since nsScrollBarFrame should have easier access to style info than the XBL binding?), and I don't have a strong opinion on the proposed feature.
Assignee: nobody → timdream
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
The current patch has a working scrollbar, but with failed tests

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

The tests are failing because the script can no longer get the scrollbar children with document.getAnonymousElementByAttribute().

I tried to replace the function call with SpecialPowers.unwrap(SpecialPowers.InspectorUtils.getChildrenForNode(scrollbar, true)[...]) but the test would fail when it reaches synthesizeMouseAtCenter().

The assertion is Assertion failure: aChild->GetProperty(nsGkAtoms::restylableAnonymousNode) (Someone passed native anonymous content directly into frame construction.  Stop doing that!), at /Users/timdream/repo/gecko/central/layout/base/nsCSSFrameConstructor.cpp:6658
(Reporter)

Comment 28

a year ago
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #27)
> The current patch has a working scrollbar, but with failed tests
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=bfc3a92c7df07b3a1c1f3a803c5c1d36e6fff474
> 
> The tests are failing because the script can no longer get the scrollbar
> children with document.getAnonymousElementByAttribute().

You should be able to use deepTreeWalker with `showAnonymousContent=true` and `currentNode=scrollbar` and then traverse down to the node you want if a test really needs to dig into the anon content. There's some similar code in tab-content.js here: https://bugzilla.mozilla.org/page.cgi?id=splinter.html&ignore=&bug=1431255&attachment=8955281.
(In reply to Brian Grinstead [:bgrins] from comment #28)
> You should be able to use deepTreeWalker with `showAnonymousContent=true`
> and `currentNode=scrollbar` and then traverse down to the node you want if a
> test really needs to dig into the anon content. There's some similar code in
> tab-content.js here:
> https://bugzilla.mozilla.org/page.cgi?id=splinter.
> html&ignore=&bug=1431255&attachment=8955281.

I have no problem accessing the DOM node in a different way, it's just that I don't think I can mutate it without hitting the assertion. I am trying to see if I can fix the tests to see if I can interact with the node indirectly.
(Reporter)

Comment 30

a year ago
(In reply to Botond Ballo [:botond] from comment #24)
> (In reply to ExE Boss from comment #23)
> > I’m quoting my comment from bug 77790 here, so that it may be addressed
> > while this bug is being worked on:
> > 
> > (In reply to ExE Boss from bug 77790 comment #218)
> > > With the XBL implementation of scrollbars being removed in bug 1431246 with
> > > plans to replace it using a native anonymous content implementation, it
> > > might be a good idea to figure out how to implement this during the planning
> > > phase, rather than trying to shoehorn it into a finished implementation,
> > > which would then likely result in this getting stalled for another 17 years
> > > or more.
> 
> +1
> 
> Being able to style scrollbars would be an advantage for APZ as well,
> because it would mean that some sites that currently feel the need to
> reimplement scrollbars in JS to get custom styling (and consequently get
> janky main-thread scrolling) could use native scrollbars with APZ scrolling
> instead.

Yes, and we'd like this feature for the devtools dark theme as well (which currently styles the XBL anon content directly). As Tim mentioned in Comment 25, the approach here to remove XBL won't really change the prospects of custom styling.

Longer term I'd like to move away building the DOM in NAC and instead use a single element / layout frame for the entire scrollbar. This is for perf reasons and also to move away from in-content XUL usage. I think that work will be blocked on styling support since we currently style the anon content using CSS in places like the devtools dark theme and also to get overlay-style scrollbars in Responsive Design Mode and geckoview.
Comment hidden (mozreview-request)
(In reply to Brian Grinstead [:bgrins] from comment #16)
> Great! By the way, I have a work in progress patch that replicates the XBL
> anon content entirely in NAC at
> https://hg.mozilla.org/try/rev/55782b48d733a93304e0e23ffe91241b63ca7b3b, but
> it doesn't support non-anonymous scrollbars. I guess whatever you figure out
> for non-anonymous resizers in bug 1450017 should apply here as well.

Brian, could you help me understand why this rule is added? Thanks.

https://hg.mozilla.org/try/rev/55782b48d733a93304e0e23ffe91241b63ca7b3b#l5.12

It's not obvious to me why this is needed...
Flags: needinfo?(bgrinstead)
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #29)
> I have no problem accessing the DOM node in a different way, it's just that
> I don't think I can mutate it without hitting the assertion. I am trying to
> see if I can fix the tests to see if I can interact with the node indirectly.

My idea worked locally; for the nodes created in nsScrollbarFrame, tests are free to do getClientBoundingRect() on it, but not to change its inline styles directly, nor send it to synthesizeMouseAtCenter().

I've replaced them the style change with injecting an UA sheet, and change the synthesizeMouse*() calls with synthesizeMouseAtPoint() with coordinates calculated from the bounding rect.

Let's see what Try says...
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #27)
> The current patch has a working scrollbar, but with failed tests
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=bfc3a92c7df07b3a1c1f3a803c5c1d36e6fff474
> 
> The tests are failing because the script can no longer get the scrollbar
> children with document.getAnonymousElementByAttribute().
> 
> I tried to replace the function call with
> SpecialPowers.unwrap(SpecialPowers.InspectorUtils.
> getChildrenForNode(scrollbar, true)[...]) but the test would fail when it
> reaches synthesizeMouseAtCenter().
> 
> The assertion is Assertion failure:
> aChild->GetProperty(nsGkAtoms::restylableAnonymousNode) (Someone passed
> native anonymous content directly into frame construction.  Stop doing
> that!), at
> /Users/timdream/repo/gecko/central/layout/base/nsCSSFrameConstructor.cpp:6658

Fwiw, that happens because you're notifying of attribute changes, one of those notifications causes a reframe, and since NAC doesn't appear on child lists we don't know how to get the order right.
Comment hidden (mozreview-request)
There are still a bunch of APZ-related test failures on Android.

For the ones that marked as fails-if with a bug filed, I've modified the manifests accordingly, but we would need to check if the test result is legit.

For some other test failure, I've left a comment each on the manifests. I don't really understand what my patch did to APZ on Android; I will try to reach out if I couldn't figure it out.
The APZ-related android reftests that are UNEXPECTED-PASS in your try push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f5b91e44409d74af08d1af797d2c00c597fd61d are all checking that the scrollbar is positioned correctly at different viewport widths and with async scrolling. Most of those tests were passing fuzzily before, so either your patch eliminated the fuzziness, or made the scrollbar disappear entirely so that the test/reference image end up identical.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #37)

Thanks for the quick response.

Not all of them I think, if you look at my patch and Cmd+F for "Fail on Android" you would see that they are failures that only happens on Android...

I also don't know about the mochitests...
(Reporter)

Comment 39

a year ago
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #32)
> (In reply to Brian Grinstead [:bgrins] from comment #16)
> > Great! By the way, I have a work in progress patch that replicates the XBL
> > anon content entirely in NAC at
> > https://hg.mozilla.org/try/rev/55782b48d733a93304e0e23ffe91241b63ca7b3b, but
> > it doesn't support non-anonymous scrollbars. I guess whatever you figure out
> > for non-anonymous resizers in bug 1450017 should apply here as well.
> 
> Brian, could you help me understand why this rule is added? Thanks.
> 
> https://hg.mozilla.org/try/rev/55782b48d733a93304e0e23ffe91241b63ca7b3b#l5.12
> 
> It's not obvious to me why this is needed...

This was so we didn't have to emulate the [inherits="collapsed=disabled"] behavior currently on the <thumb> (which would collapse the thumb when the scrollbar is disabled). That said, since the scrollbar itself is hidden it's probably unnecessary.
Flags: needinfo?(bgrinstead)
(In reply to Brian Grinstead [:bgrins] from comment #39)
> This was so we didn't have to emulate the [inherits="collapsed=disabled"]
> behavior currently on the <thumb> (which would collapse the thumb when the
> scrollbar is disabled). That said, since the scrollbar itself is hidden it's
> probably unnecessary.

I see. No worries then, I've set/unset the collapsed attribute from nsScrollbarFrame.
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #38)
> (In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #37)
> 
> Thanks for the quick response.
> 
> Not all of them I think, if you look at my patch and Cmd+F for "Fail on
> Android" you would see that they are failures that only happens on Android...
> 
> I also don't know about the mochitests...

I am now more convinced that I have broken APZ on Android than accidentally fixing it somehow. That should explain the mochitest failures and the absence of fuzziness...
Comment hidden (mozreview-request)
After building Fennec and try to run Fennec on it (hit by bug 1440714 along the way), I realize the errors are likely caused by logic error in |nsScrollbarFrame::UpdateChildrenAttributeValue()|, where I try to set the attributes on children even when the attribute is unset on <scrollbar>.

I've reverted all the changes on reftest.list and mochitest.ini. Tests I tried had pass locally. Finger crossed for the Try result!
Comment hidden (mozreview-request)
... and we have a winner on the cause of Android failures! I didn't remove -moz-binding property from the geckoview content.css here:

https://searchfox.org/mozilla-central/rev/9f3da81290054c5b8955bb67ff98cae66676f745/mobile/android/themes/geckoview/content.css#24

I have no idea why that didn't cause the test to fail locally though. It's probably because of the known defect of our Android build script (where we don't really remove a file from the package without a clobber build), but it's irreverent now.
(Reporter)

Comment 46

11 months ago
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #45)
> ... and we have a winner on the cause of Android failures! I didn't remove
> -moz-binding property from the geckoview content.css here:

Good find - that would definitely cause problems :)

Updated

11 months ago
Blocks: 1432935
Feel free to add additional reviewers. Thanks.

Comment 48

11 months ago
As per comment 25 and comment 30, bug 77790/bug 1432935 will be worked on after this is resolved.
Blocks: 77790
No longer depends on: 77790
Comment on attachment 8966202 [details]
Bug 1431246 - Create and update scrollbar markup in nsScrollbarFrame

https://reviewboard.mozilla.org/r/234960/#review242402

::: layout/xul/nsScrollbarFrame.cpp:329
(Diff revision 6)
> +  Element* el(GetContent()->AsElement());
> +
> +  // If there are children already in the node, don't create any anonymous content
> +  // (this only apply to crashtests/369038-1.xhtml)
> +  if (el->HasChildren()) {
> +    return NS_OK;

Q: Do we still care about that crashtest when no one but us can use XUL? How about replacing this with `NS_ERROR()` and forbid any XUL document from having non-anon children in the `<scrollbar />`?

Updated

11 months ago
See Also: → bug 1454363
Priority: -- → P5
(Reporter)

Updated

11 months ago
Blocks: 645563

Comment 50

11 months ago
mozreview-review
Comment on attachment 8966202 [details]
Bug 1431246 - Create and update scrollbar markup in nsScrollbarFrame

https://reviewboard.mozilla.org/r/234960/#review244678

::: layout/xul/nsScrollbarFrame.cpp:373
(Diff revision 6)
> +  mSlider->SetAttr(kNameSpaceID_None, nsGkAtoms::orient, orient, false);
> +  mSlider->SetAttr(kNameSpaceID_None, nsGkAtoms::flex,
> +                   NS_LITERAL_STRING("1"), false);
> +  mSlider->SetAttr(kNameSpaceID_None, nsGkAtoms::align,
> +                   NS_LITERAL_STRING("center"), false);
> +  mSlider->SetAttr(kNameSpaceID_None, nsGkAtoms::pack,

The pack and align are on the thumb, not the slider.

::: layout/xul/test/test_bug159346.xul:29
(Diff revision 6)
> +function synthesizeMouseAtButton(aEvent) {
> +  // Target at <scrollbarbutton sbattr="scrollbar-down-bottom">
> +  var rect = SpecialPowers.unwrap(
> +    SpecialPowers.InspectorUtils.getChildrenForNode(scrollbar, true)[4])
> +      .getBoundingClientRect();
> +    synthesizeMouseAtPoint(rect.x + rect.width / 2, rect.y + rect.height / 2, aEvent);

Does synthesizeMouseAtCenter not work here?

::: layout/xul/test/test_bug159346.xul:33
(Diff revision 6)
> +      .getBoundingClientRect();
> +    synthesizeMouseAtPoint(rect.x + rect.width / 2, rect.y + rect.height / 2, aEvent);
> +}
> +
> +function synthesizeMouseOutsideScrollbar(aEvent) {
> +  // Target at <scrollbarbutton sbattr="scrollbar-down-bottom">

I think this comment should say that the target is outside the scrollbar

::: layout/xul/test/test_bug703150.xul:25
(Diff revision 6)
>  
> +function synthesizeMouseAtThumb(aEvent) {
> +  var thumbRect = SpecialPowers.unwrap(
> +    SpecialPowers.InspectorUtils.getChildrenForNode(scrollbar, true)[2])
> +      .childNodes[0].getBoundingClientRect();
> +    synthesizeMouseAtPoint(thumbRect.x + thumbRect.width / 2, thumbRect.y + thumbRect.height / 2, aEvent);

Similar here.
Attachment #8966202 - Flags: review?(enndeakin)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 54

11 months ago
mozreview-review
Comment on attachment 8966202 [details]
Bug 1431246 - Create and update scrollbar markup in nsScrollbarFrame

https://reviewboard.mozilla.org/r/234960/#review244978

::: layout/xul/nsScrollbarFrame.cpp:436
(Diff revision 9)
> +
> +  nsAutoString value;
> +  el->GetAttr(kNameSpaceID_None, aAttribute, value);
> +  bool hasAttr = el->HasAttr(kNameSpaceID_None, aAttribute);
> +
> +  if (!hasAttr) {

This conditional block returns earlier when no attribute is present. You could remove the hasAttr variable entirely I think and just call HasAttr in the condition.

::: layout/xul/nsScrollbarFrame.cpp:477
(Diff revision 9)
> +    if (mDownBottomButton) {
> +      mDownBottomButton->SetAttr(kNameSpaceID_None, aAttribute, value, aNotify);
> +    }
> +  }
> +  else if (aAttribute == nsGkAtoms::disabled) {
> +    if (hasAttr) {

Then remove the hasAttr reference here, as it is always true at this point.

::: layout/xul/nsScrollbarFrame.cpp:487
(Diff revision 9)
> +        mDownTopButton->SetAttr(kNameSpaceID_None, aAttribute, value, aNotify);
> +      }
> +      if (mSlider) {
> +        mSlider->SetAttr(kNameSpaceID_None, aAttribute, value, aNotify);
> +      }
> +      if (mThumb) {

You might want to add a comment here so someone modifying this later notices the subtle attribute difference.

::: testing/mochitest/tests/SimpleTest/EventUtils.js:145
(Diff revision 9)
>   * sendMouseEvent({type:'click'}, 'node');
>   */
>  function getElement(id) {
>    return ((typeof(id) == "string") ?
> -    document.getElementById(id) : id); 
> -};   
> +    document.getElementById(id) : id);
> +};

this whitespace-only change probably doesn't belong here
Attachment #8966202 - Flags: review?(enndeakin) → review+
Comment hidden (mozreview-request)
Comment hidden (obsolete, typo)
Flags: needinfo?(timdream)

Comment 57

11 months ago
Pushed by timdream@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/d3b5dfca6a90
Create and update scrollbar markup in nsScrollbarFrame r=enndeakin+6102

Comment 58

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d3b5dfca6a90
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Depends on: 1457414
You need to log in before you can comment on or make changes to this bug.