876374 - adjustTabstrip() call in addTab() causes unnecessary uninterruptible reflow when opening a second tab

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

Attachment: eliminate uninterruptible reflow caused by calling adjustTabstrip() in addTab()

Bug 722681 added the tabContainer.adjustTabstrip() call in addTab() when starting the tabopen animation. While it helps with perceived performance this degrades actual performance because the getBoundingClientRect() call in adjustTabstrip() causes an uninterruptible reflow.

Most of the logic in adjustTabstrip() is not needed in order to achieve the same thing as bug 722681 intended. I moved this into a separate adjustTabstripForTabOpen() method that doesn't contain a getBoundingClientRect() call as it's *highly* unlikely that the tab width is lower than tabClipWidth with only two tabs. In the edge case of it we'll just 'closebuttons' to 'activetab' after the animation has ended. I think we can really omit the edge case here in favor of speeding up every addTab() call.

The second thing this patch does is add a adjustTabstripForTabClose() method because I figured we also want to immediately hide the first tab's close button when closing one of two tabs. We could just call adjustTabstrip() here but most of the logic isn't needed for this effect and we'd somehow need to pass a parameter that tells it to ignore the closing tab. I figured having this as a separate method as well is cleaner.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: eliminate uninterruptible reflow caused by calling adjustTabstrip() in addTab(), v2

Added test that ensures that there are no unexpected uninterruptible reflows when opening a new tab. The test fails without the change for tabbrowser.xml applied as it detects the reflow caused by adjustTabStrip() in addTab().

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Attachment: eliminate uninterruptible reflow caused by calling adjustTabstrip() in addTab(), v3

Small fix.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Attachment: eliminate uninterruptible reflow caused by calling adjustTabstrip() in addTab(), v4

Turns out that there is a couple of .focus() calls that cause reflows as well when opening new tabs. We need to add them to the expected list for now and take care of them in later bugs.

Comment 4

[Dão Gottwald [:dao]]

Comment on attachment 757378 [details] [diff] [review]
eliminate uninterruptible reflow caused by calling adjustTabstrip() in addTab(), v4

It looks like it would be simpler and more consistent to incorporate this logic into adjustTabstrip, i.e. let it never take mTabClipWidth into account when there are only two tabs. You might also be able to handle the "for tab close" case by looking at _removingTabs.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Dão Gottwald [:dao] from comment #4)
> It looks like it would be simpler and more consistent to incorporate this
> logic into adjustTabstrip, i.e. let it never take mTabClipWidth into account
> when there are only two tabs. You might also be able to handle the "for tab
> close" case by looking at _removingTabs.

Yeah, I thought about this as well while writing the patch. Thanks for the feedback, I'll look into doing it as you described.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Attachment: eliminate uninterruptible reflow caused by calling adjustTabstrip() in addTab(), v5

Logic moved to adjustTabstrip().

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 757432 [details] [diff] [review]
eliminate uninterruptible reflow caused by calling adjustTabstrip() in addTab(), v5

>+      <property name="_numValidTabs" readonly="true">
>+        <getter><![CDATA[
>+          return this.tabs.length - this._removingTabs.length;
>+        ]]></getter>
>+      </property>

"valid" is pretty ambiguous. Please either avoid adding this property or give it a more descriptive name that makes it clearer that this includes hidden tabs and excludes closing tabs.

>+            // The second tab just got closed and we will end up with a single
>+            // one. Remove the first tab's close button immediately (if needed)
>+            // rather than after the tabclose animation ends.
>+            if (this._numValidTabs == 1)
>+              this.tabContainer.adjustTabstrip();

nit: as written ("The second tab just got closed..."), the comment should be inside the "only one tab left" block

>             } else {
>               let tab = this.tabbrowser.visibleTabs[this.tabbrowser._numPinnedTabs];
>-              if (tab && tab.getBoundingClientRect().width > this.mTabClipWidth)
>+              if (numTabs == 2 || (tab && tab.getBoundingClientRect().width > this.mTabClipWidth))
>                 this.setAttribute("closebuttons", "alltabs");
>               else
>                 this.setAttribute("closebuttons", "activetab");
>             }

how about:

} else if (numTabs == 2) {
  this.setAttribute("closebuttons", "alltabs");
} else {
  let tab = ...

You should also document that this check is an optimization, what it's good for and that it's ok to ignore mTabClipWidth in that case.

>+const EXPECTED_REFLOWS = [

Hmm, seems like it will be slightly annoying to keep this updated, since there will probably be more cases where reflows are unavoidable...

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

Attachment: eliminate uninterruptible reflow caused by calling adjustTabstrip() in addTab(), v6

(In reply to Dão Gottwald [:dao] from comment #7)
> "valid" is pretty ambiguous. Please either avoid adding this property or
> give it a more descriptive name that makes it clearer that this includes
> hidden tabs and excludes closing tabs.

Yeah, I wasn't able to find a good name before so I just reverted those changes.

> how about:
> 
> } else if (numTabs == 2) {
>   this.setAttribute("closebuttons", "alltabs");
> } else {
>   let tab = ...
> 
> You should also document that this check is an optimization, what it's good
> for and that it's ok to ignore mTabClipWidth in that case.

Done.

> >+const EXPECTED_REFLOWS = [
> 
> Hmm, seems like it will be slightly annoying to keep this updated, since
> there will probably be more cases where reflows are unavoidable...

Right, I'll work on getting rid of some more reflows but some of them might probably still be unavoidable. OTOH, adding new stuff that unknowingly causes layout flushes is just too easy so I think having a test like this to avoid regression is really beneficial.

We could reduce the test to only check the addTab() call without waiting for the transition to end. That way we'd cover the important stuff, i.e. adding a new tab. After the tabopen animation is done we need to flush layout anyway and there should be no blocking reflows in between.

Comment 9

[Dão Gottwald [:dao]]

Comment on attachment 757457 [details] [diff] [review]
eliminate uninterruptible reflow caused by calling adjustTabstrip() in addTab(), v6

>       <method name="adjustTabstrip">
>         <body><![CDATA[
>+          let numRemovingTabs = this.tabbrowser._removingTabs.length;
>+          let numTabs = this.childNodes.length - numRemovingTabs;

numRemovingTabs is used just once. Looks like you could merge this into one line and possibly wrap it if it's too long.

>+              // This is an optimization to avoid layout flushes by calling
>+              // getBoundingClientRect() when we just opened a second tab. In
>+              // this case it's highly unlikely that the tab width is equal to
>+              // mTabClipWidth

"that the tab width is smaller than mTabClipWidth"

> (i.e. the tabstrip is unlikely to overflow with
>+              // only two tabs). In the edge case of the window being so narrow
>+              // that there is an overflow, we'll correct the 'closebuttons'
>+              // attribute after the tabopen animation has finished.

mTabClipWidth is independent from tab overflow.

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

Attachment: eliminate uninterruptible reflow caused by calling adjustTabstrip() in addTab(), v7

(In reply to Dão Gottwald [:dao] from comment #9)
> numRemovingTabs is used just once. Looks like you could merge this into one
> line and possibly wrap it if it's too long.

Done.

> > (i.e. the tabstrip is unlikely to overflow with
> >+              // only two tabs). In the edge case of the window being so narrow
> >+              // that there is an overflow, we'll correct the 'closebuttons'
> >+              // attribute after the tabopen animation has finished.
> 
> mTabClipWidth is independent from tab overflow.

Sorry, I didn't know that. Fixed.

Comment 11

[Dão Gottwald [:dao]]

Comment on attachment 757470 [details] [diff] [review]
eliminate uninterruptible reflow caused by calling adjustTabstrip() in addTab(), v7

thanks!

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/52806760a6da

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/52806760a6da