Closed Bug 783474 Opened 7 years ago Closed 7 years ago

Paint flex items in order of "order" property

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED DUPLICATE of bug 811521

People

(Reporter: dholbert, Assigned: dholbert)

References

()

Details

Attachments

(2 files, 1 obsolete file)

Currently, the patches in bug 666041 have us just paint a flex container's items in frametree order.  We need to paint them in order of the "order" property, when that property is set.

I might do this as part of bug 666041, but I'm filing a bug to make sure this is addressed before we enable flexbox in builds.

(I took a crack at this before, but it depended on bug 748646 / bug 772690 -- without those bugs' patches, the display list would end up being re-sorted out from under me.

Now that those bugs are fixed, I think this should be pretty straightforward...

(Side note: Eventually we'll want to address this by re-ordering the frame tree according to "order", but we can't do that right now because tab-index and probably some a11y stuff depend on the frametree ordering, and the "order" property is explicitly _not_ supposed to affect tab-index and a11y.)
Attached file testcase 1
Here's a testcase. The green button should be closest to the user -- it should cover part of the orange button.

The green button should also be first in the tab-index order (before the orange button).
Depends on: 807897
Attached patch fix v1 (obsolete) — Splinter Review
Here's the patch for this bug. Tests coming.

Overview of the patch:
* When we sort the children for reflow, make a note of whether mFrameList is already in sorted order.
  - If it is already sorted, then we don't need to do anything special when painting. (This keeps us from slowing down the usual-case, when "order" isn't explicitly set.)
  - If it's unsorted, then we create a sorted copy before we paint, and we use that to determine our paint-ordering.

(I also had to add a forward-declaration to let me call BuildSortedChildArray() from BuildDisplayList(), since BuildDisplayList() is towards the top of the file.)
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Attachment #677680 - Flags: review?(dbaron)
Depends on: 808767
Attached patch fix v2Splinter Review
Rebased fix on top of bug 808767's patch, and added a reftest & commit message.
Attachment #677680 - Attachment is obsolete: true
Attachment #677680 - Flags: review?(dbaron)
Attachment #679887 - Flags: review?(dbaron)
Comment on attachment 679887 [details] [diff] [review]
fix v2

Actually, per bug 811521, we're going to be sorting the actual child-frame list by "order" for other reasons -- so this bug's patch won't need to create a special sorted copy anymore.

Un-requesting review.
Attachment #679887 - Flags: review?(dbaron)
This may end up being trivially fixed by bug 811521, but I'm leaving it as a separate bug (dependent on bug 811521) for now.
Depends on: 811521
Duping to bug 811521, since (per comment 5) this was trivially fixed by that bug and is effectively a dupe.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 811521
You need to log in before you can comment on or make changes to this bug.