485841 - Get rid of some timeouts and avoid boxObject where appropriate in tabbrowser

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Dão Gottwald [:dao]]

Example A:
1. open lots of tabs
2. scroll to the end of the tabstrip
3. select the tab that's visible on the left end of the tabstrip, so that the last tab will be partially hidden
4. close the last tab using middle click
--or--
2. select the last tab
3. scroll a bit, so that the the tab that's visible on the left end of the tabstrip becomes fully visible and the last tab becomes partially hidden
4. close the last tab using the close button or accel+w
--> the closed tab leaves a gap that should have been filled

Example B:
1. enable session restoring ("When Minfield starts: Show my tabs and windows from last time"...)
2. open lots of tabs
3. select the last tab
4. exit the browser
5. start the browser
--> the selected tab isn't fully visible, it's cut off by the width of the right scrolling button

Comment 1

[Dão Gottwald [:dao]]

Attachment: patch

Comment 2

[Dão Gottwald [:dao]]

Morphing a bit, since the flushing that's interesting for the examples from comment 0 has been done in bug 483552. The patch here is still needed to fully fix the cases.

Comment 3

[Dão Gottwald [:dao]]

Attachment: patch

updated to trunk

Comment 4

[Dão Gottwald [:dao]]

Attachment: patch

updated to latest trunk

Comment 5

[Dão Gottwald [:dao]]

Gavin: ping?

Comment 6

[:Gavin Sharp [email: [e-mail]]]

There are a few places in that patch that use rect.left+rect.width (or clientWidth) instead of just using rect.right.

How did you determine that the setTimeout wasn't necessary for the adjustTabstrip/updateScrollButtonsDisabledState calls?

Looks good to me otherwise, but Neil Deakin may be a better reviewer for this since I think he's more familiar with the differences between client* properties and boxObjects.

Comment 7

[Dão Gottwald [:dao]]

(In reply to comment #6)
> There are a few places in that patch that use rect.left+rect.width (or
> clientWidth) instead of just using rect.right.

I see this only in one place, but I'll double-check.

> How did you determine that the setTimeout wasn't necessary for the
> adjustTabstrip/updateScrollButtonsDisabledState calls?

clientWidth in adjustTabstrip flushes layout, and so does _updateScrollButtonsDisabledState as of bug 483552.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #7)
> (In reply to comment #6)
> > There are a few places in that patch that use rect.left+rect.width (or
> > clientWidth) instead of just using rect.right.
> 
> I see this only in one place, but I'll double-check.

>+                                     ib.getBoundingClientRect().left + ib.clientWidth -

>+                  newMargin = tabRect.left - rect.left + tabRect.width;

>+                  newMargin = rect.left - tabRect.left + rect.width;

>+                  newMargin = rect.left - tabRect.left + rect.width - tabRect.width;

Comment 9

[Dão Gottwald [:dao]]

Attachment: patch v2

what Gavin said

Comment 10

[Neil Deakin]

Comment on attachment 387231 [details] [diff] [review]
patch v2

> Looks good to me otherwise, but Neil Deakin may be a better reviewer for this
> since I think he's more familiar with the differences between client*
> properties and boxObjects.

The difference is that getBoundingClientRect returns border-boxes, and boxObject.x/y does something odd where it excludes the border, (but boxObject.width/height does not).

Regardless, you may wish to test with larger borders on tabs, tabstrip, etc, just in case some theme has that.

Comment 11

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/3b6affa995be

Comment 12

[Dão Gottwald [:dao]]

leak test boxes are acting up, backed out...

Comment 13

[Dão Gottwald [:dao]]

Landed in various steps:

http://hg.mozilla.org/mozilla-central/rev/be001dd56987
http://hg.mozilla.org/mozilla-central/rev/c4fe058e760c
http://hg.mozilla.org/mozilla-central/rev/03a6732c66b3 [1]
http://hg.mozilla.org/mozilla-central/rev/af27eba56973 [2]
http://hg.mozilla.org/mozilla-central/rev/9e51ab5a80ee

1: this leaks
2: this fixed the leak, somehow

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified fixed with the steps in comment 0. Looks good on 1.9.2 with builds on all platforms like

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2b3pre) Gecko/20091112 Namoroka/3.6b3pre (.NET CLR 3.5.30729) ID:20091112051557