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.)
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.
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)
OS: Linux → All
Hardware: x86_64 → All
Summary: Make GetNextTabbableContent() not depend on frametree ordering of flex items → Make GetNextTabbableContent() not depend on frametree ordering of flex items ("order" shouldn't affect tab-index / nav-index)
(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.)
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
(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.
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.
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.
(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.
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
+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.
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.
Is there any ETA on this? This is really causing problems, especially since people use it without knowing that it works differently on Firefox.
No concrete ETA right now, but I'm hoping to have cycles to address this and other flexbox spec changes in the next ~quarter.
Thanks. I'll be looking for the updates.
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.
(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.)
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>
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?
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.
Summary: Make GetNextTabbableContent() not depend on frametree ordering of flex items ("order" shouldn't affect tab-index / nav-index) → Don't reorder frame tree for flex items (because "order" shouldn't affect tab-index / nav-index)
(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.
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?
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.
@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.
(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.
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?
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.
2 years ago
platform-rel: ? → ---
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
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).
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 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
Attachment #8854795 - Flags: review?(mats) → review+
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
Attachment #8854796 - Flags: review?(mats) → review+
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
Attachment #8854797 - Flags: review?(mats) → review+
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).
Attachment #8854798 - Flags: review?(mats) → review+
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.
Attachment #8854799 - Flags: review?(mats) → review+
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
Attachment #8854800 - Flags: review?(mats) → review+
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 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 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.
Sounds good to me, thanks.
The newly-posted "part 9" adds the remaining tests that we need here -- for comment 40 and comment 45.
(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.)
Pushed by firstname.lastname@example.org: 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)
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 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.
Pushed by email@example.com: 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).
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.
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
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
status-firefox53: --- → affected
status-firefox54: --- → affected
I'd like to go with the patch in bug 1345873 for beta 53 instead.
Attachment #8854799 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
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.
53 has the patch from bug 1345873 instead, so marking wontfix. I'm leaving 54 as affected based on comment 71.
status-firefox53: affected → wontfix
status-firefox54: affected → wontfix
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.)
(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.
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. :)
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.
Status: RESOLVED → VERIFIED
status-firefox55: fixed → verified
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.
Attachment #8854799 - Flags: feedback?(gchang)
[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.]
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?
Flags: needinfo?(gchang) → needinfo?(dholbert)
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.
Attachment #8854799 - Flags: feedback?(gchang) → feedback-
(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.
Hi :dholbert, Thanks for supporting this. Mark 54 won't fix.
status-firefox54: affected → wontfix
Last few patches included tests. in-testsuite+
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.