additional scenario where tab scrolling does not handle window resize well asaf points out the following scenario: 1) Have seven tabs open 2) resize the window and scroll the tabbar (to the right) so you only see the last three tabs. 3) enlarge the window so it /can/ show at least one more tab. asaf thinks we should always show as many tabs as possible after the window is resized. note, the original issue in bug #343019 is very similar to this one. my change in that bug was for handling resize and underflow, but it's not a complete fix. I think if I scroll and then ensure the selected item is visible, things will work as desired. the change (in bug #343019) only takes of the scenario where the selected tab is not visible

scroll before ensuring the selected item is visible. for ltr, we want to scroll one index "to the right" before ensuring the selected item is visible. for rtl, we want to scroll one index "to the left" ensuring the selected item is visible. once bug #343104 is fixed, this should work for rtl as well as ltr.

Comment on attachment 227853 [details] [diff] [review] patch r=mano

I just remembered something (which works this way at least on Windows). The window "resize" event is also fired when the chrome layout is chnaged, e.g. when dropping buttons from the customize-toolbar window on the toolbars, or when opening the sidebar. Both calls (ensureElementIsVisible and scrollByIndex) will scroll the tabbar in these cases. A workaround might be to cache the tabbar width whenever resize is fired and only scroll if the the width has changed. This will also avoid tabbar-scrolling when the height of the window is chnaged but not its width.

asaf, good point about the issue with scrolling or ensuring on resize. additionally, I'm not sure if this change is causing some unexpected behavior. I'll investigate and report back.

I should note that the ensureElementIsVisible in the resize handler probably already causes us the same unexpected behavior, sorry for not catching this in my review for 343019. I guess it's OK to fix these issues separately, but we're already working on the same code in too many bugs ;)

Comment on attachment 227853 [details] [diff] [review] patch I'm definitely seeing the onResize() get called when I'm not expecting it, so the scrollByIndex() and ensureVisible() happen when I don't want them too, which leads to unexpected behavior. I'll attach a JS stack to where onResize() is getting called unexpectedly (at least, unexpectedly to me!)

asaf, I see what you mean about the ensureIsVisible in the onResize handler. your comment #3 (and your suggested approach, about keeping track of the width) makes sense. I'll work on implementing that.

I think we want this bug fixed on both trunk and branch.

Comment on attachment 227927 [details] [diff] [review] scroll and ensure only when the width changes >Index: tabbrowser.xml >=================================================================== > var self = this; > function onResize() { > self.adjustTabstrip(false); Not that it really matters, but this should be under the if block as well. >- self.mTabstrip.scrollBoxObject >+ var width = self.mTabstrip.boxObject.width; >+ if (width != self.mTabstripWidth) { >+ self.mTabstrip.scrollByIndex(1); >+ self.mTabstrip.scrollBoxObject > .ensureElementIsVisible(self.selectedItem); nit: two more spaces. r=mano otherwise.

> Not that it really matters, but this should be under the if block as well. good point. I've moved it. I've noticed onResize() gets called frequently, so moving this to under the if block is a good idea. > nit: two more spaces. fixed. I've checked in to the trunk. I'll seek branch approval. thanks again, asaf.

Comment on attachment 227935 [details] [diff] [review] patch that I checked in carrying over asaf's review. seeking a= for the branch.

Fixed on 1.8 branch for beta 1 (branch patch is on bug 318168).