Don't flush layout to determine whether scrollbox scroll buttons should be enabled/disabled

RESOLVED FIXED in Firefox 56

Status

()

Toolkit
XUL Widgets
P1
normal
RESOLVED FIXED
3 months ago
2 months ago

People

(Reporter: dao, Assigned: dao)

Tracking

(Depends on: 1 bug, Blocks: 3 bugs, {perf})

Trunk
mozilla56
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [ohnoreflow][qf:p1][photon-performance])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

3 months ago
+++ This bug was initially created as a clone of Bug #1358453 +++

Following up from bug 1358453...

It should be possible to use MozAfterPaint and getBoundsWithoutFlushing to reliably update the scroll buttons.
Flags: qe-verify-
(Assignee)

Updated

3 months ago
No longer blocks: 1356153
Comment hidden (mozreview-request)

Updated

3 months ago
Iteration: 55.6 - May 29 → ---
(Assignee)

Updated

3 months ago
Blocks: 1356153

Comment 2

3 months ago
mozreview-review
Comment on attachment 8872029 [details]
Bug 1368208 - Don't flush layout to determine whether scrollbox scroll buttons should be enabled/disabled

https://reviewboard.mozilla.org/r/143550/#review147512

::: toolkit/content/widgets/scrollbox.xml:188
(Diff revision 1)
>            innerRect.right = innerRect.left + this._scrollbox.scrollWidth;
>            innerRect.bottom = innerRect.top + this._scrollbox.scrollHeight;
>            return innerRect;
>          ]]></getter>
>        </property>
> -      <property name="scrollboxPaddingStart" readonly="true">
> +      <field name="scrollboxPaddingStart"><![CDATA[

If I remember correctly, field values are computed lazily the first time they are used; do we need to invalidate this when the theme changes?

::: toolkit/content/widgets/scrollbox.xml:616
(Diff revision 1)
>          ]]></body>
>        </method>
>  
> +      <field name="_scrollButtonUpdatePending">false</field>
>        <method name="_updateScrollButtonsDisabledState">
>          <parameter name="aScrolling"/>

nit: aScrolling is no longer used.

::: toolkit/content/widgets/scrollbox.xml:618
(Diff revision 1)
>  
> +      <field name="_scrollButtonUpdatePending">false</field>
>        <method name="_updateScrollButtonsDisabledState">
>          <parameter name="aScrolling"/>
>          <body><![CDATA[
> -          let scrolledToStart;
> +          if (this._scrollButtonUpdatePending) {

Just the use of the _scrollButtonUpdatePending boolean here to avoid updating the buttons several times in the same frame probably saves us a lot of time when closing several tabs at once (eg. bug 1356153).

::: toolkit/content/widgets/scrollbox.xml:635
(Diff revision 1)
> +            return;
> +          }
> +
> +          // Wait until after the next paint to get current layout data without
> +          // flushing layout. Use nested requestAnimationFrame rather than
> +          // MozAfterPaint to invalidate layout as late as possible.

Can you expand a bit on what you mean by "Use nested requestAnimationFrame rather than MozAfterPaint to invalidate layout as late as possible".

If I understood Mike's explanations correctly, we should change the dom in requestAnimationFrame but avoid doing things that flush there. And I guess the MozAfterPaint event would be a reasonable place to get computed layout values.

So based on this understanding I would expect something like:
function onMozAfterPaint() {
  /* do all the stuff that flushes */
  requestAnimationFrame(() => {
    this.setAttribute(...
  });
}

::: toolkit/content/widgets/scrollbox.xml:657
(Diff revision 1)
> -            scrolledToEnd = this._isRTLScrollbox;
> -            scrolledToStart = !this._isRTLScrollbox;
> -          } else if (this.scrollClientSize + this.scrollPosition == this.scrollSize) {
> -            // In the RTL case, this means the _first_ element in the
> -            // scrollbox is visible
> -            scrolledToStart = this._isRTLScrollbox;
> +                  scrollboxPaddingStart = -scrollboxPaddingStart;
> +                  scrollboxPaddingEnd = -scrollboxPaddingEnd;
> +                }
> +                let windowUtils = window.QueryInterface(Ci.nsIInterfaceRequestor)
> +                                        .getInterface(Ci.nsIDOMWindowUtils);
> +                let rect = ele => { return windowUtils.getBoundsWithoutFlushing(ele); };

nit: you can omit '{ return ' and '};' here.

::: toolkit/content/widgets/scrollbox.xml:659
(Diff revision 1)
> -          } else if (this.scrollClientSize + this.scrollPosition == this.scrollSize) {
> -            // In the RTL case, this means the _first_ element in the
> -            // scrollbox is visible
> -            scrolledToStart = this._isRTLScrollbox;
> -            scrolledToEnd = !this._isRTLScrollbox;
> +                }
> +                let windowUtils = window.QueryInterface(Ci.nsIInterfaceRequestor)
> +                                        .getInterface(Ci.nsIDOMWindowUtils);
> +                let rect = ele => { return windowUtils.getBoundsWithoutFlushing(ele); };
> +                let scrollboxRect = rect(this._scrollbox);
> +                let elements = this._getScrollableElements();

Seeing the getBoundsWithoutFlushing usage makes me assume you intend this function to not flush... but _getScrollableElements calls _canScrollToElement which is implemented as 'window.getComputedStyle(element).display != "none"'.
(In reply to Florian Quèze [:florian] [:flo] (away until Monday May 29th) from comment #2)

> ::: toolkit/content/widgets/scrollbox.xml:635
> (Diff revision 1)
> > +            return;
> > +          }
> > +
> > +          // Wait until after the next paint to get current layout data without
> > +          // flushing layout. Use nested requestAnimationFrame rather than
> > +          // MozAfterPaint to invalidate layout as late as possible.
> 
> Can you expand a bit on what you mean by "Use nested requestAnimationFrame
> rather than MozAfterPaint to invalidate layout as late as possible".
> 
> If I understood Mike's explanations correctly, we should change the dom in
> requestAnimationFrame but avoid doing things that flush there. And I guess
> the MozAfterPaint event would be a reasonable place to get computed layout
> values.
> 
> So based on this understanding I would expect something like:
> function onMozAfterPaint() {
>   /* do all the stuff that flushes */
>   requestAnimationFrame(() => {
>     this.setAttribute(...
>   });
> }

needinfo on Mike who may have interesting input here too.
Flags: needinfo?(mconley)
(Assignee)

Comment 4

3 months ago
(In reply to Florian Quèze [:florian] [:flo] (away until Monday May 29th) from comment #2)
> ::: toolkit/content/widgets/scrollbox.xml:188
> (Diff revision 1)
> >            innerRect.right = innerRect.left + this._scrollbox.scrollWidth;
> >            innerRect.bottom = innerRect.top + this._scrollbox.scrollHeight;
> >            return innerRect;
> >          ]]></getter>
> >        </property>
> > -      <property name="scrollboxPaddingStart" readonly="true">
> > +      <field name="scrollboxPaddingStart"><![CDATA[
> 
> If I remember correctly, field values are computed lazily the first time
> they are used; do we need to invalidate this when the theme changes?

This property is only relevant for the tab strip, and it shouldn't change when applying a lightweight theme.

> ::: toolkit/content/widgets/scrollbox.xml:635
> (Diff revision 1)
> > +            return;
> > +          }
> > +
> > +          // Wait until after the next paint to get current layout data without
> > +          // flushing layout. Use nested requestAnimationFrame rather than
> > +          // MozAfterPaint to invalidate layout as late as possible.
> 
> Can you expand a bit on what you mean by "Use nested requestAnimationFrame
> rather than MozAfterPaint to invalidate layout as late as possible".
> 
> If I understood Mike's explanations correctly, we should change the dom in
> requestAnimationFrame but avoid doing things that flush there. And I guess
> the MozAfterPaint event would be a reasonable place to get computed layout
> values.
> 
> So based on this understanding I would expect something like:
> function onMozAfterPaint() {
>   /* do all the stuff that flushes */
>   requestAnimationFrame(() => {
>     this.setAttribute(...
>   });
> }

Yes, MozAfterPaint -> requestAnimationFrame works too. I guess if _updateScrollButtonsDisabledState is called after the last rAF tick and before that frame is painted, this would make a difference, but I'm not sure this is possible in practice.

> ::: toolkit/content/widgets/scrollbox.xml:659
> (Diff revision 1)
> > -          } else if (this.scrollClientSize + this.scrollPosition == this.scrollSize) {
> > -            // In the RTL case, this means the _first_ element in the
> > -            // scrollbox is visible
> > -            scrolledToStart = this._isRTLScrollbox;
> > -            scrolledToEnd = !this._isRTLScrollbox;
> > +                }
> > +                let windowUtils = window.QueryInterface(Ci.nsIInterfaceRequestor)
> > +                                        .getInterface(Ci.nsIDOMWindowUtils);
> > +                let rect = ele => { return windowUtils.getBoundsWithoutFlushing(ele); };
> > +                let scrollboxRect = rect(this._scrollbox);
> > +                let elements = this._getScrollableElements();
> 
> Seeing the getBoundsWithoutFlushing usage makes me assume you intend this
> function to not flush... but _getScrollableElements calls
> _canScrollToElement which is implemented as
> 'window.getComputedStyle(element).display != "none"'.

Tabbrowser overrides this with a method that doesn't use getComputedStyle. The generic case that the scrollbox tries to handle is more tricky...
(Assignee)

Comment 5

3 months ago
(In reply to Dão Gottwald [::dao] from comment #4)
> Yes, MozAfterPaint -> requestAnimationFrame works too. I guess if
> _updateScrollButtonsDisabledState is called after the last rAF tick and
> before that frame is painted, this would make a difference, but I'm not sure
> this is possible in practice.

By make a difference I mean MozAfterPaint -> requestAnimationFrame would catch an earlier frame than requestAnimationFrame -> requestAnimationFrame. Usually they should end up with the same frame.
MozAfterPaint is a little tricky because of what it means - it means "layers in this window have been painted, and _received_ by the compositor thread". This is the closest, I believe, we can get to "has been presented to the user".

The problem with that, is that a MozAfterPaint event handler can be called for a paint that is not related to what you care about - it might fire, for example, for the _last_ paint and composite. I wrote about this in [1], and added some workarounds to nsIDOMWindowUtils to help with this. Also see the example code in [2].

So it's possible that MozAfterPaint is a fine place to put your computed style and layout values, but after talking to the folks in #content, what they suggest instead is to either:

a) Queue a runnable in requestAnimationFrame
b) Use a setTimeout in requestAnimationFrame

The runnable / setTimeout functions will be run "asap" after the paint has occurred, which is the best time to get the computed style and layout information.

[1]: https://groups.google.com/forum/#!searchin/mozilla.dev.platform/MozAfterPaint/mozilla.dev.platform/pCLwWdYc_GY/j9A-vWm3AgAJ
[2]: https://developer.mozilla.org/en-US/docs/Web/Events/MozAfterPaint

Updated

3 months ago
Flags: needinfo?(mconley)
Comment hidden (mozreview-request)

Comment 8

3 months ago
mozreview-review
Comment on attachment 8872029 [details]
Bug 1368208 - Don't flush layout to determine whether scrollbox scroll buttons should be enabled/disabled

https://reviewboard.mozilla.org/r/143550/#review147600

::: toolkit/content/widgets/scrollbox.xml:644
(Diff revision 2)
> +          // Wait until after the next paint to get current layout data without
> +          // flushing layout.
> +          window.addEventListener("MozAfterPaint", () => {

This relates to my previous comment, I guess - this MozAfterPaint event handler might fire for the _last_ paint that occurred.
(Assignee)

Comment 9

3 months ago
(In reply to Mike Conley (:mconley) from comment #8)
> Comment on attachment 8872029 [details]
> Bug 1368208 - Don't flush layout to determine whether scrollbox scroll
> buttons should be enabled/disabled.
> 
> https://reviewboard.mozilla.org/r/143550/#review147600
> 
> ::: toolkit/content/widgets/scrollbox.xml:644
> (Diff revision 2)
> > +          // Wait until after the next paint to get current layout data without
> > +          // flushing layout.
> > +          window.addEventListener("MozAfterPaint", () => {
> 
> This relates to my previous comment, I guess - this MozAfterPaint event
> handler might fire for the _last_ paint that occurred.

In other words, layout data may be stale here from the perspective of _updateScrollButtonsDisabledState's caller? So I guess I'll go back to nesting requestAnimationFrame.
Comment on attachment 8872029 [details]
Bug 1368208 - Don't flush layout to determine whether scrollbox scroll buttons should be enabled/disabled

Forwarding the review request to Mike who has more expertise here than I do.
Attachment #8872029 - Flags: review?(florian) → review?(mconley)
Comment hidden (mozreview-request)

Comment 12

3 months ago
mozreview-review
Comment on attachment 8872029 [details]
Bug 1368208 - Don't flush layout to determine whether scrollbox scroll buttons should be enabled/disabled

https://reviewboard.mozilla.org/r/143550/#review147612

Hey Dao,

This looks great! Thanks so much for working on this stuff. Just a few notes below - let me know if you have any questions.

::: toolkit/content/widgets/scrollbox.xml:644
(Diff revision 3)
> +          // Wait until after the next paint to get current layout data without
> +          // flushing layout.
> +          window.requestAnimationFrame(() => {
> +            window.requestAnimationFrame(() => {

So what we're doing here is moving this JS to run not at the end of the original tick (A), but at the end of the _next_ tick (B). This means that, yes, any DOM changes you make in (A) will not cause us to flush layout within (B), but any DOM changes that occurred between (A) or (B) might.

What I'm saying, is that we run the risk here of having too big a gap in time, where the DOM might be dirtied, and inside your inner-most rAF, we'll still cause a layout flush.

We can reduce this probability, by running inner-most JS earlier than (B). Yes, after the (A) tick, but as close to the _start_ of the (B) tick as possible.

So what I suggest instead is either:

```js
window.requestAnimationFrame(() => {
  setTimeout(() => {
    this._scrollButtonUpdatePending = false;
    // ...
  }, 0);
});
```

or

```js
window.requestAnimationFrame(() => {
  Services.tm.dispatchToMain(() => {
    this._scrollButtonUpdatePending = false;
    // ...
  });
});
```

if you want to avoid the timer overhead.

::: toolkit/content/widgets/scrollbox.xml:661
(Diff revision 3)
> -          } else if (aScrolling) {
> -            scrolledToStart = false;
> -            scrolledToEnd = false;
> -          } else if (this.scrollPosition == 0) {
> -            // In the RTL case, this means the _last_ element in the
> -            // scrollbox is visible
> +              } else {
> +                let scrollboxPaddingStart = Math.round(this.scrollboxPaddingStart);
> +                let scrollboxPaddingEnd = Math.round(this.scrollboxPaddingEnd);
> +                let [start, end] = this._startEndProps;
> +                if (this._isRTLScrollbox) {
> +                  [start, end] = [end, start];

Neat. :) Never thought of swapping that way.

::: toolkit/content/widgets/scrollbox.xml:682
(Diff revision 3)
> -          if (scrolledToEnd)
> +              if (scrolledToEnd) {
> -            this.setAttribute("scrolledtoend", "true");
> +                this.setAttribute("scrolledtoend", "true");
> -          else
> +              } else {
> -            this.removeAttribute("scrolledtoend");
> +                this.removeAttribute("scrolledtoend");
> +              }
>  
> -          if (scrolledToStart)
> +              if (scrolledToStart) {
> -            this.setAttribute("scrolledtostart", "true");
> +                this.setAttribute("scrolledtostart", "true");
> -          else
> +              } else {
> -            this.removeAttribute("scrolledtostart");
> +                this.removeAttribute("scrolledtostart");
> +              }

We might as well do this inside a requestAnimationFrame as well, if we're going to all of the trouble to avoid flushes - that'll make it so that other code that runs in this tick has less probability of causing a layout flush.
Attachment #8872029 - Flags: review?(mconley) → review-
(Assignee)

Comment 13

3 months ago
(In reply to Mike Conley (:mconley) from comment #12)
> What I'm saying, is that we run the risk here of having too big a gap in
> time, where the DOM might be dirtied, and inside your inner-most rAF, we'll
> still cause a layout flush.

Where are we flushing layout?
Flags: needinfo?(mconley)
(In reply to Dão Gottwald [::dao] from comment #13)
> Where are we flushing layout?

Bah, sorry, I meant flushing styles. We'll, I believe, be flushing styles inside your scrollboxPaddingStart and scrollboxPaddingEnd calls.
Flags: needinfo?(mconley)
(Assignee)

Comment 15

3 months ago
(In reply to Mike Conley (:mconley) from comment #14)
> (In reply to Dão Gottwald [::dao] from comment #13)
> > Where are we flushing layout?
> 
> Bah, sorry, I meant flushing styles. We'll, I believe, be flushing styles
> inside your scrollboxPaddingStart and scrollboxPaddingEnd calls.

XBL fields have lazy getters, they run once and then the value is kept.
(Assignee)

Comment 16

3 months ago
(We'll also get rid of scrollboxPaddingStart and scrollboxPaddingEnd in bug 1349555.)

Comment 17

3 months ago
mozreview-review-reply
Comment on attachment 8872029 [details]
Bug 1368208 - Don't flush layout to determine whether scrollbox scroll buttons should be enabled/disabled

https://reviewboard.mozilla.org/r/143550/#review147612

> We might as well do this inside a requestAnimationFrame as well, if we're going to all of the trouble to avoid flushes - that'll make it so that other code that runs in this tick has less probability of causing a layout flush.

Of course this is already inside a rAF. Ignore this.

Comment 18

3 months ago
mozreview-review
Comment on attachment 8872029 [details]
Bug 1368208 - Don't flush layout to determine whether scrollbox scroll buttons should be enabled/disabled

https://reviewboard.mozilla.org/r/143550/#review147652

::: toolkit/content/widgets/scrollbox.xml:644
(Diff revision 3)
> +          // Wait until after the next paint to get current layout data without
> +          // flushing layout.

Ah, okay! I understand! You're not somehow avoiding flushes with those rAF's - you're avoiding _stale information inside the getBoundsWithoutFlushing calls_. Okay, I understand now.

In that case, I think the above comment can probably be made more clear. Instead of saying "get current layout data without flushing layout", perhaps we can directly reference getBoundsWithoutFlushing, and say that we're waiting until after the next paint to avoid stale layout data.
Attachment #8872029 - Flags: review- → review+
Comment hidden (mozreview-request)

Comment 20

3 months ago
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c0c635e938fa
Don't flush layout to determine whether scrollbox scroll buttons should be enabled/disabled. r=mconley
Backed out in https://hg.mozilla.org/integration/autoland/rev/2d19d1d3f8aa for eslint (https://treeherder.mozilla.org/logviewer.html#?job_id=102879886&repo=autoland) and reftest (https://treeherder.mozilla.org/logviewer.html#?job_id=102882864&repo=autoland) failures.
(Assignee)

Comment 22

3 months ago
(In reply to Phil Ringnalda (:philor) from comment #21)
> Backed out in https://hg.mozilla.org/integration/autoland/rev/2d19d1d3f8aa
> for eslint
> (https://treeherder.mozilla.org/logviewer.
> html#?job_id=102879886&repo=autoland)

4618 TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/toolkit/content/widgets/scrollbox.xml:189:12 | Parsing error: Unexpected token paddingStart (eslint)

I don't understand what this is complaining about... I guess I'll try removing the paddingStart and paddingEnd variables in the XBL fields.

> and reftest
> (https://treeherder.mozilla.org/logviewer.
> html#?job_id=102882864&repo=autoland) failures.

Looks like this is running unprivileged and then we get this:

JavaScript error: chrome://global/content/bindings/scrollbox.xml, line 237: TypeError: window.QueryInterface(...).getInterface is not a function
Comment hidden (mozreview-request)
(Assignee)

Comment 24

3 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9111b1035ef366f401826573881bddc3898d1b0b

Comment 25

3 months ago
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8901a598369f
Don't flush layout to determine whether scrollbox scroll buttons should be enabled/disabled. r=mconley
Backed out for eslint failure at toolkit/content/widgets/scrollbox.xml:195 (unexpected token .):

https://hg.mozilla.org/integration/autoland/rev/10d90a86329f974c92b4e08c8b952c475fd61f3a

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=8901a598369fb85f4e81d2cc61837f95acbb6475&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=103012283&repo=autoland
> TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/toolkit/content/widgets/scrollbox.xml:195:25 | Parsing error: Unexpected token . (eslint)
Flags: needinfo?(dao+bmo)
(Assignee)

Comment 27

3 months ago
eslint, you're really annoying.
Flags: needinfo?(dao+bmo)
Comment hidden (mozreview-request)

Comment 29

3 months ago
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9fc7556f5386
Don't flush layout to determine whether scrollbox scroll buttons should be enabled/disabled. r=mconley
backed out for eslint failure like https://treeherder.mozilla.org/logviewer.html#?job_id=103030382&repo=autoland
Flags: needinfo?(dao+bmo)

Comment 31

3 months ago
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7d03173f5828
Backed out changeset 9fc7556f5386 for eslint failures
(Assignee)

Comment 32

3 months ago
(In reply to Carsten Book [:Tomcat] from comment #30)
> backed out for eslint failure like
> https://treeherder.mozilla.org/logviewer.html#?job_id=103030382&repo=autoland

TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/toolkit/content/widgets/scrollbox.xml:196:10 | Do not nest ternary expressions. (no-nested-ternary)

I'm nesting ternary expressions to work around the obscure failure from comment 22. FWIW, the code actually worked fine despite eslint failing to parse it. Same for comment 26. This is killing my productivity. Mark, please advise how I should proceed here.
Flags: needinfo?(dao+bmo) → needinfo?(standard8)
The fact this didn't show on try is due to bug 1368733. I'm looking at the actual issue in a bit.
(Assignee)

Comment 34

3 months ago
(In reply to Dão Gottwald [::dao] from comment #32)
> (In reply to Carsten Book [:Tomcat] from comment #30)
> > backed out for eslint failure like
> > https://treeherder.mozilla.org/logviewer.html#?job_id=103030382&repo=autoland
> 
> TEST-UNEXPECTED-ERROR |
> /home/worker/checkouts/gecko/toolkit/content/widgets/scrollbox.xml:196:10 |
> Do not nest ternary expressions. (no-nested-ternary)
> 
> I'm nesting ternary expressions to work around the obscure failure from
> comment 22.

Actually this isn't true, the nested ternary expression was in my earlier patch but masked by the parsing error. Ideally I'd like to keep this expression, as I think it's quite readable.
(Assignee)

Comment 35

3 months ago
I'm going to drop 'this.orient == "vertical" ? "paddingTop" :' from the expression and attempt to land this a fourth time. This means that scrollboxPaddingStart and scrollboxPaddingEnd will continue to be wrong for vertical scrollboxes, but I don't think we hit this case in practice.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 38

3 months ago
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9bda0f33f7b5
Don't flush layout to determine whether scrollbox scroll buttons should be enabled/disabled. r=mconley
Backed out for 508816-1-ref.xul failures. Scroll button highlighting differences from the looks of it.
https://hg.mozilla.org/integration/autoland/rev/02c2ade8e3ece2b1fd39df5bac82e78bb62cb3d1

https://treeherder.mozilla.org/logviewer.html#?job_id=103146668&repo=autoland
The parser that was written for xbl provides a modified version to ESLint. For field elements, it pretends it is a getter and wraps the code segment in a return(), e.g.:

      get scrollboxPaddingStart() { // eslint-disable-line
        return ( // eslint-disable-line
          parseFloat(window.getComputedStyle(this._scrollbox)[
            this.orient == "vertical" ? "paddingTop" :
              (this._isRTLScrollbox ? "paddingRight" : "paddingLeft")
          ])
        ); // eslint-disable-line
      }, // eslint-disable-line

The cases in comment 21 and comment 26 are therefore found to be errors due to the return wrapping of what is seen as multiple statements.

I took a look at https://developer.mozilla.org/docs/Mozilla/Tech/XBL/XBL_1.0_Reference/Elements#field and https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Tutorial/Adding_Properties_to_XBL-defined_Elements#Fields

From my understanding of those, it seems like the field's initializer can only have one line of code that is expected to return the value. So I think these are possibly invalid xml, although it is a bit strange that the tests seemed to pass. I'm guessing we don't have other instances of field elements with multiple statements in the tree, as we'd be seeing failures for those as well.
Flags: needinfo?(standard8)
Could the semi-colon at the end of the field value be part of the problem?
(In reply to Florian Quèze [:florian] [:flo] from comment #41)
> Could the semi-colon at the end of the field value be part of the problem?

The preprocessor automatically strips that.
(Assignee)

Comment 43

3 months ago
(In reply to Mark Banner (:standard8) from comment #40)
> From my understanding of those, it seems like the field's initializer can
> only have one line of code that is expected to return the value.

No, this is not correct.

> So I think
> these are possibly invalid xml, although it is a bit strange that the tests
> seemed to pass.

I tested this manually too.
(Assignee)

Comment 44

3 months ago
(In reply to Ryan VanderMeulen [:RyanVM] from comment #39)
> Backed out for 508816-1-ref.xul failures. Scroll button highlighting
> differences from the looks of it.
> https://hg.mozilla.org/integration/autoland/rev/
> 02c2ade8e3ece2b1fd39df5bac82e78bb62cb3d1
> 
> https://treeherder.mozilla.org/logviewer.html#?job_id=103146668&repo=autoland

There appears to be a race condition exposed by that reftest. In _updateScrollButtonsDisabledState, the early this.hasAttribute("notoverflowing") check + requestAnimationFrame + _scrollButtonUpdatePending makes us miss the initial overflow event from 508816-1-ref.xul's arrowscrollbox that would remove the notoverflowing attribute.
Comment hidden (mozreview-request)

Comment 46

3 months ago
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/17792fad126f
Don't flush layout to determine whether scrollbox scroll buttons should be enabled/disabled. r=mconley
had to back this out again for the reftest failures like https://treeherder.mozilla.org/logviewer.html#?job_id=103350489&repo=autoland in the same test as mentioned in #comment 44
Flags: needinfo?(dao+bmo)

Comment 48

3 months ago
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d840ee711bc8
Backed out changeset 17792fad126f for reftestfailures in 508816-1.xul
(Assignee)

Comment 49

3 months ago
(In reply to Carsten Book [:Tomcat] from comment #47)
> had to back this out again for the reftest failures like
> https://treeherder.mozilla.org/logviewer.
> html#?job_id=103350489&repo=autoland in the same test as mentioned in
> #comment 44

Green on Linux and Mac, orange on Windows. Sigh.
Flags: needinfo?(dao+bmo)
(Assignee)

Comment 50

3 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf5bac26f334934694264258ca39d99da696f26f
(Assignee)

Comment 51

3 months ago
I don't understand what's going on with 508816-1.xul on Windows, but queuing at least three requestAnimationFrame calls in both 508816-1.xul and 508816-1-ref.xul before doing the comparison seems to fix it... none of this seems to be needed on Linux and Mac.
Comment hidden (mozreview-request)

Comment 53

3 months ago
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4b82f58570e4
Don't flush layout to determine whether scrollbox scroll buttons should be enabled/disabled. r=mconley
Still failing on QR builds. Can we please get a fully green Try push across all platforms before this lands and burns the tree again? |try: -b do -p all -u reftest[-10.6,-Windows XP] -t none| should get you what you want.
https://hg.mozilla.org/integration/autoland/rev/cd807be171322fd50de8709de13f240121259933

https://treeherder.mozilla.org/logviewer.html#?job_id=103529725&repo=autoland
https://treeherder.mozilla.org/logviewer.html#?job_id=103531016&repo=autoland

Updated

3 months ago
Whiteboard: [ohnoreflow][qf][photon-performance] → [ohnoreflow][qf:p1][photon-performance]
(Assignee)

Updated

2 months ago
Depends on: 1371604
(Assignee)

Updated

2 months ago
Depends on: 1371620
Comment hidden (mozreview-request)
(Assignee)

Comment 56

2 months ago
I think I'll land this with 508816-1.xul disabled. The scroll buttons seem to suffer from some weird invalidation bug that I can reproduce even in nightly (e.g. in the all tabs list or in the bookmarks menu). My patch doesn't seem to make this worse but changes the timing such the reftest now exposes this bug.
Comment hidden (mozreview-request)
(Assignee)

Comment 58

2 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cd6e30f644d673e800920c1c59be30928c749270
(Assignee)

Updated

2 months ago
Depends on: 1375012
Comment hidden (mozreview-request)

Comment 60

2 months ago
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ea7d29871244
Don't flush layout to determine whether scrollbox scroll buttons should be enabled/disabled r=mconley

Comment 61

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ea7d29871244
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56

Updated

2 months ago
Iteration: --- → 56.1 - Jun 26
Depends on: 1376397
You need to log in before you can comment on or make changes to this bug.