When the tab strip is in overflow mode switching Tabs from and to the Addon Manager Tab repositions the Addon Manager Tab to the end (of visible tabs). Firefox UX-branch 22.0a1 (2013-03-21) + Fresh Profile Steps to reproduce 1. Open new tabs until overflow occurs 2. Open Addon Manager 3. Switch to another (visible) tab Expected: The selected tab get highlighted, tab strip position remains. Actual: The selected tab get highlighted, tab strip reposition the new selected tab as the last visible one.

If this is due to hiding the address bar for the Addon Manager, I'd rather see the address bar not being hidden (which fixes this issue).

Unfortunately the reposition of the tab strip is happening, if the windows lose the focus. Steps to reproduce 1. Open new tabs until overflow occurs 2. Select a visible tab (but not the last) 3. Click on the desktop (or another window) Expected: No tab strip reposition Actual: The tab strip repositions the selected tab as the last visible tab.

So this is not reproducible on m-c + all of Matt's tab patches.

Alright. I've made a new STR: Firefox UX 23.0a1 (2013-04-13) (fresh profile) 1. Open those websites in Tabs: - http://www.mozilla.org/en-US/ - https://wiki.mozilla.org/Main_Page - http://planet.mozilla.org/ - https://www.kernel.org/ - http://git-scm.com/ - http://python.org/ - https://www.djangoproject.com/ - http://www.apple.com/ - http://www.google.com/ - http://www.chromium.org/ - https://github.com/ - https://duckduckgo.com/ - (new Tab) This let the tabstrip to overflow (in my case). 2. Select 'Github.com' Tab 3. Click on Desktop (or another program), which let Firefox lose its focus. Notice: The tabstrip scrolls the 'github' tab as the last visible. 4. Try to click on 'python' Notice: 'python' is NOT selected, but one of the others! This is more annoying with far more tabs. I don't know if this is related to a Australis Tab patch or one of those depending patches. I see this behavior on OS X 10.8.3 and Windows 7 SP1 on 3 different hardwares (Mac + Windows). No one else?

(In reply to Morpheus3k from comment #4) > Alright. I've made a new STR: > > Firefox UX 23.0a1 (2013-04-13) > (fresh profile) > > 1. Open those websites in Tabs: > - http://www.mozilla.org/en-US/ > - https://wiki.mozilla.org/Main_Page > - http://planet.mozilla.org/ > - https://www.kernel.org/ > - http://git-scm.com/ > - http://python.org/ > - https://www.djangoproject.com/ > - http://www.apple.com/ > - http://www.google.com/ > - http://www.chromium.org/ > - https://github.com/ > - https://duckduckgo.com/ > - (new Tab) > > This let the tabstrip to overflow (in my case). > > 2. Select 'Github.com' Tab > 3. Click on Desktop (or another program), which let Firefox lose its focus. > > Notice: The tabstrip scrolls the 'github' tab as the last visible. > > 4. Try to click on 'python' > > Notice: 'python' is NOT selected, but one of the others! > > This is more annoying with far more tabs. I don't know if this is related to > a Australis Tab patch or one of those depending patches. > > I see this behavior on OS X 10.8.3 and Windows 7 SP1 on 3 different > hardwares (Mac + Windows). > > No one else? I'm sorry, I should have been more clear. I can't reproduce with *just* mozilla-central + the tab patches, which means this is probably related to customization work (the other patches on the UX branch). I'll try and figure out what's causing it right now. :-)

(In reply to :Gijs Kruitbosch from comment #5) > I'm sorry, I should have been more clear. I can't reproduce with *just* > mozilla-central + the tab patches, which means this is probably related to > customization work (the other patches on the UX branch). I'll try and figure > out what's causing it right now. :-) Uh, so this is weird, but I can't reproduce this anymore. I definitely could see this on older builds, with the old steps to reproduce as well, but the latest nightly (from the 16th, check with the "About UX" menuitem) seems to have fixed it (testing on OSX). I don't see any more issues with either set of STR. Marking as WFM.

I can confirm that this bug does not happen anymore, but there is a difference between the UX build "Firefox UX 23.0a1 (2013-04-13)" and the "23.0a1 (2013-04-16)": The patch "https://bugzilla.mozilla.org/attachment.cgi?id=722830" in Bug 625989 did not get landed again, which puts the tabs in the title bar. I assume that this patch causes the behavior.

(In reply to Morpheus3k from comment #7) > The patch "https://bugzilla.mozilla.org/attachment.cgi?id=722830" in Bug > 625989 did not get landed again, which puts the tabs in the title bar. > > I assume that this patch causes the behavior. Good point, you're probably right. As it says in that bug, that patch will not go to mozilla-central as-is so that's why it has been removed from UX. UX is now only for reviewed and finalized patches that are intended for mozilla-central. A similar patch will be finished and reviewed eventually.

This bug is back again.

(In reply to Morpheus3k from comment #9) > This bug is back again. Why do you assume it's related to bug 865374? Did you actually bisect, or?

I've noticed that every time a patch lands putting the tabs into the titlebar, I've seen this bug here. Either is the patch itself that put the tabs into the titlebar, or it is related to setting the "drawInTitlebar" to true. Don't you see this bug?

(In reply to Morpheus3k from comment #11) > I've noticed that every time a patch lands putting the tabs into the > titlebar, I've seen this bug here. Either is the patch itself that put the > tabs into the titlebar, or it is related to setting the "drawInTitlebar" to > true. > > Don't you see this bug? I couldn't check at the time and wanted to know how sure you were it's caused by tabs-in-titlebar.

Mike, can you take a look at this? I'm guessing that because this is also related to scrollbox stuff, the issue is going to be similar to bug 850924?

(In reply to :Gijs Kruitbosch from comment #13) > Mike, can you take a look at this? I'm guessing that because this is also > related to scrollbox stuff, the issue is going to be similar to bug 850924? And similar to that bug, I can't reproduce this anymore now that the app button is gone. Morpheus3k, can you try a mac build from here: http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/jamun-macosx64/1367863101/ or a windows build from here: http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/jamun-win32/1367863101/ and let me know if you can still reproduce? It's possible I'm missing something. :-)

Alright, I've tested your builds. The good news is, I can't reproduce it on Windows anymore. The bad news is that I still can reproduce it on OS X. I've tested it with "browser.tabs.drawInTitlebar" set to false: Problem fixed. So I assume it is either related to Bug 625989 or to Bug 865374. STR in Comment 4 are still valid. Remember to let Firefox Tab strip to overflow, otherwise this bug can't be seen.

(In reply to Morpheus3k from comment #15) > Alright, I've tested your builds. The good news is, I can't reproduce it on > Windows anymore. The bad news is that I still can reproduce it on OS X. > > I've tested it with "browser.tabs.drawInTitlebar" set to false: Problem > fixed. So I assume it is either related to Bug 625989 or to Bug 865374. > > STR in Comment 4 are still valid. Remember to let Firefox Tab strip to > overflow, otherwise this bug can't be seen. Yup, and the STR in comment #0 work too, on OS X. I'll poke at this some more to see if I can figure out why exactly this is broken. Thanks for your help, it's been very useful!

So the ordinal="..." magic that we use to reposition elements inside the tabstoolbar depending on the OS is making a mess of the scroll under/overflow code. The reason this is now working on Windows is that we no longer have a placeholder that we position *before* the tabs using an ordinal attribute. There used to be an app button placeholder, but that's now gone. The reason it's still broken on OS X is that our caption-buttons are on the left on OS X, and so those are repositioned using an ordinal attribute. This is why OS X is still broken. I am not sure exactly why these attributes create problems, to be honest, but there's a whole bunch of magical native gfx and XBL code involved, so I didn't manage to create a minimal testcase (in fact, I didn't even manage to get a scrolling <tabs/> element like the one we use, because we use tabbrowser-specific XBL for it). While the #ifdef'd ordinal attributes are neat, they break Real Things here. There's 3 workarounds that I could come up with: - Reorder the nodes from TabsInTitlebar's JS code - Duplicate the XUL within #ifdefs so they're inserted in the right place without ordinal attributes - Get rid of the elements and using padding on the <toolbar> element instead. I didn't think duplication (option 2) sounded very nice, so I made a patch for option 1 for the time being, which I've verified solves the actual problem at hand. I figured that there was a reason we were using placeholder elements rather than padding so I left that option. If nobody knows why we're using these rather than adjusting padding on the toolbar, then let me know and that might be a better solution... I'm a little on the fence about whether this is the right way forward (i.e. this smells like wallpapering and I'm not convinced that others wouldn't be able to figure out what's *actually* breaking here) and defer to wiser people on that decision. But having a patch at least gives us a way out of this problem should we not find a better way before landing this on m-c.

Comment on attachment 746345 [details] [diff] [review] Patch reordering placeholder elements using JS The ordinal attribute is needed to ensure that the placeholder is the first or last element, respectively, even if the toolbar is customized. You don't seem to be handling that case. The reason for not using padding is that the placeholders can be adjusted by themes more easily (e.g. http://hg.mozilla.org/mozilla-central/annotate/8922af45886e/browser/themes/windows/browser.css#l563) and are trivial to hide when needed (e.g. in fullscreen mode, http://hg.mozilla.org/mozilla-central/annotate/8922af45886e/browser/base/content/browser.css#l107).

(In reply to :Gijs Kruitbosch from comment #17) > So the ordinal="..." magic that we use to reposition elements inside the > tabstoolbar depending on the OS is making a mess of the scroll > under/overflow code. The reason this is now working on Windows is that we no > longer have a placeholder that we position *before* the tabs using an > ordinal attribute. There used to be an app button placeholder, but that's > now gone. It's still there on mozilla-central, which doesn't suffer from this bug. So there must be something specific to Australis causing this.

(In reply to Dão Gottwald [:dao] from comment #20) > (In reply to :Gijs Kruitbosch from comment #17) > > So the ordinal="..." magic that we use to reposition elements inside the > > tabstoolbar depending on the OS is making a mess of the scroll > > under/overflow code. The reason this is now working on Windows is that we no > > longer have a placeholder that we position *before* the tabs using an > > ordinal attribute. There used to be an app button placeholder, but that's > > now gone. > > It's still there on mozilla-central, which doesn't suffer from this bug. So > there must be something specific to Australis causing this. Do we draw tabs in the title bar on OS X on m-c?

(In reply to Dão Gottwald [:dao] from comment #19) > Comment on attachment 746345 [details] [diff] [review] > Patch reordering placeholder elements using JS > > The ordinal attribute is needed to ensure that the placeholder is the first > or last element, respectively, even if the toolbar is customized. You don't > seem to be handling that case. I don't think the tabs bar has a customization point in Australis, and if it does, its placement is up to us. So, AIUI, this shouldn't be a problem in Australis.

(In reply to :Gijs Kruitbosch from comment #21) > (In reply to Dão Gottwald [:dao] from comment #20) > > (In reply to :Gijs Kruitbosch from comment #17) > > > So the ordinal="..." magic that we use to reposition elements inside the > > > tabstoolbar depending on the OS is making a mess of the scroll > > > under/overflow code. The reason this is now working on Windows is that we no > > > longer have a placeholder that we position *before* the tabs using an > > > ordinal attribute. There used to be an app button placeholder, but that's > > > now gone. > > > > It's still there on mozilla-central, which doesn't suffer from this bug. So > > there must be something specific to Australis causing this. > > Do we draw tabs in the title bar on OS X on m-c? We do it on Windows. (In reply to :Gijs Kruitbosch from comment #22) > I don't think the tabs bar has a customization point in Australis, and if it > does, its placement is up to us. So, AIUI, this shouldn't be a problem in > Australis. As far as I know, the tab bar is going to get a customization point, but even if that avoids the problem, add-ons and other random code can still append non-customizable stuff to the tab bar and nav bar. We actually do something like this ourselves for fullscreen window controls: http://hg.mozilla.org/mozilla-central/annotate/8922af45886e/browser/base/content/browser-fullScreen.js#l567

(In reply to :Gijs Kruitbosch from comment #22) > (In reply to Dão Gottwald [:dao] from comment #19) > > Comment on attachment 746345 [details] [diff] [review] > > Patch reordering placeholder elements using JS > > > > The ordinal attribute is needed to ensure that the placeholder is the first > > or last element, respectively, even if the toolbar is customized. You don't > > seem to be handling that case. > > I don't think the tabs bar has a customization point in Australis, and if it > does, its placement is up to us. So, AIUI, this shouldn't be a problem in > Australis. The tab strip will be customizable, yes. Does it help any if you stop using the ordinal attribute, and use the -moz-box-ordinal-group CSS attribute instead?

(In reply to Mike Conley (:mconley) from comment #24) > Does it help any if you stop using the ordinal attribute, and use the > -moz-box-ordinal-group CSS attribute instead? No. :-(

In that case, perhaps we could listen for the event for customization being finished, and ensure that the placeholders are correctly positioned in the relevant toolbars?

So, on current Jamun tip on OS X, I'm also seeing that if the window unfocuses, the scroll position resets (to leftmost/0). I've tried setting breakpoints with the browser debugger in scrollbox's XBL code to see what's doing that, but none of them are getting hit. Frank, do you have any clue what could be triggering this and/or where I should be looking for a "proper" fix?

(In reply to :Gijs Kruitbosch from comment #27) That's very bizarre. This never happens on mozilla-central. Does Jamun change the scrollbox binding at all? Does it scroll smoothly to 0 or instantly reset to 0? If it's instant, my only guess is that the binding might be getting destroyed and recreated. If it's scrolling smoothly, my only guess is that ensureElementIsVisible is being called on the first tab, but that's even more unlikely.

(In reply to Frank Yan (:fryn) from comment #28) > (In reply to :Gijs Kruitbosch from comment #27) > > That's very bizarre. This never happens on mozilla-central. Does Jamun > change the scrollbox binding at all? Does it scroll smoothly to 0 or > instantly reset to 0? If it's instant, my only guess is that the binding > might be getting destroyed and recreated. If it's scrolling smoothly, my > only guess is that ensureElementIsVisible is being called on the first tab, > but that's even more unlikely. It's instant, but it seems to take a few fractions of a second to happen. Also, I don't know if I can trust animations on background windows to animate if they would. I've tried setting breakpoints in the destructors of the scrollbox and the tabbrowser tabs, neither were called (but they were when I cmd-q'd the process, so the breakpoints seem to have been working). AFAICT we haven't touched those bindings, save for refactoring the resize handler's TabsInTitlebar call, but that event isn't triggered when you blur the window. We *have* switched toolbar bindings, but I don't see how that'd affect anything, also given that this bug started (re-)occurring when the tabs in titlebar stuff landed, seemingly independent of the customization stuff. Which does make that one change in the resize handler suspect, but I just don't see how that could affect anything if there is no such event. I'll check some more.

(In reply to :Gijs Kruitbosch from comment #29) > It's instant, but it seems to take a few fractions of a second to happen. This was unclear, I realized as I submitted it. I mean that it resets to 0 in a "snap" fashion, but it doesn't happen *immediately* when you blur the window, but "a little bit" later. If I get really desperate I can start timing it to see if that gives us a clue... :-\

You should probably start by finding the regression range on Windows, where this works on mozilla-central but broke in Australis builds (until you removed the app menu button).

On Windows, this regressed all the way back in http://hg.mozilla.org/projects/jamun/rev/4d301b6c8d27 / http://hg.mozilla.org/projects/ux/rev/4d301b6c8d27 (Implement the Australis tab shape for Windows, bug 738491). This was the first real change after branching UX off m-c (apart from changing the branding to "UX" rather than Firefox).

(In reply to :Gijs Kruitbosch from comment #32) > On Windows, this regressed all the way back in > http://hg.mozilla.org/projects/jamun/rev/4d301b6c8d27 / > http://hg.mozilla.org/projects/ux/rev/4d301b6c8d27 (Implement the Australis > tab shape for Windows, bug 738491). This was the first real change after > branching UX off m-c (apart from changing the branding to "UX" rather than > Firefox). We have a winner on Windows, at least; removing this block makes the problem go away: http://hg.mozilla.org/projects/ux/file/00787d1b557a/browser/themes/windows/browser-aero.css#l305 (I still don't understand *why* we're breaking because of this, but hey...)

(In reply to :Gijs Kruitbosch from comment #33) > (In reply to :Gijs Kruitbosch from comment #32) > > On Windows, this regressed all the way back in > > http://hg.mozilla.org/projects/jamun/rev/4d301b6c8d27 / > > http://hg.mozilla.org/projects/ux/rev/4d301b6c8d27 (Implement the Australis > > tab shape for Windows, bug 738491). This was the first real change after > > branching UX off m-c (apart from changing the branding to "UX" rather than > > Firefox). > > We have a winner on Windows, at least; removing this block makes the problem > go away: > > http://hg.mozilla.org/projects/ux/file/00787d1b557a/browser/themes/windows/ > browser-aero.css#l305 > > (I still don't understand *why* we're breaking because of this, but hey...) On OS X, similarly, removing the image loading :before hack in tabs.inc.css, and the tabstrip's bottom border (drawn using :after) in browser.css, also makes this go away. We can probably move the preloading hack to some other element, but I'm not sure how to workaround the :after stuff (or, again, why this is having the bizarre effect that it has). :-\

Per discussion on IRC, Matt, can you have a look at what you think would be the right way forward here? Can we get someone who knows more about layout to look at this with the info we have right now, or do we need to have a more minimal testcase?

Found a few free hours tonight and thought I'd look at this for OSX. In the case of either block, what's happening (in case viewers at home aren't aware), is that the overflow event is being fired when the window is re-focused. Not 100% sure why yet, but looking...

More details: The overflow event is, I believe, being fired via nsGfxScrollFrameInner::FireScrollPortEvent. FireScrollPortEvent is being fired from nsGfxScrollFrameInner::AsyncScrollPortEvent as a runnable in nsRootPresContext::FlushWillPaintObservers. Not sure why that's being called just yet.

And last bit before I call it, nsGfxScrollFrameInner::AsyncScrollPortEvent is called from nsGfxScrollFrameInner::PostOverflowEvent, and nsGfxScrollFrameInner::PostOverflowEvent is called from nsXULScrollFrame::Layout So, the stack is: nsGfxScrollFrameInner::FireScrollPortEvent [fired via nsRootPresContext::FlushWillPaintObservers] nsGfxScrollFrameInner::AsyncScrollPortEvent nsGfxScrollFrameInner::PostOverflowEvent nsXULScrollFrame::Layout

Just a note that this might be fixed by bug 752434

(In reply to Mike Conley (:mconley) from comment #39) > Just a note that this might be fixed by bug 752434 I don't think it'd be fixed outright; we'd need to also move the shadow that we currently use for the tab bottom border to be a shadow on the nav-bar, as we did in one of the original patches for bug 851009, and move image preloading "somewhere else".

(In reply to :Gijs Kruitbosch from comment #35) > Per discussion on IRC, Matt, can you have a look at what you think would be > the right way forward here? Can we get someone who knows more about layout > to look at this with the info we have right now, or do we need to have a > more minimal testcase? Both would work but a minimal testcase (without XUL) would be preferred. I don't care about moving the preloading hack but moving the border is not ideal. I can also take a look at making a reduced testcase if you want.

Daniel, I asked bz for some help in finding someone to help us sort through this. It seems you might know how to fix this because of your experiences with flexbox stuff. I realize this is XUL boxes and not the new flexbox stuff, but we're still hopeful. ;-) A quick recap of the investigation/problem so far: Quickest way to reproduce: 1. Get an OS X UX build (https://hg.mozilla.org/projects/ux/ or http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013/05/2013-05-29-04-02-02-ux/ for nightlies) 2. Open enough tabs to get overflow/tabscroll. 3. Select a tab in the middle of the scrolled tabstrip 4. Focus another window The selected tab will snap as far to the right as possible given how many tabs are before it. This seems to be caused by 'something' triggering a reflow, which triggers an overflow event, which makes things scroll. See also comment 36, comment 37, comment 38. Things which seem to workaround this issue: - removing the <tabs>' element's siblings with XUL ordinal attributes (there to make space for the window's buttons so there's no overlap with those) OR - removing the :before, :after pseudoelements related to the tabstrip (drawing borders and preloading images. These workarounds are less than ideal, and ultimately we think this is a core issue... but we're not sure where it is and how hard it'd be to fix it. Do you have any ideas? :-)

I'll take a look. I can reproduce the bug as described in comment 42, in a mac ux nightly build. (though not in a linux ux nightly build) I'm making a mac debug ux build right now, to dig a bit deeper.

(In reply to Daniel Holbert [:dholbert] from comment #43) > I'll take a look. I can reproduce the bug as described in comment 42, in a > mac ux nightly build. (though not in a linux ux nightly build) > > I'm making a mac debug ux build right now, to dig a bit deeper. Yes, sorry, I should have been more specific about this: at this point this only occurs on OS X (to my knowledge, anyway) - we've gotten rid of the :before and :after elements on Windows/Linux (if they ever existed on Linux), so the conditions triggering the issue are no longer there.

(In reply to :Gijs Kruitbosch from comment #44) > Yes, sorry, I should have been more specific about this: at this point this > only occurs on OS X (to my knowledge, anyway) - we've gotten rid of the > :before and :after elements on Windows/Linux (if they ever existed on > Linux), so the conditions triggering the issue are no longer there. I don't recall getting rid of the pseudo elements on Linux and I still see them in DOMi on #TabsToolbar. I also cannot reproduce on Linux and don't know that I ever could. I used to be able to reproduce the problem on Windows but cannot anymore.

Sorry, I'm a bit useless on this at the moment, due to some old-mac impcompatibility hell. tl;dr, I need to upgrade to a new mac mini (or upgrade the OS on my old mac mini, which isn't really worth it) to get a version of GDB that works with our clang-generated binaries.* I'm talking with our desktop support team about that - I'll report back once I've got a functional environment/debuggable build. * due to a variant of http://code.google.com/p/chromium/issues/detail?id=138198

[OK, I've got a functional mac dev environment again now, sorry about that] So one thing I'm noticing, while trying to see why we're reflowing / what's being triggered by the window focus event: We're getting two calls to RecreateFramesForContent() for the toolbar, on each tab interaction (e.g. change which tab is focused, clicking arrow to pan to overflowed tabs). This also happens when the window is un-focused. So after each of those actions, we get a completely new frame tree inside of the toolbar. That seems sub-optimal, to say the least, and it might be related to what's going on here. Not sure why it's happening yet, though.

(In reply to Daniel Holbert [:dholbert] from comment #47) > [OK, I've got a functional mac dev environment again now, sorry about that] > > So one thing I'm noticing, while trying to see why we're reflowing / what's > being triggered by the window focus event: We're getting two calls to > RecreateFramesForContent() for the toolbar, on each tab interaction [...] This > also happens when the window is un-focused. Specifically, we hit this line: > 8157 RecreateFramesForContent(content, false); http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp?rev=0a801c3516c0&mark=8157-8157#8150 ...with 'content' being the content-node for a <toolbar> element that is the parent of a <tabs> element (from looking at the frame tree).

[Sorry, it's not quite as bad as I thought in comment 47 -- we only call RecreateFramesForContent for the toolbar when you de-focus the window (and trigger the bug) -- not on every tab interaction after all. (I had a typo in the condition for one of my breakpoints, with "=" instead of "==", which apparently was assigning a pointer out from under me.)] SO: we're getting a RecreateFramesForContent() on the <toolbar> parent of the tabstrip every time you de-focus the window. Why? Because we re-resolve style (due to a style change on the <window>), and that apparently triggers reframing for every descendant that has generated content, from this chunk: > 1413 // Check whether we might need to create a new ::before frame. [...] > 1431 if (!nsLayoutUtils::GetBeforeFrame(aFrame) && > 1432 nsLayoutUtils::HasPseudoStyle(localContent, newContext, > 1433 nsCSSPseudoElements::ePseudo_before, > 1434 aPresContext)) { > 1435 // Have to create the new :before frame > 1436 NS_UpdateHint(aMinChange, nsChangeHint_ReconstructFrame); > 1437 aChangeList->AppendChange(aFrame, content, > 1438 nsChangeHint_ReconstructFrame); > 1439 } http://mxr.mozilla.org/mozilla-central/source/layout/base/nsFrameManager.cpp?rev=b7330cb50416&mark=1431-1438#1413 ...and a similar chunk below for :after. So basically, it looks like if you have foo:after or foo:before, then every time foo's ancestor gets a style change (including window focus changes, for chrome UI), then foo & all of its children will get reframed. I don't immediately know why that is -- I need to talk to bz -- but assuming it's sane/necessary, then it's a strong reason to avoid :before and :after on things with unbounded numbers of descendants like the tab-strip.

Ah, I misread the conditions in the code-sample at Comment 49. So that code is saying: If we *don't have* a :before frame, and we need one, *then* reframe. That's less insane. So we're triggering that code right now, even though we already have a before frame, because nsLayoutUtils::GetBeforeFrame() isn't sensitive enough. It only checks to see if our first child is :before, but in this case, the :before frame is our *second* child.

So, this patch appears to fix this by making GetBeforeFrame() and GetAfterFrame() more sensitive (though not quite sensitive enough, per my comment in the patch). (Gijs, could you test this to sanity-check me on that?) I don't think we should actually land this, though, because it slows these functions down to O(N) instead of O(1) (where N = number of child frames), and in the usual case, we don't need to walk the frame list. I'm intending this more as an illustration than an actual fix. IMHO the best / most targeted fix here would be to either: (a) make sure the :before patch or :after patch are actually at the beginning/end of the frame list (e.g. by setting appropriate ordinal values on them) ...OR... (b) use "real" content instead of generated :before/:after content Either of those changes should make us avoid the ReconstructFrame chunk quoted in comment 49.

(In reply to Daniel Holbert [:dholbert] from comment #51) > IMHO the best / most targeted fix here would be to either: > (a) make sure the :before patch or :after patch are actually at the > beginning/end of the frame list (e.g. by setting appropriate ordinal values > on them) In particular, here's what the beginning of the toolbar's child list looks like, in the frame tree: Box(toolbar)(4)@0x11b890b80 next=[...] Box(hbox)(4)@0x11b891cf8 next=[...] Block(_moz_generated_content_before)(-1)@0x11f978ae0 next=[...] Note that "before" frame is the second child there, preceded by a hbox. Here's what the end of that child list looks like: Placeholder(_moz_generated_content_after)(-1)@0x11b31d168 next=[...] Box(hbox)(5)@0x11d3584c8 {19800,0,1200,1860} [...] Note that "after" is the second-to-last child there, followed by an hbox.

(In reply to Daniel Holbert [:dholbert] from comment #51) > Created attachment 756095 [details] [diff] [review] > strawman patch: Make GetBeforeFrame / GetAfterFrame walk the full child list > > So, this patch appears to fix this by making GetBeforeFrame() and > GetAfterFrame() more sensitive (though not quite sensitive enough, per my > comment in the patch). > > (Gijs, could you test this to sanity-check me on that?) Yes, this patch fixes it. I'll try and get a patch that just reorders the before and after elements in a second.

From my local testing, I can get the :before frame to the beginning by setting -moz-box-ordinal-group:0 on it, but I haven't been able to do the same with ::after yet (setting -moz-box-ordinal-group:5000 for now) -- it still ends up second-to-last. I'll try to dig in and figure out why; have to go do an interview now, though, so I'll be out for a bit.

Note: We could maaaybe fix this by adding a restricted special-case in GetBeforeFrame() / GetAfterFrame() along the lines of my strawman patch, but with a (small) cap on how far we'll look, and maybe also restricted based on frame-type. I can justify that to myself in the spirit of "Optimization to avoid reframing when we don't need to", as opposed to "hacky fix for this mysterious bug". (and then it would also end up incidentally fixing this bug)

(In reply to Daniel Holbert [:dholbert] from comment #54) > From my local testing, I can get the :before frame to the beginning by > setting -moz-box-ordinal-group:0 on it, but I haven't been able to do the > same with ::after yet (setting -moz-box-ordinal-group:5000 for now) -- it > still ends up second-to-last. OK, so that's happening due to bug 877890 which I've just filed. Basically, when the toolbar frame sorts its children, it has a nsPlaceholderFrame for the abspos-:after-frame, and the nsPlaceholderFrame has a different style context and is un-stylable. So it has -moz-box-ordinal-group = 1; meanwhile, the abspos frame has its huge -moz-box-ordinal-group value, but that does it no good because it's in a different child list. I've got a suggested fix for this over in bug 877890, but it's non-trivial, so we probably shouldn't rely on it to fix this bug. So, I lean towards either Comment 55 or just swapping out the generated content for "real" content.

So, this basically does what I suggested in comment 55, with the "cap on how far we'll look" being just 1 extra frame, which is sufficient in this case and probably sufficient in the majority of cases that'd accidentally hit this. bz, what are your thoughts on this patch, as an optimization to let us find an existing :before / :after frame and skip the ReconstructFrame in comment 49? (in cases where one other child has been reordered to the extreme end of the child list, past the :before or :after frame)

(In reply to Daniel Holbert [:dholbert] from comment #54) > From my local testing, I can get the :before frame to the beginning by > setting -moz-box-ordinal-group:0 on it This should be sufficient for us if we move the ::before stuff to some other element and the ::after stuff to ::before. The problem is that Gijs was not able to get -moz-box-ordinal-group:0 to work for him with ::before. Could you give more details on how you got that to work?

> This should be sufficient for us if we move the ::before stuff to some other element > and the ::after stuff to ::before. No -- it still won't work, as long as the the formerly-::after-now-::before thing is absolutely positioned, for the reasons I mentioned in comment 56. (Basically: -moz-box-ordinal-group will be effectively *ignored* on anything that's absolutely positioned, as described in bug 877890. So if your ::before is abspos, and you've got something else that has -moz-box-ordinal-group:0 (as we do here), then you're hosed.) > Could you give more details on how you got that to work? (I didn't do anything particularly fancy; I think I just set -moz-box-ordinal-group:0 on the existing ::before content, and then inspected the frame tree and confirmed that the ::before thing was now first. I suspect the abspos thing that I just brought up is what was giving Gijs trouble, though.)

...but as long as you're moving the ::before stuff to some other element, maybe you could move the ::after stuff to some other element, too? :) Regardless of whether the strawman lands (or whether bug 877890 gets fixed), the bottom line appears to be that ::before and ::after are fragile & have some of their assumptions (i.e. that they're first/last) violated, when they're used on a frame whose kids are reordered with -moz-box-ordinal-group. So it might be best to just avoid mixing those features altogether, rather than trying to shoehorn it into working.

Hmm. I guess we do allow generated content on XUL boxes, hrm. How does that play with things like AdjustAppendParentForAfterContent (in the frame constructor)? Basically, comment 60 is right: doing reordering on the kids of an element that has ::before or ::after will most likely lead to grief.

Comment on attachment 756244 [details] [diff] [review] less disgusting strawman patch: Make GetBeforeFrame / GetAfterFrame walk one extra child, if we're display:-moz-box or display:flex I can live with this as a temporary hack if we need to, but I'd really prefer us to come up with a better plan long-term...

(In reply to Daniel Holbert [:dholbert] from comment #56) > (In reply to Daniel Holbert [:dholbert] from comment #54) > > From my local testing, I can get the :before frame to the beginning by > > setting -moz-box-ordinal-group:0 on it, but I haven't been able to do the > > same with ::after yet (setting -moz-box-ordinal-group:5000 for now) -- it > > still ends up second-to-last. > > OK, so that's happening due to bug 877890 which I've just filed. Basically, > when the toolbar frame sorts its children, it has a nsPlaceholderFrame for > the abspos-:after-frame, and the nsPlaceholderFrame has a different style > context and is un-stylable. So it has -moz-box-ordinal-group = 1; > meanwhile, the abspos frame has its huge -moz-box-ordinal-group value, but > that does it no good because it's in a different child list. > > I've got a suggested fix for this over in bug 877890, but it's non-trivial, > so we probably shouldn't rely on it to fix this bug. Reading more comments there, it does seem to be easier than expected in this comment. Could we expect this to happen, set -moz-ordinal-group: 1001/5000/9001/whatever on our abs-pos ::after element, move the ::before element (which we're only using to preload images) to some other element, and be good? If so, I can do up a patch for that. :-)

I can confirm this change...

... together with this change, fixes it. I'm assuming dholbert/bz would be planning to make such a change (but better?) as part of bug 877890. :-) (I would have done a proper patch for bug 877890, but for the flexbox side this got a little messy, I wanted a helper function to keep the code clean, and failed to figure out how to make that work, due to my limited C++ skills).

(In reply to :Gijs Kruitbosch from comment #65) > Created attachment 756552 [details] [diff] [review] > Quick XUL ordering patch > > ... together with this change, fixes it. I'm assuming dholbert/bz would be > planning to make such a change (but better?) as part of bug 877890. :-) Yup, basically that, plus a nsFlexboxFrame chunk plus a testcase. So: As long as you use -moz-box-ordinal-group to enforce that any :before frames are at the beginning and any :after frames are at the end of a box with reordered children, then I'm reasonably satisfied that Comment 60 is addressed. (I had concerns that it might not reorder things correctly in one situation, but from local testing it seems to work better than I anticipated.) I'll post a patch over on bug 877890 today.

Comment on attachment 756550 [details] [diff] [review] Browser-side patch In which case, checking with Matt if this is OK! :-)

Comment on attachment 756550 [details] [diff] [review] Browser-side patch > #TabsToolbar::after { > content: ''; >+ -moz-box-ordinal-group: 9001; Where does this number come from? Can you document it?

I've added a comment. Any value over 1000 would do. I went over 9000 because... over 9000? Jokes are never funny when explained. :-)

Comment on attachment 756753 [details] [diff] [review] Patch with comment Review of attachment 756753 [details] [diff] [review]: ----------------------------------------------------------------- I don't exactly like moving the preloading further from the tabs but I don't know of a better place to put it. Now that we are back to using background-image for the tab images, we could combine the 6 tab images in a sprite sheet but I don't think there is a need for that change at the moment.

Pushed https://hg.mozilla.org/projects/ux/rev/75392ec4426d; note that after some discussion on IRC, this *doesn't* move the ::before anywhere, but just sets an ordinal of 0 on that, which should also work. Holding off on marking fixed in UX until bug 877890 gets fixed and merged to UX.

I just merged m-c to UX ( http://hg.mozilla.org/projects/ux/rev/2c6006ba259c ), and rebuilt, and we're done here, AFAICT. Thanks a lot for all your help, everyone! :-)