1431246 - Implement scrollbars without XBL

Status: RESOLVED FIXED

(Core :: XUL, task, P5)

Description

[Brian Grinstead [:bgrins]]

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.

Comment 1

[Brian Grinstead [:bgrins]]

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();"/>

Comment 2

[Brian Grinstead [:bgrins]]

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?

Comment 3

[Neil Deakin]

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

Comment 4

[Brian Grinstead [:bgrins]]

(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?

Comment 5

[Brian Grinstead [:bgrins]]

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.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Do default actions still happen?  For example, does bug 70698 regress?

Comment 7

[Brian Grinstead [:bgrins]]

(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.

Comment 8

[Brian Grinstead [:bgrins]]

Attachment: drag-drop-scrollbar.gif

As mentioned in Comment 7

Comment 9

[Brian Grinstead [:bgrins]]

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

Comment 10

[Boris Zbarsky [:bzbarsky]]

> 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 11

[Brian Grinstead [:bgrins]]

Attachment: Bug 1431246 - Remove unused sborient attribute in scrollbar binding;

Review commit: https://reviewboard.mozilla.org/r/214158/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/214158/

Comment 12

[Brian Grinstead [:bgrins]]

(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.

Comment 13

[Brian Grinstead [:bgrins]]

Comment on attachment 8943784 [details]
Bug 1431246 - Remove unused sborient attribute in scrollbar binding;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/214158/diff/1-2/

Comment 14

[Brian Grinstead [:bgrins]]

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=.

Comment 15

[Tim Guan-tin Chien [:timdream] (please needinfo)]

I would like to give it a shot after bug 1450017.

Comment 16

[Brian Grinstead [:bgrins]]

(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.

Comment 17

[Brian Grinstead [:bgrins]]

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 18

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to (Unavailable until Apr 11) Brian Grinstead [:bgrins] from comment #16)
> but it doesn't support non-anonymous scrollbars.

What's the use case of non-anonymous scrollbars? Again, I don't see anything beyond test cases. I am tempted to land a small JS shim on these test files instead to unblock your patch.

Comment 19

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #18)
> What's the use case of non-anonymous scrollbars? Again, I don't see anything
> beyond test cases. I am tempted to land a small JS shim on these test files
> instead to unblock your patch.

Or we could manually construct the markup in these tests. I, however, don't know if that invalidates the tests...

Comment 20

[Tim Guan-tin Chien [:timdream] (please needinfo)]

My currently is that I can create the children by implementing nsScrollbarFrame::CreateAnonymousContent(). Still figuring it out...

Comment 21

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Bug 1431246 - Create and update scrollbar markup in nsScrollbarFrame

Review commit: https://reviewboard.mozilla.org/r/234960/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/234960/

Comment 22

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(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

[ExE Boss]

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.

Comment 24

[Botond Ballo [:botond]]

(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.

Comment 25

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(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.

Comment 26

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8966202 [details]
Bug 1431246 - Create and update scrollbar markup in nsScrollbarFrame

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234960/diff/1-2/

Comment 27

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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

Comment 28

[Brian Grinstead [:bgrins]]

(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.

Comment 29

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(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.

Comment 30

[Brian Grinstead [:bgrins]]

(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 31

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8966202 [details]
Bug 1431246 - Create and update scrollbar markup in nsScrollbarFrame

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234960/diff/2-3/

Comment 32

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(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...

Comment 33

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(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...

Comment 34

[Emilio Cobos Álvarez (:emilio)]

(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 35

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8966202 [details]
Bug 1431246 - Create and update scrollbar markup in nsScrollbarFrame

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234960/diff/3-4/

Comment 36

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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.

Comment 37

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 38

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Kartikaya Gupta (email:[e-mail]) 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...

Comment 39

[Brian Grinstead [:bgrins]]

(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.

Comment 40

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(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.

Comment 41

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #38)
> (In reply to Kartikaya Gupta (email:[e-mail]) 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 42

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8966202 [details]
Bug 1431246 - Create and update scrollbar markup in nsScrollbarFrame

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234960/diff/4-5/

Comment 43

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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 44

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8966202 [details]
Bug 1431246 - Create and update scrollbar markup in nsScrollbarFrame

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234960/diff/5-6/

Comment 45

[Tim Guan-tin Chien [:timdream] (please needinfo)]

... 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.

Comment 46

[Brian Grinstead [:bgrins]]

(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 :)

Comment 47

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Feel free to add additional reviewers. Thanks.

Comment 48

[ExE Boss]

As per comment 25 and comment 30, bug 77790/bug 1432935 will be worked on after this is resolved.

Comment 49

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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 />`?

Comment 50

[Neil Deakin]

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.

Comment 51

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8966202 [details]
Bug 1431246 - Create and update scrollbar markup in nsScrollbarFrame

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234960/diff/6-7/

Comment 52

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8966202 [details]
Bug 1431246 - Create and update scrollbar markup in nsScrollbarFrame

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234960/diff/7-8/

Comment 53

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8966202 [details]
Bug 1431246 - Create and update scrollbar markup in nsScrollbarFrame

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234960/diff/8-9/

Comment 54

[Neil Deakin]

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

Comment 55

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8966202 [details]
Bug 1431246 - Create and update scrollbar markup in nsScrollbarFrame

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/234960/diff/9-10/

Comment 56

[Eliza Balazs [:ebalazs_]]

Backed out changeset 2a760b1c0759 (bug 356831) for adding files without having a bugzilla product and component

https://hg.mozilla.org/integration/autoland/rev/9c2af62ba0cd59f8bd0b4cdbf037576db7a150bd

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=175362837&repo=autoland

Comment 57

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d3b5dfca6a90
Create and update scrollbar markup in nsScrollbarFrame r=enndeakin+6102

Comment 58

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/d3b5dfca6a90