Closed Bug 1261703 Opened 8 years ago Closed 8 years ago

When moving a flex item's frame, position its view as well as any child views

Categories

(Core :: Web Painting, defect)

Product:
Core
Component:
Web Painting
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla48
Tracking Flags:
Tracking Status
firefox48 --- fixed

People

(Reporter: tnikkel, Assigned: tnikkel)

Details

Attachments

(2 files)

patch
1.25 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
testcase 1 (seems to load without any trouble)
433 bytes, text/html
Details 
      No description provided.
Attached patch patch — — Splinter Review 
PositionChildViews only moves child views. I can't prove easily that this frame won't have a view; maybe it's obvious to you Daniel that it can't have a view. Is there code somewhere else that would move it's view that I'm missing?
Attachment #8737633 - Flags: review?(dholbert)
I don't think it ever has a view.  Very few frames have views these days,
PluginFrame, SubDocumentFrame, ViewportFrame and the combobox dropdown and XUL menus.
(In reply to Mats Palmgren (:mats) from comment #2)
> I don't think it ever has a view.  Very few frames have views these days,
> PluginFrame, SubDocumentFrame, ViewportFrame and the combobox dropdown and
> XUL menus.

I don't know about the structure of flex frames to know if one of those can be in this code.
Oh, from the bug summary I thought you were talking about nsFlexContainerFrame itself.
Looking at the patch though, it's a flex item, which could be a PluginFrame or
SubDocumentFrame, so yeah that patch looks correct.
This patch makes sense in theory, but I can't make this bug cause problems in practice (yet), so I'm slightly hesitant until we've got a testcase.

Here's the testcase I was using to test this.

I verified that the "hover"-triggered margin-tweak here makes us take the MoveFlexItemToFinalPosition codepath (the one modified in this patch), not the slower & more thorough nsFlexContainerFrame::ReflowFlexItem codepath.

I don't see any bad symptoms in this testcase, though it's possible I'm not sure what symptoms to look for...
Attachment #8737697 - Attachment description: testcase 1 (seems to l → testcase 1 (seems to load without any trouble)
The bounds of views aren't used for much anymore (so this bug isn't hugely important). The most important use of view bounds is probably sizing of widgets. So if your testcase had a plugin frame, with a widget, that might cause problems. But if not, then the easiest way to test this bug would be to inspect the view bounds and compare them to the frame bounds in a debugger (they should be the same, except the offset would be to the parent view instead of the parent frame).
Thanks!

So, in my testcase, we are in fact already getting a call to PositionFrameView for the nsSubDocumentFrame, because the flex container's parent-block calls nsContainerFrame::PlaceFrameView here:
 http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsBlockFrame.cpp?rev=8ae38f8e1def#2449
...which recursively walks the descendants and updates their view positions.

So, the flex container's neglect is being addressed by its block parent, it seems.

So I think we could only get into a situation where this causes trouble if it were possible for a nsFlexContainerFrame to have
 (1) a child frame with a view
 (2) zero ancestors that do any view-positioning -- i.e. its parent cannot be a block or a flex container frame.

This can almost happen for the root frame itself (<html style="display:flex">) except I don't think it's possible to have an <iframe> as a direct child of that frame -- we'll generate an intermediate <body> if one isn't already there explicitly.

So: not convinced we can actually cause trouble in the real world, but I'll buy that we should do it from a correctness/consistency perspective -- and I'll buy that it's possible that some future refactoring could cause bugs if not for this patch.
Comment on attachment 8737633 [details] [diff] [review]
patch

>Bug 1261703. When moxing flex frame, position its view as well as any child views.

s/moxing/moving/

r=me with that
Attachment #8737633 - Flags: review?(dholbert) → review+
Summary: When moxing flex frame, position its view as well as any child views → When moving a flex item's frame, position its view as well as any child views
Thanks for digging in to that!
Sure! Thanks for catching this bug.
(In reply to Daniel Holbert [:dholbert] from comment #7)
> So, in my testcase, we are in fact already getting a call to
> PositionFrameView for the nsSubDocumentFrame, because the flex container's
> parent-block calls nsContainerFrame::PlaceFrameView here:
>  http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsBlockFrame.
> cpp?rev=8ae38f8e1def#2449
> ...which recursively walks the descendants and updates their view positions.

It walks until it finds a view, then stops descending (because it doesn't need to update any views in that subtree).

> So, the flex container's neglect is being addressed by its block parent, it
> seems.
> 
> So I think we could only get into a situation where this causes trouble if
> it were possible for a nsFlexContainerFrame to have
>  (1) a child frame with a view
>  (2) zero ancestors that do any view-positioning -- i.e. its parent cannot
> be a block or a flex container frame.

or (3) a frame with a view between ancestors that do view positioning and the flex child item.

But I don't think that can happen either, since (as Mats mentions in comment 2) the complete list of frames with views is "PluginFrame, SubDocumentFrame, ViewportFrame and the combobox dropdown and XUL menus".
https://hg.mozilla.org/mozilla-central/rev/2d9d427348ed
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Component: Layout: View Rendering → Layout: Web Painting
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: