The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Firefox 24

Status

()

Firefox
Tabbed Browser
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: ttaubert, Assigned: ttaubert)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 24
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Assignee)

Description

4 years ago
Created attachment 754400 [details] [diff] [review]
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.
Attachment #754400 - Flags: review?(dao)
(Assignee)

Comment 1

4 years ago
Created attachment 757310 [details] [diff] [review]
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().
Attachment #754400 - Attachment is obsolete: true
Attachment #754400 - Flags: review?(dao)
Attachment #757310 - Flags: review?(dao)
(Assignee)

Comment 2

4 years ago
Created attachment 757313 [details] [diff] [review]
eliminate uninterruptible reflow caused by calling adjustTabstrip() in addTab(), v3

Small fix.
Attachment #757310 - Attachment is obsolete: true
Attachment #757310 - Flags: review?(dao)
Attachment #757313 - Flags: review?(dao)
(Assignee)

Comment 3

4 years ago
Created attachment 757378 [details] [diff] [review]
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.
Attachment #757313 - Attachment is obsolete: true
Attachment #757313 - Flags: review?(dao)
Attachment #757378 - Flags: review?(dao)

Updated

4 years ago
Summary: adjustTabstrip() call in addTab() causes unnecessary, uninterruptible reflow → adjustTabstrip() call in addTab() causes unnecessary uninterruptible reflow when opening a second tab
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.
Attachment #757378 - Flags: review?(dao) → review-
(Assignee)

Comment 5

4 years ago
(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.
(Assignee)

Comment 6

4 years ago
Created attachment 757432 [details] [diff] [review]
eliminate uninterruptible reflow caused by calling adjustTabstrip() in addTab(), v5

Logic moved to adjustTabstrip().
Attachment #757378 - Attachment is obsolete: true
Attachment #757432 - Flags: review?(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...
Attachment #757432 - Flags: review?(dao) → review-
(Assignee)

Comment 8

4 years ago
Created attachment 757457 [details] [diff] [review]
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.
Attachment #757432 - Attachment is obsolete: true
Attachment #757457 - Flags: review?(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.
(Assignee)

Comment 10

4 years ago
Created attachment 757470 [details] [diff] [review]
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.
Attachment #757457 - Attachment is obsolete: true
Attachment #757457 - Flags: review?(dao)
Attachment #757470 - Flags: review?(dao)

Updated

4 years ago
Blocks: 722680
Comment on attachment 757470 [details] [diff] [review]
eliminate uninterruptible reflow caused by calling adjustTabstrip() in addTab(), v7

thanks!
Attachment #757470 - Flags: review?(dao) → review+
(Assignee)

Comment 12

4 years ago
https://hg.mozilla.org/integration/fx-team/rev/52806760a6da
https://hg.mozilla.org/mozilla-central/rev/52806760a6da
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24
Depends on: 879375

Updated

4 years ago
Blocks: 783064
You need to log in before you can comment on or make changes to this bug.