1411372 - Order of buttons on menu toolbar changes after restart / new window

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[08xjcec48]

Attachment: order of buttons.png

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20171023180840

Steps to reproduce:

Updated Firefox 57 beta.


Actual results:

Every time Firefox 57 is updated, the order of the extension buttons I have on the same row as the tab bar (placed next to the window control buttons) is reversed.

The extensions are Containers on the Go (https://addons.mozilla.org/addon/containers-on-the-go/) and Undo Closed Tabs Button (https://addons.mozilla.org/addon/undo-closed-tabs-revived/).

When I enter the "Customize..." mode, the buttons automatically go back to the order I arranged them.

This has happened after all beta version updated I've installed (57.0b09 -> 57.0b10 -> 57.0b11).

Comment 1

[Sam Foster [:sfoster] (he/him)]

We're worried this might be a migration bug. QA: Can you attempt reproduce this, using a profile created in 56 and updating to any of the 57 betas with the extensions mention.

Comment 2

[Sam Foster [:sfoster] (he/him)]

Flagging as p4 until we know more. We should update priority if this points at a serious underlying migration issue.

Comment 3

[u554753]

The icons did not change order when migrating from 56.0 to 57.0b11.

Comment 4

[Sam Foster [:sfoster] (he/him)]

(In reply to Grover Wimberly IV [:Grover-QA] from comment #3)
> The icons did not change order when migrating from 56.0 to 57.0b11.

Hi Grover, could you try one more thing? Take the profile from 56 with the extensions installed, and upgrade (open in) 57.0b09, to see if that reproduces the issue. 

The only other possibility I can think of here is if this profile is being used simultaneously in 56 and 57, which could potentially lead to undefined behavior.

Comment 5

[08xjcec48]

Updating to 57.0b12 did it again. I didn't install Firefox 57 over 56. I actually uninstalled it beforehand and started a new profile.

Comment 6

[u554753]

Updating from 56.0 to 57.0b09, I was able to reproduce the issue. The icons are switched.

User Agent:
Mozilla/5.0 (Windows NT 10.0; WOW64; rv:57.0) Gecko/20100101 Firefox/57.0

Comment 7

[Sam Foster [:sfoster] (he/him)]

Thanks for confirming, Grover. 

Looks like next steps here are for someone to take on some investigation to determine the extent of this bug, and if there are other user-facing migration impacts.

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

So this is:
- "broken" for 56->57b9
- "fixed" for 56-57b11
- "broken" for 56-57b12

It sounds like it is a timing issue or something else that would add some randomness to when it occurs, and less likely the result of changes between 57b9->57b11 and 57b11->57b12.

Comment 9

[:Gijs (he/him)]

Bob, are you or others on the webextensions team aware of issues like this?

Grover, can you do another test with b9 (or b12 if you can repro with b12) and evaluate the following in the browser console before and after the update, to help debug this:

CustomizableUI.getWidgetIdsInArea("TabsToolbar")

And can you check if the same problem happens if you have these buttons in the menu bar, the bookmarks toolbar or the navbar?

Thanks!

Comment 10

[Bob Silverberg [:bsilverberg]]

Thanks for the heads up, Gijs. I wasn't aware of this, and I don't see why it would necessarily have showed up on anyone else's radar as it's not in the WebExtensions component. Did you want us to investigate, or are you just checking to see if it's a known issue?

Comment 11

[:Gijs (he/him)]

(In reply to Bob Silverberg [:bsilverberg] from comment #10)
> Thanks for the heads up, Gijs. I wasn't aware of this, and I don't see why
> it would necessarily have showed up on anyone else's radar as it's not in
> the WebExtensions component. Did you want us to investigate, or are you just
> checking to see if it's a known issue?

I'm not sure if it's specific to webextensions at this point, or if it's an issue with customizable UI. As I just got back from 3 weeks of PTO, I have a... significant... backlog. If you have cycles to investigate "soon", please do. If not, I should be able to look at this in the next week or two.

Comment 12

[u554753]

56.0
Array [ "tabbrowser-tabs", "new-tab-button", "alltabs-button", "_7e56c1ad-71c3-47fe-bdba-372c7770e0…", "_5997e7bd-1940-4058-a5f4-1562afce63…" ]


57.0b9 
Array [ "tabbrowser-tabs", "new-tab-button", "alltabs-button", "_7e56c1ad-71c3-47fe-bdba-372c7770e0…", "_5997e7bd-1940-4058-a5f4-1562afce63…" ]

Was not changed this time, so this doesn't seem to be 100% reproducible.

57.0b11
Array [ "tabbrowser-tabs", "new-tab-button", "alltabs-button", "_7e56c1ad-71c3-47fe-bdba-372c7770e0…", "_5997e7bd-1940-4058-a5f4-1562afce63…" ]

The icons swapped at this point.

57.0b12
Array [ "tabbrowser-tabs", "new-tab-button", "alltabs-button", "_7e56c1ad-71c3-47fe-bdba-372c7770e0…", "_5997e7bd-1940-4058-a5f4-1562afce63…" ]

Remains switched.

Comment 13

[:Gijs (he/him)]

What about the same issue in other toolbars? Is this limited to the tabstrip? Because that would seem... strange...

Comment 14

[Bob Silverberg [:bsilverberg]]

I asked around and it doesn't sound like this is a known issue for us. Removing my needinfo for now as I don't think this is waiting on anything else from me.

Comment 15

[u554753]

(In reply to :Gijs (slow, PTO recovery mode) from comment #13)
> What about the same issue in other toolbars? Is this limited to the
> tabstrip? Because that would seem... strange...

That doesn't seem to be the case for me. Reporter, are you getting switched buttons in other parts?

Comment 16

[08xjcec48]

Only the tabstrip.

Comment 17

[08xjcec48]

This hasn't happened to me anymore (currently on Firefox 58 downloaded from the FTP server).

You can close this issue if you can't reproduce it either.

Comment 18

[:Gijs (he/him)]

Tentatively resolving based on comment #17... we can reopen if this reoccurs, though it'd be helpful if we could gather more details on it if it does happen again.

Comment 19

[08xjcec48]

After telling you to close this, I've replaced Containers on the Go with Temporary Containers, and kept the Undo Close Tab Button.

https://addons.mozilla.org/addon/undo-close-tab-button/
https://addons.mozilla.org/addon/temporary-containers/

Now, their order on the tabstrip sometimes get reversed randomly when I restart Firefox, even when there has been no update. Opening the "Customize" page still fixes it.

Is there anything I can do to track down what is happening?

Comment 20

[:Gijs (he/him)]

(In reply to Hide my Email from comment #19)
> After telling you to close this, I've replaced Containers on the Go with
> Temporary Containers, and kept the Undo Close Tab Button.
> 
> https://addons.mozilla.org/addon/undo-close-tab-button/
> https://addons.mozilla.org/addon/temporary-containers/
> 
> Now, their order on the tabstrip sometimes get reversed randomly when I
> restart Firefox, even when there has been no update. Opening the "Customize"
> page still fixes it.
> 
> Is there anything I can do to track down what is happening?

I'm really not sure. You can try flipping browser.uiCustomization.debug in about:config to 'true' and copying the log of what happens when you restart Firefox (from the browser console, https://developer.mozilla.org/en-US/docs/Tools/Browser_Console ) and of what happens when you open customize (ie clear it inbetween and separate the two logs, please). Perhaps that will shed some light on this...

Comment 22

[:Gijs (he/him)]

(In reply to :Gijs (he/him) from comment #20)
> (In reply to Hide my Email from comment #19)
> > After telling you to close this, I've replaced Containers on the Go with
> > Temporary Containers, and kept the Undo Close Tab Button.
> > 
> > https://addons.mozilla.org/addon/undo-close-tab-button/
> > https://addons.mozilla.org/addon/temporary-containers/
> > 
> > Now, their order on the tabstrip sometimes get reversed randomly when I
> > restart Firefox, even when there has been no update. Opening the "Customize"
> > page still fixes it.
> > 
> > Is there anything I can do to track down what is happening?
> 
> I'm really not sure. You can try flipping browser.uiCustomization.debug in
> about:config to 'true' and copying the log of what happens when you restart
> Firefox (from the browser console,
> https://developer.mozilla.org/en-US/docs/Tools/Browser_Console ) and of what
> happens when you open customize (ie clear it inbetween and separate the two
> logs, please). Perhaps that will shed some light on this...

Seems the reporter of this bug isn't around - Fanolian, can you obtain a log like this? Per this bug, other people haven't been able to reliably reproduce...

Comment 23

[Fanolian]

Attachment: uiCustomization logs

> Seems the reporter of this bug isn't around - Fanolian, can you obtain a log
> like this? Per this bug, other people haven't been able to reliably
> reproduce...

There are 3 files in the log zip. The icons I used are WebExtension Firefox Multi-Account Containers, and built-in Add-ons button, just like bug 1470756.

1_before_restart.txt:
You did not ask for it but I included it just in case. It was logged by clicking customize after putting icons in place but before restarting, i.e. no reordering.

2_after_restart.txt:
It was logged right after restarting and not clicking anything.

3_restart_customize.txt:
It was logged after clicking customize after a restart. Icons were snapped back to correct orders.

Comment 24

[Fanolian]

> 2_after_restart.txt:
> It was logged right after restarting and not clicking anything.

The order of icons were changed at that moment.

Comment 25

[:Gijs (he/him)]

I'm flummoxed. We call `insertBefore` on the toolbar, and the first argument is the new button, and the second argument is the "normal" button (built-in addons button in the example) and the node appears in the correct space in the DOM (its nextSibling/previousSibling are correct) but visually, it's clearly after the addons button instead of before. While a bunch of nodes (accessibility indicator, caption button placeholders, private browsing placeholder) have an `ordinal` attribute, the buttons don't. I suspect the frame tree is not arranged correctly for these nodes somehow... that or I'm missing some CSS that I'm (a) unaware of and (b) can't find in the inspector...

emilio, can you help?

STR (seems to reproduce for me, at least):

0. clean profile
1. install https://addons.mozilla.org/firefox/addon/multi-account-containers/
2. in the browser console, execute:

CustomizableUI.addWidgetToArea("_testpilot-containers-browser-action", "TabsToolbar");
CustomizableUI.addWidgetToArea("add-ons-button", "TabsToolbar");

3. open about:addons (accel-shift-a or click the button you just added to the toolbar)
4. switch to the "extensions" tab and disable and re-enable the container add-on.

This seems to have to do with the order in which elements are inserted into the DOM, because if you then open a new window with the add-on enabled (where we'll insert the nodes in a different order because they'll all be available), the issue does not appear and the buttons are ordered correctly. So I suspect a bug somewhere between DOM and Layout...

Comment 26

[Emilio Cobos Álvarez (:emilio)]

XBL is known to not handle dynamic nested insertion points. Does the patch in bug 1425362 help?

Comment 27

[:Gijs (he/him)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #26)
> XBL is known to not handle dynamic nested insertion points. Does the patch
> in bug 1425362 help?

Wouldn't any mis-insertion show up in the resulting DOM tree rather than the visual result, though? :-)

I'll try the patch on Monday, assuming it still applies - keeping ni for that...

Comment 28

[Emilio Cobos Álvarez (:emilio)]

(In reply to :Gijs (he/him) from comment #27)
> (In reply to Emilio Cobos Álvarez [:emilio] from comment #26)
> > XBL is known to not handle dynamic nested insertion points. Does the patch
> > in bug 1425362 help?
> 
> Wouldn't any mis-insertion show up in the resulting DOM tree rather than the
> visual result, though? :-)

Nope, XBL shuffles the layout tree via the assigned nodes of the XBL children element, the assigned nodes' DOM looks the same:

  https://searchfox.org/mozilla-central/rev/4074ba403219b7accdf00220b20dc492bfd4d83e/dom/xbl/XBLChildrenElement.h#148

Comment 29

[:Gijs (he/him)]

(In reply to :Gijs (he/him) from comment #27)
> (In reply to Emilio Cobos Álvarez [:emilio] from comment #26)
> > XBL is known to not handle dynamic nested insertion points. Does the patch
> > in bug 1425362 help?
> 
> Wouldn't any mis-insertion show up in the resulting DOM tree rather than the
> visual result, though? :-)
> 
> I'll try the patch on Monday, assuming it still applies - keeping ni for
> that...

OK, make it Tuesday - but unfortunately it didn't help. Any other suggestions, short of removing the XBL binding for <toolbar>? :-(

Comment 30

[:Gijs (he/him)]

(In reply to :Gijs (he/him) from comment #29)
> (In reply to :Gijs (he/him) from comment #27)
> > (In reply to Emilio Cobos Álvarez [:emilio] from comment #26)
> > > XBL is known to not handle dynamic nested insertion points. Does the patch
> > > in bug 1425362 help?
> > 
> > Wouldn't any mis-insertion show up in the resulting DOM tree rather than the
> > visual result, though? :-)
> > 
> > I'll try the patch on Monday, assuming it still applies - keeping ni for
> > that...
> 
> OK, make it Tuesday - but unfortunately it didn't help. Any other
> suggestions, short of removing the XBL binding for <toolbar>? :-(

Also, the binding (for <toolbar> into which we're inserting) doesn't really have <content> to speak of, I think - are you sure this is a XBL problem?

Comment 31

[Emilio Cobos Álvarez (:emilio)]

(In reply to :Gijs (he/him) from comment #30)
> (In reply to :Gijs (he/him) from comment #29)
> > (In reply to :Gijs (he/him) from comment #27)
> > > (In reply to Emilio Cobos Álvarez [:emilio] from comment #26)
> > > > XBL is known to not handle dynamic nested insertion points. Does the patch
> > > > in bug 1425362 help?
> > > 
> > > Wouldn't any mis-insertion show up in the resulting DOM tree rather than the
> > > visual result, though? :-)
> > > 
> > > I'll try the patch on Monday, assuming it still applies - keeping ni for
> > > that...
> > 
> > OK, make it Tuesday - but unfortunately it didn't help. Any other
> > suggestions, short of removing the XBL binding for <toolbar>? :-(
> 
> Also, the binding (for <toolbar> into which we're inserting) doesn't really
> have <content> to speak of, I think - are you sure this is a XBL problem?

Hmm... I suspect it is, but it's easy to double check. Is the issue fixed if you do something like:

 container.style.display = "none"; container.offsetTop; container.style.display = "";

Comment 32

[:Gijs (he/him)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #31)
> (In reply to :Gijs (he/him) from comment #30)
> > (In reply to :Gijs (he/him) from comment #29)
> > > (In reply to :Gijs (he/him) from comment #27)
> > > > (In reply to Emilio Cobos Álvarez [:emilio] from comment #26)
> > > > > XBL is known to not handle dynamic nested insertion points. Does the patch
> > > > > in bug 1425362 help?
> > > > 
> > > > Wouldn't any mis-insertion show up in the resulting DOM tree rather than the
> > > > visual result, though? :-)
> > > > 
> > > > I'll try the patch on Monday, assuming it still applies - keeping ni for
> > > > that...
> > > 
> > > OK, make it Tuesday - but unfortunately it didn't help. Any other
> > > suggestions, short of removing the XBL binding for <toolbar>? :-(
> > 
> > Also, the binding (for <toolbar> into which we're inserting) doesn't really
> > have <content> to speak of, I think - are you sure this is a XBL problem?
> 
> Hmm... I suspect it is, but it's easy to double check. Is the issue fixed if
> you do something like:
> 
>  container.style.display = "none"; container.offsetTop;
> container.style.display = "";

"No" if I run those 3 statements in 1 go, "yes" if I set style.display = "none" in one command from the browser console, and run the rest as a second command.

If I run:

container = gBrowser.tabContainer.parentNode; container.style.display = "none"; container.clientHeight; container.style.display = "";

in one go (ie using clientHeight instead of offsetTop to flush layout), that fixes the issue.

I... don't understand how that tells us anything. I would expect both XBL bindings and the frame tree to get nuked from the container being display: none, so confusion in either one would be "fixed" like this. I presume I'm still missing something? :-)

Comment 33

[Emilio Cobos Álvarez (:emilio)]

The frame tree does get nuked, but not the XBL binding. The XBL binding only gets nuked from RemoveFromBindingManagerRunnable[1], at the point the element is actually unbound from the DOM.

I would've expected this to hit[2] and don't load the XBL binding again. If that path is hit that means that this is probably a frame constructor bug. If it's not indeed it doesn't tell us anything.

I'll take a closer look when I have the time.

[1]: https://searchfox.org/mozilla-central/rev/6ef785903fee6c0b16a1eab79d722373d940fd78/dom/base/Element.h#2005
[2]: https://searchfox.org/mozilla-central/rev/6ef785903fee6c0b16a1eab79d722373d940fd78/dom/xbl/nsXBLService.cpp#499

Comment 34

[Emilio Cobos Álvarez (:emilio)]

So comment 25 doesn't repro for me on Linux. I'm supposed to see the two toolbar buttons swapped, right?

Which OS can you repro this in? Is there any OS-dependent rule related to those?

Comment 35

[:Gijs (he/him)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #34)
> So comment 25 doesn't repro for me on Linux. I'm supposed to see the two
> toolbar buttons swapped, right?
> 
> Which OS can you repro this in? Is there any OS-dependent rule related to
> those?

Windows. Not sure about rule specifics, but I suspect the relevant thing might be whether you have tabs-in-titlebar turned on. I *think* that's not yet the default on Linux? I'll test on macOS/Linux later.

Comment 36

[:Gijs (he/him)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #34)
> So comment 25 doesn't repro for me on Linux. I'm supposed to see the two
> toolbar buttons swapped, right?

Yeah, when you disable the add-on it disappears, and when you re-enable the add-on it appears in the other spot.

> Which OS can you repro this in? Is there any OS-dependent rule related to
> those?

I can also reproduce on macOS (as well as Windows 10 as noted before), fwiw. For some complicated reasons, I can't easily check on Linux at the moment, sorry. But I expect the tabs-in-titlebar stuff is relevant, in case that helps (we do support that on Linux these days, but I think it might still be opt-in, not sure).

Comment 37

[Fanolian]

(In reply to :Gijs (he/him) from comment #35) 
> Windows. Not sure about rule specifics, but I suspect the relevant thing
> might be whether you have tabs-in-titlebar turned on. I *think* that's not
> yet the default on Linux? I'll test on macOS/Linux later.

The issue does go away when I enable title bar in Customize in Nightly on Windows 10. Of course it returns when I disable it again.

Comment 38

[:Gijs (he/him)]

We've removed XBL bindings for the toolbar, and some of the tabsintitlebar stuff has changed. I wonder if this is fixed on current Nightly. If not, we can at least exclude this having to do with XBL and our tabsintitlebar stuff itself, and should probably retry figuring out how the ordinal stuff is breaking things here, also because we're going to be using it a bit more still with the fix in bug 1510631. Needinfo'ing myself so I take a poke at this, though Emilio, if you see this and you have time to look at this again that'd be much appreciated. (STR in comment #25, but might be windows/mac-specific, or need tabs-in-titlebar turned on on Linux (which needs CSD support on the window manager))

Comment 39

[:Gijs (he/him)]

Can still reproduce this on current nightly (tested on mac this time).

Comment 41

[:Gijs (he/him)]

As should be obvious from the dupe, this also affects the menubar (on Windows) when it is in the titlebar (ie in the default configuration on windows, when you haven't turned on the separate titlebar), and doesn't require add-ons. Because the DOM tree is definitely correct, this is definitely a platform/layout issue. I expect it has to do with the use of 'ordinal' in the styling for the (transparent) placeholder item nodes that are at the end of the toolbar to 'leave room' for the caption buttons, as well as the private browsing window + accessibility indicators. Either way, it shouldn't cause other nodes to be laid out in a different order.

Comment 42

[Emilio Cobos Álvarez (:emilio)]

Does using the -moz-box-ordinal-group instead of the ordinal attribute work?

If you could somehow reduce it I'd be happy to investigate it.

Comment 43

[:Gijs (he/him)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #42)
> Does using the -moz-box-ordinal-group instead of the ordinal attribute work?

No.

> If you could somehow reduce it I'd be happy to investigate it.

I have no idea how to reduce it. IIRC I tried to do this months ago in jsbin or similar with a "regular" web doc, and couldn't find a way to get the same problem to show up. :-(

Do you have suggestions? To be clear, it's in the main browser.xul window so I don't think it's hard to reproduce as it is... What would reducing it achieve?

Comment 44

[Fanolian]

I have not been able to reproduce this bug (putting icons on tab bar) for a while, but bug 1511191 (icons on menu bar) is still reproducible.
According to Mozregression, this is fixed by bug 1513200.

Note:
Steps from bug 1511191 (using all built-in icons; no WebExtensions icons required) also applies here, except putting icons on tab bar instead.

Comment 45

[:Gijs (he/him)]

For convenience, the steps from bug 1511191 comment 0 are:

> 1. setting to show menubar
> 2. put items on menubar, left to right, menu item/download icon/Bookmark
> toolbar items(NOT Bookmarks menu)
> 3. close Firefox and restart

I guess we may be able to work around the layout issue here by using a customization target in the menubar as well, then. Dão/Mike, does that sound feasible, and would it allow getting rid of the ordinal grouping stuff here?

Comment 46

[08xjcec48]

(In reply to Fanolian from comment #44)
> I have not been able to reproduce this bug (putting icons on tab bar) for a
> while, but bug 1511191 (icons on menu bar) is still reproducible.
> According to Mozregression, this is fixed by bug 1513200.
> 
> Note:
> Steps from bug 1511191 (using all built-in icons; no WebExtensions icons
> required) also applies here, except putting icons on tab bar instead.



This has still been happening to me every time I restart Firefox 64.0 (using Temporary Containers and Undo Close Tab icons on the tab bar).

Comment 47

[:Gijs (he/him)]

(In reply to Hide my Email from comment #46)
> (In reply to Fanolian from comment #44)
> > I have not been able to reproduce this bug (putting icons on tab bar) for a
> > while, but bug 1511191 (icons on menu bar) is still reproducible.
> > According to Mozregression, this is fixed by bug 1513200.
> > 
> > Note:
> > Steps from bug 1511191 (using all built-in icons; no WebExtensions icons
> > required) also applies here, except putting icons on tab bar instead.
> 
> 
> 
> This has still been happening to me every time I restart Firefox 64.0 (using
> Temporary Containers and Undo Close Tab icons on the tab bar).

Yes, because bug 1513200 has only been fixed in Firefox 66 (current Nightly).

Comment 48

[Dão Gottwald [:dao]]

(In reply to :Gijs (he/him) from comment #45)
> For convenience, the steps from bug 1511191 comment 0 are:
> 
> > 1. setting to show menubar
> > 2. put items on menubar, left to right, menu item/download icon/Bookmark
> > toolbar items(NOT Bookmarks menu)
> > 3. close Firefox and restart
> 
> I guess we may be able to work around the layout issue here by using a
> customization target in the menubar as well, then. Dão/Mike, does that sound
> feasible,

Yeah.

> and would it allow getting rid of the ordinal grouping stuff here?

Some of it, but it probably doesn't even matter once these elements are in different containers.

Comment 49

[Mike Conley (:mconley) (:⚙️)]

(In reply to Dão Gottwald [::dao] from comment #48)

I guess we may be able to work around the layout issue here by using a
customization target in the menubar as well, then. Dão/Mike, does that sound
feasible,

Yeah.

WFM!

Comment 52

[:Gijs (he/him)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #34)

So comment 25 doesn't repro for me on Linux. I'm supposed to see the two
toolbar buttons swapped, right?

Which OS can you repro this in? Is there any OS-dependent rule related to
those?

The steps in bug 1671820 are on Linux. Does this help with reproducing?

(Also, a lot of the discussion in this bug is about XBL, and XBL is now gone, but the problem persists, which is a little surprising...)

Comment 53

[Emilio Cobos Álvarez (:emilio)]

Ok, so I could repro bug 1671820 here, and yes, that was very helpful, I understand what's going on now. Some bits of our discussion:

emilio
gijs: ok, I can repro if I leave just the downloads button
gijs: ok so I see why this happens
gijs: indeed .titlebar-buttonbox-container { -moz-box-ordinal-group: 1000; } is part of the issue
gijs: so the issue is that -moz-box-ordinal-group works by reordering the frames (unlike the order property)
gijs: so when you insert the downloads button (or other items) dynamically, the previous sibling in this case is the titlebar-buttonbox-container which is at the end of the frame tree
gijs: so we find the titlebar-buttonbox-container as the reference frame to insert after (since the frame tree mimics DOM order), and then reorder (so we have <flexible> <searchbar> <titlebar-buttonbox-container> <downloads> and reorder it so that the ordinal groups are ordered which ends up being <flexible> <searchbar> <downloads> <titlebar-buttonbox-container>
gijs: so the order ends up being wrong
gijs: (or not match the DOM order at least)
gijs
emilio: OK. Would switching to order help? Is that feasible without wholesale-switching to new-flexbox for everything in there?
emilio
gijs: so fixing it is slightly annoying... either rewrite xul box layout to use CSSOrderAwareIterator rather than the bespoke reordering (fwiw, the emulate-box-with-flex work dholbert was doing should fix it)
gijs: or somehow teach the frame constructor about ordinal-group, which I'm not particularly excited about...
gijs: switching the toolbar to flex / order would work too of course
gijs
but we'd need to switch the entire toolbar, right?
that's gonna be a right old can o worms
emilio
gijs: (well, work == paper over it), and yes, likely
gijs
hm, so that leaves adding the container as discussed in the bug
or trying to fix the insertion logic to pick another insertion point
ie insert before whatever comes after the buttons atm
emilio
gijs: I can look at whether there's a better fix in layout land, maybe switching to CSSOrderAwareIterator is not that terrible...
gijs
as that won't matter for DOM order and sounds like it'll fix the issue, maybe?
emilio
gijs: Yeah, that should work too
gijs
emilio: rewriting any XUL layout thing sounds like a can of worms tho
although this setup is "amazing" it's a bit of an edgecase and I dunno what else might end up breaking, and I'm sure (read in sarcastic voice) we have excellent (not) unit-test coverage of all the XUL layout stuff :|
emilio
gijs: we'll see... but yes, a bit, see also bug 1665476 where I tried to remove some XUL code and ended up crashing all WR reftests
gijs
:'(
emilio
gijs: but anyhow I'll paste the diagnostic into the bug, and I'll poke a bit later. There might be an even easier fix
gijs: maybe just fixing the XUL reordering to account for dom order too would work

Comment 54

[Emilio Cobos Álvarez (:emilio)]

So potential fixes:

• Don't use -moz-box-ordinal-group on the front-end to reorder the window close buttons, either:
  • Reorder the window buttons on the DOM.
  • Switch the toolbar to use flex / order.
• Fix the sorting in XUL layout to either:
  • Account for DOM order.
  • Don't reorder frames (use CSSOrderAwareIterator or such).

I believe the "Account for DOM order" might be the least tricky but we'll see...

Comment 55

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1411372 - Remove unused XULrelayoutChildAtOrdinal. r=#layout-reviewers

Comment 56

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1411372 - Use enum classes in CSSOrderAwareFrameIterator. r=#layout-reviewers

Depends on D94788

Comment 57

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1411372 - Remove bogus XUL box sorting. r=#layout-reviewers

Instead, sort stuff using CSSOrderAwareFrameIterator. The current
sorting is broken in presence of dynamic insertions, consider the
following <Child(order)> combination in the DOM:

<A(1000)> <B(0)>

That'd look like:

<B(0)> <A(1000)>

On the frame tree. However when appending a child before B so that the
DOM looks like:

<A(1000)> <C(0)> <B(0)>

The frame constructor will properly insert after A, and the reordering,
which is stable, will end up with:

<B(0)> <C(0)> <A(1000)>

Which is the wrong frame tree order.

We only use -moz-box-tree-ordinal in regular sprocket layout, so just
handle it there rather than everywhere.

Depends on D94789

Comment 59

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/424377419d0d
Remove unused XULrelayoutChildAtOrdinal. r=TYLin
https://hg.mozilla.org/integration/autoland/rev/44db783208d7
Use enum classes in CSSOrderAwareFrameIterator. r=TYLin

Comment 60

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/424377419d0d
https://hg.mozilla.org/mozilla-central/rev/44db783208d7

Comment 61

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d26e05828f37
Remove bogus XUL box sorting. r=TYLin

Comment 62

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/d26e05828f37