812687 - Don't reorder frame tree for flex items (because "order" shouldn't affect tab-index / nav-index)

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert]]

As noted in bug 811521, we're going keep nsFlexContainerFrame child-frames in order of the "order" property.  "order" isn't supposed to affect the tabindex of these items, or their ordering for accessibility purposes.

So, we need to at least edit nsFocusManager::GetNextTabbableContent() to make it handle flex items that are reordered in the frame tree.  (Right now it uses nsIFrameEnumerator to walk the frames, but we need to teach it that in a flex container, it should walk the content tree instead of the frame tree).

https://mxr.mozilla.org/mozilla-central/source/dom/base/nsFocusManager.cpp#2651

(This technically should be fixed before we fix bug 811521, but I might fix them in the reverse order.)

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Maybe GetNextTabbableContent should walk the content tree but do something special to walk anonymous content too. I seem to recall some API for that but I can't find it now.

Comment 3

[Daniel Holbert [:dholbert]]

Handy testcase from bug 962558:
 https://bug962558.bugzilla.mozilla.org/attachment.cgi?id=8363644
("two, tab first" should be the first thing that tab hits)

Comment 4

[Mayhem~]

You're welcome.

Comment 5

[Daniel Holbert [:dholbert]]

(Thanks for the testcase :) & sorry for this bug not having had a more searchable title. Hopefully it's more discoverable now & will pop up in the suggested-duplicates area of the new bug UI.)

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #0)
> "order" isn't supposed to affect the
> tabindex of these items, or their ordering for accessibility purposes.

Good news: we already do the correct thing [per spec] for accessibility, at least -- unlike GetNextTabbableContent() and our tab-navigation, the a11y code seems to follow the content tree, so it ignores the effects of "order". I verified this with the testcase linked in comment 3 as well as in a quick local textual testcase that I whipped up, with both the "Fangs" extension and also with the "Orca" screen-reader.

So the only thing currently broken here is tab navigation across fields on a page.

Comment 7

[Daniel Holbert [:dholbert]]

FWIW: Bo Campbell just posted a proposal on www-style, suggesting that the spec change in such a way that happens to match our current behavior here:
 http://lists.w3.org/Archives/Public/www-style/2014Dec/0235.html

Specifically, he's suggesting that tab navigation should follow "order" reordering, though speech should not (which matches what I described in comment 6).

If that proposal is accepted, this bug would become INVALID I suppose.

Comment 9

[Rodney Rehm]

Is there a reason that the tabbing sequence is allowed to be altered by the CSS property `order` specifically, but not by reversing all children via `flex-direction: column reverse`? At least the simple test at https://jsbin.com/yowiyi suggests that reversing the `flex-direction` has no effect.

Comment 10

[Daniel Holbert [:dholbert]]

(In reply to Rodney Rehm from comment #9)
> Is there a reason that the tabbing sequence is allowed to be altered by the
> CSS property `order` specifically

(It's not technically allowed in the spec yet, actually.)

> but not by reversing all children via
> `flex-direction: column reverse`?

This question is probably better-directed at the CSSWG, if & when they actually consider letting "order" affect tabbing sequence.

My short answer, though: The spec says "A flex container lays out its content in order-modified document order". If tab behavior changes per comment 7, then it would be changing for consistency with that. (following order-modified document order)

My slightly longer answer: "reverse" just flips which side is considered the "start" and the "end"; whereas "order" actually reorders a child's box *in the box tree*, almost as if that child was were reordered in the DOM. "reverse" also doesn't influence paint-order -- so if two flex items overlap (due to overflowing content, say), the one that's later in the DOM (closer to the flexbox's "end" side) will be on top, regardless of whether "reverse" is present.

Comment 11

[Adrian Roselli]

A reminder that this is still out there. I made a test page to show what happens when a user tabs through the focusable areas depending on the browser: http://codepen.io/aardrian/full/MavVeb/

All browsers except Firefox follow the source order. Flex Editor’s Draft (14 September 2015) also suggests that Firefox's behavior is non-conforming:

"Likewise, order does not affect the default traversal order of sequential navigation modes (such as cycling through links, see e.g. tabindex [HTML5])."

https://drafts.csswg.org/css-flexbox/#order-accessibility

Animation showing the tabbing in Firefox: https://twitter.com/aardrian/status/653954728506826752/photo/1

Animation showing the tabbing in Chrome: https://twitter.com/aardrian/status/653954368283217920/photo/1

Comment 12

[Jen Simmons [:jensimmons]]

+1 on fixing this so Firefox and all the other browsers have interoperability. I'm seeing conversations about tab order picking up speed, now that Authors are starting to wrap their heads around Grid and catch-up with Flexbox. Anything that can be perceived as broken for accessibility is not popular. It'd be great if Firefox could align with the spec's consensus before too many people get frustrated over this.

Comment 13

[fredrik.nicol]

Does this issue also affect context stacking? Because in this example http://codepen.io/faddee/pen/dYLBjY the appearance of the two areas are expected to look the same.

Comment 14

[Nicole Torcolini]

Is there any ETA on this? This is really causing problems, especially since people use it without knowing that it works differently on Firefox.

Comment 15

[Daniel Holbert [:dholbert]]

No concrete ETA right now, but I'm hoping to have cycles to address this and other flexbox spec changes in the next ~quarter.

Comment 16

[Nicole Torcolini]

Thanks. I'll be looking for the updates.

Comment 17

[Rodney Rehm]

In somewhat related news, the CSS Grids implementation does *not* exhibit the same bug - http://jsbin.com/votoqifixe/1/edit?html,css,output - i.e. it actually follows source order.

Comment 18

[Daniel Holbert [:dholbert]]

(Note that this bug is specifically about the "order" property messing things up, and your testcase doesn't use "order". Nonetheless, you're right -- we implemented "order" for CSS Grid in a way that's slightly more computationally expensive but doesn't cause this bug, as discussed in bug 1107786 comment 1 & bug 1107786 comment 6.)

Comment 19

[Jen Simmons [:jensimmons]]

Léonie Watson wrote a blog post recently on this problem, and why she prefers Firefox's implementation:
http://tink.uk/flexbox-the-keyboard-navigation-disconnect/

<blockquote>TLDR: The only viable way (in my opinion) for the flexbox disconnect to be resolved, is in the browser (as with the Firefox “bug”) and the accessibility tree.</blockquote>

Comment 20

[Nicole Torcolini]

I am sorry, but I very much disagree. It's CSS for crying out loud. You are going to get the same problem with other CSS. It is the fault of the developer if he/she decides to use CSS such that the keyboard order and the visual order are not the same. For people who do not use a screen reader, it may seem right to have the tab order change, but, for people who use a screen reader, it is a big problem. Because flexbox does not change the DOM order, all of the sudden the tab order and the tab order do not match, which is a problem because screen readers have other ways of traversing the page other than tabbing, and those ways follow the dom order. Throughout this bug, it says that the accessibility is correct when using flexbox on Firefox, but it is not. If the tab order and the dom order do not match, then the accessibility is broken. Okay, so maybe not changing the tab order does not seem correct, but at least it would be consistent. Sometimes, for the sake of the users and the developers who really are trying to do stuff right, even if it does not seem right, if it is consistent, then it is the correct choice. Also, if there is something that is making this hard to implement, could someone please explain this to me?

Comment 21

[Daniel Holbert [:dholbert]]

I'll be switching our flexbox code to match our CSS Grid behavior here (i.e. not reordering the frame tree based on "order"), as hinted at in comment 18.  Updating bug summary to clarify.

Comment 23

[Marco Zehe (:MarcoZ)]

(In reply to Daniel Holbert [:dholbert] from comment #6)
> (In reply to Daniel Holbert [:dholbert] from comment #0)
> > "order" isn't supposed to affect the
> > tabindex of these items, or their ordering for accessibility purposes.
> Good news: we already do the correct thing [per spec] for accessibility, at
> least -- unlike GetNextTabbableContent() and our tab-navigation, the a11y
> code seems to follow the content tree, so it ignores the effects of "order".
FYI: I filed bug 1278759 to test this in our Mochitest framework. I used test cases from both bug 1277559 and bug 962558, since they differ slightly.

Comment 24

[Neil Deakin]

Note that the actual reason for implementing the order property (ordinal in XUL flexbox) many years ago is to allow the user to modify the visual ordering of items, most specifically, the arrangement of the columns in a tree can be changed by dragging them around, allowing the items to appear visually in a different order but not requiring the application to move the nodes around in the document.

In this regard the current Firefox behaviour of also adjusting the tab order is correct, and, speaking to dbolter and surkov, the accessibility order would follow the visual order as well (I didn't test if it actually does)

What are some real world use cases for using 'order' where the visual order does not match the tab navigation order?

Comment 25

[alexander :surkov (:asurkov)]

I would be curious too to know use cases of CSS flexbox order. 

My understanding was that XUL flexbox ordinal and CSS flexbox order are just synonyms and serve for difference proposes. So ordering in the former case should affect on tab ordering and accessibility tree, while the ordering in the latter case shouldn't have any affect on tab or accessible tree. So if a web author sticks with CSS flexbox ordering to implement a tree widget to drag columns like we do in XUL, then it was a wrong tool to do that.

Comment 26

[craigfreeman]

@neil(In reply to Neil Deakin from comment #24)
> What are some real world use cases for using 'order' where the visual order
> does not match the tab navigation order?

Real world use case: On mobile breakpoints, a "submit" button appears (visually and in the DOM) before a "cancel" button and tab order goes from "submit" to "cancel." On desktop breakpoints, the "cancel" button appears before the "submit" button (using Flexbox; DOM ordering is still "submit," then "cancel") and tab focus should go to the "submit" button first, then "cancel," but it doesn't only in Firefox.

Comment 27

[Neil Deakin]

(In reply to craigfreeman from comment #26)
> Real world use case: On mobile breakpoints, a "submit" button appears
> (visually and in the DOM) before a "cancel" button and tab order goes from
> "submit" to "cancel." On desktop breakpoints, the "cancel" button appears
> before the "submit" button (using Flexbox; DOM ordering is still "submit,"
> then "cancel") and tab focus should go to the "submit" button first, then
> "cancel," but it doesn't only in Firefox.

I would expect the cancel button to be first in the tab order if it appears visually first, the same way that it does on systems (Mac and GTK for example) were the buttons appear in that order. This seems to be an another example where the spec behaviour would not match the expected behaviour.

Comment 28

[art.trityak]

Actually this looks like nice feature to have in other browsers. Based on accessibility it will be nice to have tab order based on real elements position, not DOM positions. So maybe spec should be corrected?

Comment 29

[Jen Simmons [:jensimmons]]

It might look like a "nice feature" on first glance, but there's a lot here to debate. In fact, this has been heavily debated in the CSS Working Group, and the spec was written with care. It's clear, tab order and screen reader order should follow the DOM order of the HTML, not some idea of visual order. Then it's up to authors to build their websites properly. And while they might not, it's just as likely they won't do things properly if we don't follow the spec.

This issue comes up for both Flexbox and CSS Grid. It's complex. But Both specs are clear. We should be following DOM order. And for CSS Grid, we do. So that means not only is Mozilla insisting on breaking interop by not following this bug, we are deviating from CSS Grid. Which means, as long as this is not fixed, it is *impossible* for authors to make an accessible website. 

We need to fix this. And follow what's written in the spec. Doing nothing is exasperating the problem.

Comment 30

[Daniel Holbert [:dholbert]]

Note: When fixing this bug, we need to be sure that we continue to consider the *order-reordered* flex items when picking out the first/last children & lines, for baseline determination purposes.

This is per a CSSWG resolution that just happened, here: https://github.com/w3c/csswg-drafts/issues/995#issuecomment-283413771

Comment 31

[Daniel Holbert [:dholbert]]

Finally getting around to fixing this, in part because it's needed in order to correctly-fix bug 1345873 (a pretty-bad regression that we accidentally shipped).

I'll be posting patches here shortly. The basic strategy of the patch series will be the following:
 - First: Generify the naming of our CSS Grid "order-aware iterator" class.  (Most of the patches will be pieces of that.)
 - Then: Make nsFlexContainer share that iterator for layout & painting purposes, instead of reordering the frame tree.

So tabbing index won't be affected by "order" anymore, since it depends on the frametree ordering (which we'll no longer be messing with).

Comment 32

[Daniel Holbert [:dholbert]]

Attachment: Bug 812687 part 1: Rename GridItemCSSOrderIteratorT to CSSOrderAwareFrameIteratorT, and drop "Grid" from its method names.

This patch is purely type-renames & method-renames, so it should not affect our
behavior at all.

Review commit: https://reviewboard.mozilla.org/r/126740/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/126740/

Comment 33

[Daniel Holbert [:dholbert]]

Attachment: Bug 812687 part 2: Rename GridItemCSSOrderIterator to CSSOrderAwareFrameIterator (& similar for its Reverse form).

This patch is purely type-renames, so it should not affect our behavior at all.

Review commit: https://reviewboard.mozilla.org/r/126742/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/126742/

Comment 34

[Daniel Holbert [:dholbert]]

Attachment: Bug 812687 part 3: Move CSSOrderAwareFrameIterator code to its own .h/.cpp file.

This patch just moves code, so it should not affect our behavior at all.

Review commit: https://reviewboard.mozilla.org/r/126744/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/126744/

Comment 35

[Daniel Holbert [:dholbert]]

Attachment: Bug 812687 part 4: Add an optional parameter which can make CSSOrderAwareFrameIterator use the legacy "box-ordinal-group" property.

This patch just adds an optional codepath that isn't taken yet, so it shouldn't
affect our behavior. (The next patch in the series will make use of this new
codepath.)

Note: the large code-comment that this patch adds is just taken verbatim from
some nsFlexContainerFrame.cpp code. (The original copy will be removed by the
next patch in this series, when we switch to take advantage of this new
mechanism.)

Review commit: https://reviewboard.mozilla.org/r/126746/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/126746/

Comment 36

[Daniel Holbert [:dholbert]]

Attachment: Bug 812687 part 5: Adjust nsFlexContainerFrame to use CSSOrderAwareFrameIterator instead of reordering its child frame list.

This patch shouldn't change our layout order or paint order for flex items
(though it will change our behavior for the better when an abspos child is
present, as discussed in bug 1345873).

This patch *will* change the tab-index behavior of flex items. Previously, the
default tab order would match the visual order (i.e. it would respect "order")
-- because it depends on the frame tree, and we sorted the frame tree by
"order". Now, the tab-index will come from the DOM order (the unmodified frame
tree), as the spec requires.

Review commit: https://reviewboard.mozilla.org/r/126748/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/126748/

Comment 37

[Daniel Holbert [:dholbert]]

Attachment: Bug 812687 part 6: Rename & invert polarity of a flexbox frame-state-bit, for consistency with grid.

This patch is just flipping some logic in a way that should produce the same
outcome, so it shouldn't affect behavior.

Review commit: https://reviewboard.mozilla.org/r/126750/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/126750/

Comment 38

[Daniel Holbert [:dholbert]]

Attachment: Bug 812687 part 7: Minor cleanup in existing test_flexbox_order mochitests. (no review, test-only)

This patch:
 - does s/var/let/ to upgrade to modern JS best-practices.
 - Wraps some lines that are too long.
 - Changes loops to "for (let foo of [...])" rather than foreach+function-pointer.
 - Changes some copypasted cleanup code to use a loop instead (which will be especially useful in a forthcoming version of this test that'll add another thing to clean up).

Review commit: https://reviewboard.mozilla.org/r/126768/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/126768/

Comment 39

[Daniel Holbert [:dholbert]]

Attachment: Bug 812687 part 8: Add a test for "order" on flex items with abspos siblings.

Review commit: https://reviewboard.mozilla.org/r/126770/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/126770/

Comment 40

[Daniel Holbert [:dholbert]]

Things I still need to do here:

* Tests:
 - Add a reftest or mochitest that actually tests nav-index. (I did manually test the codepen from comment 11 and verify that it's fixed with these changes.)
 - Add a reftest for the scenario described in comment 30, or make sure we already have one.  (I'm pretty sure we'll handle that correctly without any special changes, because our nsFlexContainer::Reflow code [which is primarily responsible for baseline determination] traverses our linked list of FlexLines and their FlexItems, and *those* will still be reordered [via the newly-modified GenerateFlexlines method's usage of the special Iterator class].)

* Documentation:
 - Add some documentation to the (preexisting but newly-spun-off) OrderAwareFrameIterator class.

Note that I've already included a test for bug 1345873, in the final patch here. (It's taken from the fix+test that I originally posted over on that bug; I've carried forward r=mats on it.)

Comment 41

[Mats Palmgren (inactive)]

Comment on attachment 8854795 [details]
Bug 812687 part 1: Rename GridItemCSSOrderIteratorT to CSSOrderAwareFrameIteratorT, and drop "Grid" from its method names.

https://reviewboard.mozilla.org/r/126740/#review129580

::: layout/generic/nsGridContainerFrame.h:222
(Diff revision 1)
>    static nsGridContainerFrame* GetGridFrameWithComputedInfo(nsIFrame* aFrame);
>  
>    struct TrackSize;
>    struct GridItemInfo;
>    struct GridReflowInput;
> -  template<typename Iterator> class GridItemCSSOrderIteratorT;
> +  template<typename Iterator> class OrderAwareFrameIteratorT;

I think I'd prefer to keep "CSS" in the name for clarity since there are other kinds of order to deal with, in particular FindFirst/LastItemInGridOrder which takes one of these iterators in a param.  If it's named plain OrderAware it's ambiguous if that iterator gives items in "CSS order" or "Grid order":
https://drafts.csswg.org/css-grid/#grid-order

I think it's fine to just call it CSSOrderAwareFrameIteratorT even if it handles both 'order' and 'box-ordinal-group', since they are both CSS properties, so it doesn't need to be more specific than that.

(It should be easy to rename by running "sed -ie 's/OrderAware/CSSOrderAware/g'" directly on the relevant patches unapplied).

r=mats with that

Comment 42

[Mats Palmgren (inactive)]

Comment on attachment 8854796 [details]
Bug 812687 part 2: Rename GridItemCSSOrderIterator to CSSOrderAwareFrameIterator (& similar for its Reverse form).

https://reviewboard.mozilla.org/r/126742/#review129582

Comment 43

[Mats Palmgren (inactive)]

Comment on attachment 8854797 [details]
Bug 812687 part 3: Move CSSOrderAwareFrameIterator code to its own .h/.cpp file.

https://reviewboard.mozilla.org/r/126744/#review129584

Comment 44

[Mats Palmgren (inactive)]

Comment on attachment 8854798 [details]
Bug 812687 part 4: Add an optional parameter which can make CSSOrderAwareFrameIterator use the legacy "box-ordinal-group" property.

https://reviewboard.mozilla.org/r/126746/#review129586

I think it would be good the have a doc comment for this class that gives a short overview of what functionality it provides. (Sorry, I know I should have written that myself, but moving it to a header file makes it more motivated now.)
Here's the things we could point out:
* it's used for iterating frame children on a given child list sorted on either CSS 'order' or 'box-ordinal-group' (with DOM order secondary)
* it can optionally skip placeholders
* it's good for performance to reuse the same iterator, just call Reset() to start a new iteration
* it's good for performance to give a eKnownOrdered or eKnownUnordered state to the ctor if you know it
* the behavior is undefined if the child frame list changes while iterating - don't do that!

nit:
I think you should skip this comment "(particularly since sufficiently-huge values are busted in Chrome/WebKit per https://bugs.chromium.org/p/chromium/issues/detail?id=599645 )" - it seems unimportant to me (and it may change too).

Comment 45

[Mats Palmgren (inactive)]

Comment on attachment 8854799 [details]
Bug 812687 part 5: Adjust nsFlexContainerFrame to use CSSOrderAwareFrameIterator instead of reordering its child frame list.

https://reviewboard.mozilla.org/r/126748/#review129596

Seeing "RenumberList()" in there... I wonder if it also corrects a bug regarding list numbering? (it should be in DOM order per spec https://drafts.csswg.org/css-lists/#creating-counters )  Might be worth adding test for that if so.

Comment 46

[Mats Palmgren (inactive)]

Comment on attachment 8854800 [details]
Bug 812687 part 6: Rename & invert polarity of a flexbox frame-state-bit, for consistency with grid.

https://reviewboard.mozilla.org/r/126750/#review129598

Comment 47

[Daniel Holbert [:dholbert]]

Comment on attachment 8854795 [details]
Bug 812687 part 1: Rename GridItemCSSOrderIteratorT to CSSOrderAwareFrameIteratorT, and drop "Grid" from its method names.

https://reviewboard.mozilla.org/r/126740/#review129580

> I think I'd prefer to keep "CSS" in the name for clarity since there are other kinds of order to deal with, in particular FindFirst/LastItemInGridOrder which takes one of these iterators in a param.  If it's named plain OrderAware it's ambiguous if that iterator gives items in "CSS order" or "Grid order":
> https://drafts.csswg.org/css-grid/#grid-order
> 
> I think it's fine to just call it CSSOrderAwareFrameIteratorT even if it handles both 'order' and 'box-ordinal-group', since they are both CSS properties, so it doesn't need to be more specific than that.
> 
> (It should be easy to rename by running "sed -ie 's/OrderAware/CSSOrderAware/g'" directly on the relevant patches unapplied).
> 
> r=mats with that

Sounds good to me. I've fixed that locally (and I've made some subsequently-needed indentation changes, and some filename-list-ordering changes for affected chunks of moz.build & #include chunks).  New patches will be up soon.

Comment 48

[Daniel Holbert [:dholbert]]

Comment on attachment 8854798 [details]
Bug 812687 part 4: Add an optional parameter which can make CSSOrderAwareFrameIterator use the legacy "box-ordinal-group" property.

https://reviewboard.mozilla.org/r/126746/#review129586

Yeah, I was planning on adding more documentation, per comment 40.  But in the interests of getting this landed and backported ASAP, I'm going to punt on that wordsmithing to a followup bug (which I intend to fix immediately afterwards): bug 1353888.  (I assume you're OK with that, since you filed this as a comment rather than an issue, and since the code I'd be documenting already exists as you note.)

And RE the Chrome/WebKit afterthought in the code-comment: I'll drop that part, as you suggest.

Comment 49

[Daniel Holbert [:dholbert]]

Comment on attachment 8854799 [details]
Bug 812687 part 5: Adjust nsFlexContainerFrame to use CSSOrderAwareFrameIterator instead of reordering its child frame list.

https://reviewboard.mozilla.org/r/126748/#review129596

This does indeed correct such a bug (and brings us into conformance with Chrome, at least).  I'll include a new reftest in layout/reftests/list-item/ for that.

Comment 50

[Daniel Holbert [:dholbert]]

Comment on attachment 8854795 [details]
Bug 812687 part 1: Rename GridItemCSSOrderIteratorT to CSSOrderAwareFrameIteratorT, and drop "Grid" from its method names.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/126740/diff/1-2/

Comment 51

[Daniel Holbert [:dholbert]]

Comment on attachment 8854796 [details]
Bug 812687 part 2: Rename GridItemCSSOrderIterator to CSSOrderAwareFrameIterator (& similar for its Reverse form).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/126742/diff/1-2/

Comment 52

[Daniel Holbert [:dholbert]]

Comment on attachment 8854797 [details]
Bug 812687 part 3: Move CSSOrderAwareFrameIterator code to its own .h/.cpp file.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/126744/diff/1-2/

Comment 53

[Daniel Holbert [:dholbert]]

Comment on attachment 8854798 [details]
Bug 812687 part 4: Add an optional parameter which can make CSSOrderAwareFrameIterator use the legacy "box-ordinal-group" property.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/126746/diff/1-2/

Comment 54

[Daniel Holbert [:dholbert]]

Comment on attachment 8854799 [details]
Bug 812687 part 5: Adjust nsFlexContainerFrame to use CSSOrderAwareFrameIterator instead of reordering its child frame list.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/126748/diff/1-2/

Comment 55

[Daniel Holbert [:dholbert]]

Comment on attachment 8854800 [details]
Bug 812687 part 6: Rename & invert polarity of a flexbox frame-state-bit, for consistency with grid.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/126750/diff/1-2/

Comment 56

[Daniel Holbert [:dholbert]]

Comment on attachment 8854801 [details]
Bug 812687 part 7: Minor cleanup in existing test_flexbox_order mochitests. (no review, test-only)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/126768/diff/1-2/

Comment 57

[Daniel Holbert [:dholbert]]

Comment on attachment 8854802 [details]
Bug 812687 part 8: Add a test for "order" on flex items with abspos siblings.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/126770/diff/1-2/

Comment 58

[Mats Palmgren (inactive)]

Sounds good to me, thanks.

Comment 59

[Daniel Holbert [:dholbert]]

Attachment: Bug 812687 part 9: Add tests for "order" property's influence on list-numbering, baseline, and focus order in a flexbox. (no review, test-only)

Review commit: https://reviewboard.mozilla.org/r/126992/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/126992/

Comment 60

[Daniel Holbert [:dholbert]]

Comment on attachment 8854802 [details]
Bug 812687 part 8: Add a test for "order" on flex items with abspos siblings.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/126770/diff/2-3/

Comment 61

[Daniel Holbert [:dholbert]]

The newly-posted "part 9" adds the remaining tests that we need here -- for comment 40 and comment 45.

Comment 62

[Daniel Holbert [:dholbert]]

Comment on attachment 8855094 [details]
Bug 812687 part 9: Add tests for "order" property's influence on list-numbering, baseline, and focus order in a flexbox. (no review, test-only)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/126992/diff/1-2/

Comment 63

[Daniel Holbert [:dholbert]]

(Comment 62 is for a reftest-stylo.list tweak, to keep up with a reftest that I'm renaming [and cloning] in part 9. Without that tweak, the reftest-stylo test job fails.)

Comment 64

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a6f0d546c63
part 1: Rename GridItemCSSOrderIteratorT to CSSOrderAwareFrameIteratorT, and drop "Grid" from its method names. r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/1732e9dd1f9e
part 2: Rename GridItemCSSOrderIterator to CSSOrderAwareFrameIterator (& similar for its Reverse form). r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/947d5e737c2d
part 3: Move CSSOrderAwareFrameIterator code to its own .h/.cpp file. r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6cc0dd3e7b8
part 4: Add an optional parameter which can make CSSOrderAwareFrameIterator use the legacy "box-ordinal-group" property. r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/173a4f49dfe3
part 5: Adjust nsFlexContainerFrame to use CSSOrderAwareFrameIterator instead of reordering its child frame list. r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1034f23ceb8
part 6: Rename & invert polarity of a flexbox frame-state-bit, for consistency with grid. r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/51504aea09b0
part 7: Minor cleanup in existing test_flexbox_order mochitests. (no review, test-only)
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a1df59228cb
part 8: Add a test for "order" on flex items with abspos siblings. r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/001fd3755ac1
part 9: Add tests for "order" property's influence on list-numbering, baseline, and focus order in a flexbox. (no review, test-only)

Comment 65

[Daniel Holbert [:dholbert]]

Try run (with not-quite-final version of Part 9, hence some Rs2 failures):
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=4bf6f0dc7d2d3df60d7653ee2f2cbad6a9a903da
Stylo-only try run with updated part 9:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc87859ab91dd1ef3839c2a7f39727e8f888b340

--> Landed on inbound.

Comment 66

[Daniel Holbert [:dholbert]]

Comment on attachment 8854799 [details]
Bug 812687 part 5: Adjust nsFlexContainerFrame to use CSSOrderAwareFrameIterator instead of reordering its child frame list.

I'd like to uplift all of this bug's patches to Aurora & Beta (i.e. ship these patches to users ASAP), because this fixes bug 1345873, which is a layout regression that we unfortunately shipped in Firefox 52.  (This patch-stack fixes that bug by replacing the codepath that triggers that bug with a more-spec-compliant codepath which does not have that bug.)  (I'm picking "part 5" for the approval-request simply because that's the only patch that actually changes behavior. Everything else is just refactoring in support of this part. But I'd like to uplift all of the parts.)

Approval Request Comment
[Feature/Bug causing the regression]: The regression (Bug 1345873) was caused by bug 1269045, which was really just a helper-bug for bug 874718 (a large spec change about handling abspos children in a css flexbox).
[User impact if declined]: Broken layout on any websites that has a css flexbox with "order"-reordered children as well as abspos children. (We've received several reports of such sites being broken, over on bug 1345873.)
[Is this code covered by automated tests?]: Yes. We have fairly comprehensive flexbox tests, and I've included several additional tests in this patch stack to verify our new behavior.
[Has the fix been verified in Nightly?]: I've verified it in local builds. It landed on inbound earlier today, so hopefully it'll be in Nightly tomorrow.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: It's somewhat risky, but I think the benefit outweighs the risk.
[Why is the change risky/not risky?]: It's somewhat risky because it's changing how flexbox layout works (specifically how "order" is honored for reordering children).  However, it's not as risky as it might seem, because it's co-opting an existing structure that we've already been using for the same purpose in CSS Grid.  So it's reusing existing well-tested code for a new purpose, to replace known-buggy code.  Also: don't be daunted by the number of patches here. Part 5 is the only part that actually changes behavior -- all the other parts are just moving code around, or renaming things, or adding tests, in service of the main patch here (part 5) which is relatively targeted.
[String changes made/needed]: None.

Comment 67

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cbc3fdb9aab8
part 10: Skip new mochitest test_flexbox_focus_order.html on mac for now (pending a tweak to give it sane mac focus behavior in followup bug 812687).

Comment 68

[Daniel Holbert [:dholbert]]

One of the new mochitests here is flaky on mac, because mac focus handling is special and needs a bespoke pref-tweak to become predictable (based on what I saw from skimming some similar tests earlier today). I've pushed a followup to skip that test on mac for the moment, and I'll re-enable it with the needed pref-tweak over in followup bug 1353992.  In the meantime, I'm not super-concerned with having this test disabled on one platform, since the behavior that the test is really targeting (flex item ordering) isn't platform-specific.

Comment 69

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5a6f0d546c63
https://hg.mozilla.org/mozilla-central/rev/1732e9dd1f9e
https://hg.mozilla.org/mozilla-central/rev/947d5e737c2d
https://hg.mozilla.org/mozilla-central/rev/f6cc0dd3e7b8
https://hg.mozilla.org/mozilla-central/rev/173a4f49dfe3
https://hg.mozilla.org/mozilla-central/rev/c1034f23ceb8
https://hg.mozilla.org/mozilla-central/rev/51504aea09b0
https://hg.mozilla.org/mozilla-central/rev/0a1df59228cb
https://hg.mozilla.org/mozilla-central/rev/001fd3755ac1
https://hg.mozilla.org/mozilla-central/rev/cbc3fdb9aab8

Comment 70

[Liz Henry (:lizzard) (relman/hg->git project)]

I'd like to go with the patch in bug 1345873 for beta 53 instead.

Comment 71

[Daniel Holbert [:dholbert]]

Comment on attachment 8854799 [details]
Bug 812687 part 5: Adjust nsFlexContainerFrame to use CSSOrderAwareFrameIterator instead of reordering its child frame list.

I'm canceling the Aurora54 approval request, since I'd like to uplift the spot-fix in bug 1345873 instead, to get additional testing of that fix before it [hopefully] ships in 53.

(After we've shipped that spot fix in 53, *then* I think we should uplift this bug's patches to 54 [which will be on beta at that point] early on in the cycle, since it's pretty safe and addresses some user pain, and we might as well ship it sooner if we can.  Mats suggested this in bug 1345873 comment 37, and I agree.)

So, I intend to re-request approval-mozilla-beta here in a few weeks (for the beta54 cycle), but for now, no need for uplifts here.

Comment 72

[Julien Cristau [:jcristau]]

53 has the patch from bug 1345873 instead, so marking wontfix.  I'm leaving 54 as affected based on comment 71.

Comment 73

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

(Here's a testcase for this bug. I'd been informally using the codepen from comment 11 previously, but it looks like that's disappeared now.)

Comment 74

[Adrian Roselli]

(In reply to Daniel Holbert [:dholbert] from comment #73)
> Created attachment 8859694 [details]
> testcase 1
> 
> (Here's a testcase for this bug. I'd been informally using the codepen from
> comment 11 previously, but it looks like that's disappeared now.)

The CodePen is still there: http://codepen.io/aardrian/full/MavVeb/ (or at http://codepen.io/aardrian/pen/MavVeb to get the editor view). I can see it from a non-logged-in browser.

Either way, if you have a test case then keen. If you want me to provide you with the HTML / CSS / JS from mine, I am happy to do that too.

Comment 75

[Daniel Holbert [:dholbert]]

My mistake (sort of) -- the "/full/" version of that codepen is 404 in my normal browser profile (including in a private browsing window), but it's fine in a fresh profile. Huh!  Maybe a weird interaction with one of my add-ons or cache or something.  Anyway -- all's well, now we've just got two different ways of testing this. :)

Comment 76

[Daniel Holbert [:dholbert]]

This is VERIFIED FIXED in 55.0a1 (2017-04-18) (64-bit) -- I verified using these two testcases:
 Codepen: http://codepen.io/aardrian/full/MavVeb/
 Attached testcase: attachment 8859694 [details]
We now traverse the items in source-code-order, in both of those testcases.

Comment 77

[Daniel Holbert [:dholbert]]

Comment on attachment 8854799 [details]
Bug 812687 part 5: Adjust nsFlexContainerFrame to use CSSOrderAwareFrameIterator instead of reordering its child frame list.

Hi Gerry!  I'd like to re-request beta approval for this bug's patch-stack, for the Beta 54 train. This has "approval-beta-" right now, but that's from the previous version-53 beta cycle, and it was minused there because it was late in the game and I came up with a preferred / more targeted band-aid over on bug 1345873 to fix the proximal pain-point regression that we were concerned with in that release.

Current beta (beta 54) already has that same band-aid, too -- my goal with this new uplift request isn't to fix a regression, but rather to fix a long-standing interop issue.  (I normally would've requested uplift for this interop-issue in the aurora period rather than the beta period -- but in the Aurora54 cycle here, we needed extra testing for the late-breaking-beta53-band-aid, so we took the band-aid on *both* Beta53 and Aurora54, to get that extra testing before 53 shipped. And now that 53 has shipped, I'd like 54 to upgrade to the full "better fix" that Nightly received here.)


(I'll re-answer the approval questions here, since now the main goal isn't about fixing a regression, which means the answers are a bit different.)

[Feature/Bug causing the regression]: This main bug here is not a regression, just a CSS Flexbox spec change which has resulted in interop issues & web developer pain.
[User impact if declined]: Pressing "tab" on pages like the attached testcase and http://codepen.io/aardrian/full/MavVeb/ will continue to move through flex items in a different order in Firefox, as opposed to other browsers.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Yes. (Fixed for a while, verified just now.)
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None, though we need to backout bug 1345873 for the patches to apply cleanly.  (The patches replace the code that bug 1345873 tweaked.)
[Is the change risky?]: It's somewhat risky, but I think the benefit outweighs the risk.
[Why is the change risky/not risky?]: See answer in comment 66. (Also, these changes have baked in Nightly for 2 weeks without problems now, too. And we're at the very beginning of the beta cycle, so we have time to backout if this does turn out to cause problems.)
[String changes made/needed]: None.

Comment 78

[Daniel Holbert [:dholbert]]

[ni=gchang to be sure he sees the feedback-request-which-is-really-a-beta-uplift-request. I'm really hoping to get this in towards the *start* of the Beta54 cycle, which is why I requested uplift right after the merge.]

Comment 79

[Gerry Chang [:gchang]]

Hi :dholbert,
If this is a long-standing interop issue and only baked for 2 weeks, why do we need this so urgent? I would expect this is supposed to bake more time. Can we let this ride the train on 55?

Comment 80

[Gerry Chang [:gchang]]

Comment on attachment 8854799 [details]
Bug 812687 part 5: Adjust nsFlexContainerFrame to use CSSOrderAwareFrameIterator instead of reordering its child frame list.

Please see comment #79. Let me know if you have other thoughts.

Comment 81

[Daniel Holbert [:dholbert]]

(In reply to Gerry Chang [:gchang] from comment #79)
> Hi :dholbert,
> If this is a long-standing interop issue and only baked for 2 weeks

(It's sort of baked for longer than that -- it's co-opting a bunch of code that we've already baked for a long time with CSS Grid.)

> why do
> we need this so urgent? I would expect this is supposed to bake more time.
> Can we let this ride the train on 55?

I suppose it's not really urgent -- the only urgency is that it was really the best way to fix formerly-urgent-regression bug 1345873.  And while I think the band-aid that we uplifted to 53 & 54 should work, I was also hoping to get the "full" fix out sooner.

So if I had my druthers, I'd want to land this in "early beta", and be quick to backout in case of regressions.

But it's also OK if this were to just rides the trains.

Comment 82

[Gerry Chang [:gchang]]

Hi :dholbert,
Thanks for supporting this. Mark 54 won't fix.

Comment 83

[Daniel Holbert [:dholbert]]

Last few patches included tests. in-testsuite+