758620 - Better support for zooming (including asynchronous rendering) with fixed position layers

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Chris Lord [:cwiiis]]

Bug #607417 has landed, which gives us basic support for asynchronous scrolling and zooming with fixed position layers. Unfortunately, fixed position elements are still always laid out with respect to the unzoomed viewport, so zooming in can cause elements to become inaccessible.

We would like to, initially, have the same behaviour as the Android Honeycomb/ICS browser, and have fixed position elements layout with respect to the current viewport, taking into account zoom (so a fixed position element with bottom: 0 will always be anchored to the bottom of the screen, even when you zoom in).

Summarising discussion from bug #6074717, we would like for each fixed position element to receive its own layer (or at least, where fixed position elements have different anchoring, receive their own layer), whereby we can record in layer FrameMetrics the information we need to asynchronously pan/zoom.

We would also like to be able to specify a viewport (likely via nsIDOMWindowUtils) to which fixed position elements will be laid out with respect to, so that we can call this function when we change the zoom level. This will also assist us when we want to go further than this and have fixed position elements anchor to the CSS viewport instead of the screen.

I've been trying to accomplish this first part - tn said:

"I don't know if this will work out but it's the first thing I came up with. In nsIFrame::BuildDisplayListForChild where we go from a placeholder frame to its out of flow near the start check if it is a fixed frame. If it is then wrap the resulting display list in a new display item type specifically for giving fixed items their own layers. It's BuildLayer method can set whatever metadata you need on the layer. This is more complicated because it is building a display list set, so you'll probably want to implement TryMerge on the new display item so they can all get merged, hopefully. roc may have a better idea."

Does anyone (roc?) have any comments or other ideas?

Designating this as a gfx bug at the moment, but it likely involves equal work in layout and gfx.

Comment 1

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

(In reply to Chris Lord [:cwiiis] from comment #0)
> We would also like to be able to specify a viewport (likely via
> nsIDOMWindowUtils) to which fixed position elements will be laid out with
> respect to, so that we can call this function when we change the zoom level.
> This will also assist us when we want to go further than this and have fixed
> position elements anchor to the CSS viewport instead of the screen.

Why not just go directly to anchoring the elements to the CSS viewport instead of the screen, if that's what you want? (Are you sure that's what you want?)

> "I don't know if this will work out but it's the first thing I came up with.
> In nsIFrame::BuildDisplayListForChild where we go from a placeholder frame
> to its out of flow near the start check if it is a fixed frame. If it is
> then wrap the resulting display list in a new display item type specifically
> for giving fixed items their own layers. It's BuildLayer method can set
> whatever metadata you need on the layer. This is more complicated because it
> is building a display list set, so you'll probably want to implement
> TryMerge on the new display item so they can all get merged, hopefully. roc
> may have a better idea."
> 
> Does anyone (roc?) have any comments or other ideas?

Sounds reasonable, but it might be a good idea to not try to merge (since we can't always safely merge) and handle multiple layers for the same element. AFAICT we should be able to make this "just work".

Comment 2

[Chris Lord [:cwiiis]]

Attachment: Part 1 - Make sure fixed position elements get their own layers

Is this what you meant, or similar to it? In my very short testing, it seems to work (fixed position elements end up with their own individual layers)

Comment 3

[Timothy Nikkel (:tnikkel)]

Comment on attachment 631025 [details] [diff] [review]
Part 1 - Make sure fixed position elements get their own layers

Yeah, this seems good.

Comment 4

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

Giving a fixed-pos element its own layer can change the rendering in rare cases where the fixed-pos element's descendants are interleaved in z-order with other elements.

Comment 5

[Chris Lord [:cwiiis]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (away June 9-19) from comment #4)
> Giving a fixed-pos element its own layer can change the rendering in rare
> cases where the fixed-pos element's descendants are interleaved in z-order
> with other elements.

I'd have thought that if this was the case, the z-ordering would force things to get their own layers anyway and it'd render correctly? Of course, knowing very little about this code, I don't doubt you :) Could you elaborate though?

Comment 6

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

Attachment: testcase

In this testcase, the fixed-pos element's descendants are interrupted in z-order by a non-fixed element.

Comment 7

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

In http://lists.w3.org/Archives/Public/www-style/2012May/0473.html James Robinson proposed making position:fixed elements always induce a stacking context, to get around this problem.

The problem is that this is interoperably implemented on desktop and a few sites depend on it. It would be nice not to break that.

Comment 8

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

Actually come to think of it, your patch won't break the testcase in comment #6, because you're only wrapping display items which are already forced to be contiguous in z-order due to the wrapping in nsDisplayWrapList. So you can do no harm.

It does mean that the "extraPositionedDescendants" (like the z-index:2 element in my example) won't get the special treatment you want. That's probably OK though. Capturing their layout constraints correctly for the compositor would probably be hard anyway.

It would be cleaner to create your own subclass of nsDisplayOwnLayer with its own display item type for your purposes. This would clearer and would help preserve the invariant that no two display items for a given frame have the same display item type/key.

Also, your subclass can override BuildLayer and set the right properties on the layer it generates, based on the style of the fixed-pos frame.

You'll want to look at the spec: http://www.w3.org/TR/CSS2/visudet.html#abs-non-replaced-width
probably the way to go is to figure out some subset of conditions that we should detect and propagate to the layer compositor, and leave the rest unhandled (doing what we currently do). I'd handle situations with "bottom:0", "left:0", "top:0" and/or "right:0" and bail on the rest.

You'll check the frame's style using frame->GetStylePosition(), which will tell you what style was requested (auto, percentage, length). frame->mRect will give you the actual computed position and dimensions of the border-box, in appunits, if you need that.

Comment 9

[Chris Lord [:cwiiis]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (away June 9-19) from comment #8)
> You'll want to look at the spec:
> http://www.w3.org/TR/CSS2/visudet.html#abs-non-replaced-width
> probably the way to go is to figure out some subset of conditions that we
> should detect and propagate to the layer compositor, and leave the rest
> unhandled (doing what we currently do). I'd handle situations with
> "bottom:0", "left:0", "top:0" and/or "right:0" and bail on the rest.

I think we ought to handle more than that - I quite like the way the Android honeycomb/ics browser handles this: All positioning for fixed elements is treated as unscaled and relative to unscaled screen coordinates. So if something has top: 10px, its y position will remain unchanged at 10px from the edge of the top of the window, regardless of zoom. Likewise, if it has top: 10% and the screen is 800 pixels tall, it will remain at 80 pixels from the top of the window.

In this situation, if you have fixed position elements in the four corners of the screen (like http://chrislord.net/files/mozilla/fixed.html) and zoom in, they appear to grow towards each other, but their origin (whose anchor is determined by which of top/left/right/bottom are specified) remains unchanged.

This would require positioning of fixed elements to change based on resolution... This would alter the meaning of resolution though, so perhaps we need to have a separate configurable scale factor for the positioning of fixed position elements, so we can set it to 1/resolution when changing resolution?

I can't immediately think of a less contrived way of doing this without breaking previous behaviour. It would be good to get a hold of a recent iPhone/iPad and confirm if they have act similarly - if they do, I really think we ought to mimic them.

Comment 10

[Chris Lord [:cwiiis]]

Attachment: Part 1 - Make sure fixed position elements get their own layers + metadata

This does the same thing as the first patch, but uses a new display-list item and also sets an 'anchor' point on the layer via said item, to allow us to reposition layers correctly when async zooming.

Comment 11

[Chris Lord [:cwiiis]]

I've prototyped the necessary CompositorParent changes, and we end up with behaviour that (in my limited testing) appears identical to the ICS Android browser.

Of course, as soon as layout re-renders at a non-1.0 resolution, the results are incorrect.

The third part of this work will be to alter layout to take into account the resolution when laying out fixed position items. I was thinking a boolean property on the presShell that the resolution is set on, something like 'ResolutionScalesFixedPositioning' (please suggest a better name!) could be set to enable/disable this behaviour?

I assume, especially with the new high-res Macbook Pros, that resolution may start getting used outside of fennec using it for zooming, which is why I suggest this boolean to control the layout behaviour.

Comment 12

[Chris Lord [:cwiiis]]

I thought about this some more, and I think that the simplest current solution would be for fixed position items to layout to a scroll-port if one has been set - As we update that with the zoom level, and it represents the unscaled viewport, this would suit us quite nicely and not require any extra API.

Even nicer, I think, would be to replace the scroll-port size with a single ResolutionScalesViewport boolean on the presShell (default true to maintain existing behaviour) that would affect both of these things, and this would remove some complication in mobile/android/chrome/content/browser.js too, but that's an aside.

Now, when setting the scroll-port, any fixed position items on the same presShell  would need to change their position. They oughtn't need to reflow, so I guess we could just change their position and recalculate the overflow areas using the overflow areas changed hint introduced in bug 524925.

I'm not sure exactly how you'd go about this yet, but that's my current plan and I'll now be looking at implementing it. If anyone agrees with my second paragraph (about replacing the scroll-port size), it ought to be a pretty easy change to make later.

Comment 13

[Chris Lord [:cwiiis]]

Just a note, when width/height are percentages of the containing block, the ICS browser ends up with a visible re-render after the zoom where the elements will change position (the intermediary frames are correct).

I can't quite tell exactly what's going wrong in this case (perhaps they're not applying the zoom at some point during layout?), but I think the method I describe would avoid it, if it's possible.

My current method that just alters the size given to the containing block on reflow doesn't work for this case (the fixed position elements will of course get smaller).

I've altered my test (at http://chrislord.net/files/fixed.html) to represent what I hope is a pretty pathological case that I want to work. When zooming, the fixed position elements should grow in direct proportion to the current zoom, and they should stay anchored in their respective corners. Ideally, there should be no reflow either.

Comment 14

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

(In reply to Chris Lord [:cwiiis] from comment #12)
> Now, when setting the scroll-port, any fixed position items on the same
> presShell  would need to change their position. They oughtn't need to
> reflow, so I guess we could just change their position and recalculate the
> overflow areas using the overflow areas changed hint introduced in bug
> 524925.

If the scrollport size changes we need to reflow, yes? If it just moves, we don't.

Comment 15

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

(I like the idea of attaching fixed-pos frames to the scrollport.)

Comment 16

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

Comment on attachment 632341 [details] [diff] [review]
Part 1 - Make sure fixed position elements get their own layers + metadata

Review of attachment 632341 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with those fixed

::: gfx/layers/Layers.h
@@ +739,5 @@
> +   * is considered the "anchor" point, that is, the point which remains in the
> +   * same position when compositing the layer tree with a transformation
> +   * (such as when asynchronously scrolling and zooming).
> +   */
> +  void SetFixedPositionAnchor(const nsPoint& aAnchor) { mAnchor = aAnchor; }

Don't use appunits in layers. This should be gfxPoint probably.

::: layout/generic/nsFrame.cpp
@@ +1978,2 @@
>    if (childType == nsGkAtoms::placeholderFrame) {
> +    isPlaceholder = true;

Call it hasPlaceholder

@@ +2176,5 @@
> +        rv = aLists.PositionedDescendants()->AppendNewToTop(new (aBuilder)
> +            nsDisplayFixedPosition(aBuilder, child, &list));
> +      else
> +        rv = aLists.PositionedDescendants()->AppendNewToTop(new (aBuilder)
> +            nsDisplayWrapList(aBuilder, child, &list));

{} around these bodies. But share as much code as you can, so share the AppendNewToTop call.

Comment 17

[Chris Lord [:cwiiis]]

Attachment: Part 2 - Layout fixed-pos children to the clamping scroll-port

This does what I suggested, though it reflows all abspos children and not just fixedpos children (I think) - any suggestions on a concise way of doing that? And does this seem like the right way to go about this?

I also struggled for a while to see if I could have it only alter positioning when the scrollport changes so that we could get away without reflowing, but unfortunately I'm not that clever. I'd have thought reflowing just the fixedpos children, especially as they'll be on their own layers, shouldn't be too expensive? (depending on content of course)

Comment 18

[Chris Lord [:cwiiis]]

Attachment: Part 3 - Respect layer anchor point in CompositorParent

Just attaching this in case anyone wants to see progress so far - this patch isn't complete... I'd like it to keep fixed position layers within the page boundaries (which the old code did, but in a conflicting manner).

This also shows up that the anchor points aren't correct, and that they should be based on the frame rect of the containing block and not its own frame rect, so I'll update part 1 to fix that (and address the other review comments).

Comment 19

[Chris Lord [:cwiiis]]

(In reply to Chris Lord [:cwiiis] from comment #18)
> This also shows up that the anchor points aren't correct, and that they
> should be based on the frame rect of the containing block and not its own
> frame rect, so I'll update part 1 to fix that (and address the other review
> comments).

Just to correct/clarify, it's not that they should be based on the containing block, but that they should include the frame's offsets (which would end up anchoring it to the edges of the scroll-port in this situation, as desired). Or something along these lines anyway.

Comment 20

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

Comment on attachment 634450 [details] [diff] [review]
Part 2 - Layout fixed-pos children to the clamping scroll-port

Review of attachment 634450 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsPresShell.cpp
@@ +8968,5 @@
> +    // XXX Actually, we only need to reflow fixedpos, but that's trickier.
> +    nsIFrame* rootScrollFrame = GetRootScrollFrame();
> +    if (rootScrollFrame) {
> +      nsIFrame* absContainingBlock = GetAbsoluteContainingBlock(rootScrollFrame);
> +      FrameNeedsReflow(absContainingBlock, eResize, NS_FRAME_IS_DIRTY);

This is weird. The root frame is the container for fixed-pos children, so why not just get the root frame, then iterate over GetChildList(kFixedList)?

::: layout/generic/nsAbsoluteContainingBlock.cpp
@@ +392,5 @@
> +      GetScrollPositionClampingScrollPortSize();
> +    if (-1 != containingBlockScrollWidth)
> +      containingBlockScrollWidth = size.width;
> +    containingBlockScrollHeight = size.height;
> +  }

Maybe it would be easier to modify ViewportFrame::Reflow to pass a different size to GetAbsoluteContainingBlock()->Reflow?

Comment 21

[Chris Lord [:cwiiis]]

Attachment: Part 1 - Layout fixed position elements to the clamping scroll-port

Reworked part 2 - decided to re-order, as it makes more sense this way.

Comment 22

[Chris Lord [:cwiiis]]

Attachment: Part 2 - Make sure fixed position elements get their own layers + metadata

Addressed review comments. Only difference, the anchor point is the edge of the containing block, rather than the edge of the frame rect (this is what we really wanted and provides the correct results in combination with part 3).

Comment 23

[Chris Lord [:cwiiis]]

Attachment: Part 3 - Respect layer anchor point in CompositorParent

This respects the layer's anchor point as well as keeping the layers within the page area, regardless of how you zoom/pan.

Comment 24

[Chris Lord [:cwiiis]]

Comment on attachment 634942 [details] [diff] [review]
Part 3 - Respect layer anchor point in CompositorParent

Removing r? as I've noticed a rather annoying problem that I think I can mitigate/fix before checking this in. Sorry for the spam.

Comment 25

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

Comment on attachment 634925 [details] [diff] [review]
Part 2 - Make sure fixed position elements get their own layers + metadata

Review of attachment 634925 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsDisplayList.cpp
@@ +2030,5 @@
> +  // any positioning set (left/top/right/bottom) indicates that the
> +  // corresponding side of its container should be the anchor point,
> +  // defaulting to top-left.
> +  nsIFrame* parentFrame = mFrame->GetParent();
> +  nsPresContext *presContext = parentFrame->PresContext();

Let's call this viewportFrame since that's what it is.

@@ +2049,5 @@
> +  nsIFrame *activeScrolledRoot =
> +    nsLayoutUtils::GetActiveScrolledRootFor(this, aBuilder);
> +  gfx3DMatrix ctm =
> +    nsLayoutUtils::GetTransformToAncestor(parentFrame, activeScrolledRoot);
> +  gfxRect absAnchorRect = ctm.TransformBounds(anchorRect);

I think here you should be computing the anchorRect relative to the nsDisplayListBuilder's reference frame (using this->ToReferenceFrame() as the topleft of anchorRect) and then converting to layer pixels using factor and the scale factors in aContainerParameters. That will give you layer pixel coordinates in the coordinate system of this layer (to which the layer's transform will be applied).

::: layout/generic/nsFrame.cpp
@@ +2170,5 @@
>      // Genuine stacking contexts, and positioned pseudo-stacking-contexts,
>      // go in this level.
>      if (!list.IsEmpty()) {
> +      // Make sure fixed position placeholder frames get their own layer, and
> +      // that the necessary metadata is set on that layer.

Comment is wrong. placeholders don't get their own layer, that's not what you want or do here.

@@ +2172,5 @@
>      if (!list.IsEmpty()) {
> +      // Make sure fixed position placeholder frames get their own layer, and
> +      // that the necessary metadata is set on that layer.
> +      nsDisplayItem* item;
> +      if (hasPlaceholder && disp->mPosition == NS_STYLE_POSITION_FIXED) {

Instead of this check, I think you should check !this->GetParent() && disp->mPosition == NS_STYLE_POSITION_FIXED. That means we're a fixed-pos frame hanging off the viewport frame. It's possible to have fixed-pos style frames whose parent is not the viewport frame, and you don't want to handle that here.

Comment 26

[Chris Lord [:cwiiis]]

Attachment: Part 2 - Make sure fixed position elements get their own layers + metadata

Addressed review comments, I hope.

Comment 27

[Chris Lord [:cwiiis]]

Attachment: Part 3 - Respect layer anchor point in CompositorParent

Fixed the case of zooming out from a non-1.0x zoom enough to be able to see more than the content rect.

Comment 28

[Chris Lord [:cwiiis]]

Comment on attachment 635792 [details] [diff] [review]
Part 2 - Make sure fixed position elements get their own layers + metadata

Argh, I didn't test this properly... It isn't correct.

Comment 29

[Ali Juma [:ajuma]]

Comment on attachment 635793 [details] [diff] [review]
Part 3 - Respect layer anchor point in CompositorParent

Review of attachment 635793 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/ipc/CompositorParent.cpp
@@ +318,5 @@
>  {
>    if (aLayer->GetIsFixedPosition() &&
>        !aLayer->GetParent()->GetIsFixedPosition()) {
> +    const gfxPoint& anchor = aLayer->GetFixedPositionAnchor();
> +    gfxPoint translation(aTranslation.x - (anchor.x - anchor.x / aScaleDiff.x) / aResolution.x,

An explanation of this calculation would be helpful. When this is called by TransformShadowTree, aResolution.x is rootScaleX, and aScaleDiff.x is either rootScaleX * mXScale or just rootScaleX. In the first case, the above is equivalent to:
aTranslation.x/rootScaleX - anchor.x/rootScaleX + anchor.x/(mXScale * rootScaleX * rootScaleX)

It seems weird that we need to divide by the square of rootScaleX.

Comment 30

[Chris Lord [:cwiiis]]

Attachment: Part 2 - Make sure fixed position elements get their own layers + metadata

Tested this time.

Regarding your comments, I've interpreted them to the best of my knowledge - if something is incorrect here, I'll need a little more help understanding (sorry).

Comment 31

[Chris Lord [:cwiiis]]

Attachment: Part 3 - Respect layer anchor point in CompositorParent

Part 2 has changed, which simplified the equation - I added a comment explaining it, regardless.

Comment 32

[Chris Lord [:cwiiis]]

Just a note, we may want to file a follow-up bug to replicate the anchor-point code in layout/ipc/RenderFrameParent.

This also seems to expose a bug that when you let go of a pinch-zoom that's zoomed out so overscroll is on both sides of an axis, we ask for a render with those viewport metrics, and almost immediately afterwards, a render with the metrics after the snap animation has finished - we probably shouldn't ask for that intermediate render, it could interrupt animations (and this causes a visible glitch as the scroll-port is set to a larger value than the content size, causing parts of fixed position elements to be clipped).

Comment 33

[Chris Lord [:cwiiis]]

Attachment: Part 3 - Respect layer anchor point in CompositorParent

Sorry, removed an unused variable.

Comment 34

[Chris Lord [:cwiiis]]

Attachment: Part 4 - Clamp scroll-port to content size in browser.js

This removes part of the glitch when zooming out to over-scroll.

Comment 35

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 636359 [details] [diff] [review]
Part 4 - Clamp scroll-port to content size in browser.js

Review of attachment 636359 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM

Comment 36

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

Comment on attachment 636335 [details] [diff] [review]
Part 2 - Make sure fixed position elements get their own layers + metadata

Review of attachment 636335 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsFrame.cpp
@@ +2174,5 @@
> +      if (!child->GetParent()->GetParent() &&
> +          disp->mPosition == NS_STYLE_POSITION_FIXED) {
> +        item = new (aBuilder)nsDisplayFixedPosition(aBuilder, child, &list);
> +      } else {
> +        item = new (aBuilder)nsDisplayWrapList(aBuilder, child, &list);

Space after (aBuilder)

Comment 37

[Chris Lord [:cwiiis]]

Seems this code works perfectly on the Flyer, but the 'zoomed out to beyond content size' case is broken on a Galaxy Nexus... Investigating.

Comment 38

[Chris Lord [:cwiiis]]

Pushed with comment addressed and small equation fix to part 3 (reviewed on IRC):

http://hg.mozilla.org/integration/mozilla-inbound/rev/03efce585669
http://hg.mozilla.org/integration/mozilla-inbound/rev/9b952d924953
http://hg.mozilla.org/integration/mozilla-inbound/rev/ddd519d0767e
http://hg.mozilla.org/integration/mozilla-inbound/rev/de70e79ced32

Comment 39

[Ed Morley [:emorley]]

Backed out for reftest failures:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=de70e79ced32

https://hg.mozilla.org/integration/mozilla-inbound/rev/941774f975ac

Comment 40

[Chris Lord [:cwiiis]]

So, the reftest failures point to there being some kind of rounding issue in basic layers :( (no issue on Mac/Windows builds)

I'll delve into this tomorrow.

Comment 41

[Chris Lord [:cwiiis]]

The failures were precision errors with alpha blending (composited layers vs. cairo) - Discussed with roc, agreed to mark the affected tests as fuzzy.

try run is green, hopefully second-time lucky!

http://hg.mozilla.org/integration/mozilla-inbound/rev/2866469094c3
http://hg.mozilla.org/integration/mozilla-inbound/rev/4d177002f473
http://hg.mozilla.org/integration/mozilla-inbound/rev/cfa7d5e01f60
http://hg.mozilla.org/integration/mozilla-inbound/rev/6f3be04963ad
http://hg.mozilla.org/integration/mozilla-inbound/rev/5ad1927e8011

Comment 42

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2866469094c3
https://hg.mozilla.org/mozilla-central/rev/4d177002f473
https://hg.mozilla.org/mozilla-central/rev/cfa7d5e01f60
https://hg.mozilla.org/mozilla-central/rev/6f3be04963ad
https://hg.mozilla.org/mozilla-central/rev/5ad1927e8011

Comment 43

[Chris Peterson [:cpeterson]]

cwiiis, these patches cause NS_ABORT_IF_FALSE() aborts on Android debug builds:

###!!! ABORT: clamped(): max must be greater than or equal to min: 'max >= min'"

The problem is that CompositorParent::TransformShadowTree() calls clamped(a=-57.3570824, min=0, max=-17.4481812) here:

https://hg.mozilla.org/mozilla-central/rev/6f3be04963ad#l1.74

Max < min because max == (mContentRect.XMost() - mWidgetSize.width / tempScaleDiffX) == (0 + 720) - (720 / 0.976339757) == -17.448203699442274!

Is |(mContentRect.XMost() - mWidgetSize.width / tempScaleDiffX)| correct? Should it be |((mContentRect.XMost() - mWidgetSize.width) / tempScaleDiffX)|?


#0  NS_DebugBreak_P (aSeverity=3, aStr=0x62acbba4 "clamped(): max must be greater than or equal to min", aExpr=<optimized out>, aFile=<optimized out>, aLine=57) at ~/mozilla/central/xpcom/base/nsDebugImpl.cpp:264
#1  0x61f44f2c in mozilla::clamped<float> (a=@0x648ffc2c, min=@0x648ffc28, max=@0x648ffc24) at ../../../../dist/include/nsAlgorithm.h:57
#2  0x624db1f2 in mozilla::layers::CompositorParent::TransformShadowTree (this=0x63c67c00) at ~/mozilla/central/gfx/layers/ipc/CompositorParent.cpp:430
#3  0x624db3b6 in mozilla::layers::CompositorParent::Composite (this=0x63c67c00) at ~/mozilla/central/gfx/layers/ipc/CompositorParent.cpp:262
#4  0x624d9f18 in DispatchToMethod<mozilla::layers::CompositorParent, void (mozilla::layers::CompositorParent::*)()> (arg=<optimized out>, method=<optimized out>, obj=<optimized out>) at /Users/cpeterson/Code/m
ozilla/central/ipc/chromium/src/base/tuple.h:383
#5  RunnableMethod<mozilla::layers::CompositorParent, void (mozilla::layers::CompositorParent::*)(), Tuple0>::Run (this=<optimized out>) at ~/mozilla/central/ipc/chromium/src/base/task.h:307
#6  0x62460512 in MessageLoop::RunTask(Task*) () from ~/mozilla/central/OBJDIR-ANDROID/dist/bin/libxul.so
#7  0x62460d38 in MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask const&) () from ~/mozilla/central/OBJDIR-ANDROID/dist/bin/libxul.so
#8  0x62461a86 in MessageLoop::DoWork() () from ~/mozilla/central/OBJDIR-ANDROID/dist/bin/libxul.so
#9  0x62462066 in base::MessagePumpDefault::Run (this=0x63708080, delegate=0x648ffdd4) at ~/mozilla/central/ipc/chromium/src/base/message_pump_default.cc:23
#10 0x62460aae in MessageLoop::RunInternal() () from ~/mozilla/central/OBJDIR-ANDROID/dist/bin/libxul.so
#11 0x62460b0e in MessageLoop::Run() () from ~/mozilla/central/OBJDIR-ANDROID/dist/bin/libxul.so
#12 0x6246b744 in base::Thread::ThreadMain (this=0x637df0d0) at ~/mozilla/central/ipc/chromium/src/base/thread.cc:156
#13 0x6247b3ba in ThreadFunc (closure=0x3) at ~/mozilla/central/ipc/chromium/src/base/platform_thread_posix.cc:31
#14 0x40084c50 in __thread_entry () from /Users/cpeterson/Code/mozilla/jimdb/lib/014691061901701A/system/lib/libc.so
#15 0x400847a4 in pthread_create () from /Users/cpeterson/Code/mozilla/jimdb/lib/014691061901701A/system/lib/libc.so
#16 0x00000000 in ?? ()

Comment 44

[Chris Lord [:cwiiis]]

(In reply to Chris Peterson (:cpeterson) from comment #43)
> cwiiis, these patches cause NS_ABORT_IF_FALSE() aborts on Android debug
> builds:
> 
> ###!!! ABORT: clamped(): max must be greater than or equal to min: 'max >=
> min'"
> 
> The problem is that CompositorParent::TransformShadowTree() calls
> clamped(a=-57.3570824, min=0, max=-17.4481812) here:
> 
> https://hg.mozilla.org/mozilla-central/rev/6f3be04963ad#l1.74
> 
> Max < min because max == (mContentRect.XMost() - mWidgetSize.width /
> tempScaleDiffX) == (0 + 720) - (720 / 0.976339757) == -17.448203699442274!
> 
> Is |(mContentRect.XMost() - mWidgetSize.width / tempScaleDiffX)| correct?
> Should it be |((mContentRect.XMost() - mWidgetSize.width) / tempScaleDiffX)|?

Yes, I think it should, but I'd want to test it out first - it may ought to be mContentRect.XMost() / tempScaleDiffX - mWidgetSize.width... It's difficult to keep the various coordinate spaces in my head at once without testing it out...

Well spotted though, and oops on my part. Will ping you on IRC later to see how we want to go about fixing this.

Comment 45

[Chris Lord [:cwiiis]]

Attachment: Fix incorrect offset clamping

The original equation was almost correct, it was actually the min that was the incorrect value. Dividing by the scale diff puts the compositor-side coordinates in the space of the content, and multiplying does the opposite - so dividing the min value was incorrect.

The value should also have been set in the else block of the if statement that checks if both sides of an axis are visible - this is the condition in which max could be less than min.

cpeterson - if you could double-check that this fixes the issue, that'd be much appreciated!

Comment 47

[Kartikaya Gupta (email:[e-mail])]

The patch fixes the crash for me (well at least in my limited testing so far).

Comment 48

[Chris Lord [:cwiiis]]

(In reply to Kartikaya Gupta (:kats) from comment #47)
> The patch fixes the crash for me (well at least in my limited testing so
> far).

That's good enough for me - pushed to inbound:

http://hg.mozilla.org/integration/mozilla-inbound/rev/8ca7a4f665a1

Comment 49

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/8ca7a4f665a1

Comment 50

[Chris Peterson [:cpeterson]]

Comment on attachment 638090 [details] [diff] [review]
Fix incorrect offset clamping

LGTM

Comment 51

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

Chris, I think we should fix the regressions in FF16 by only creating nsDisplayFixedPositions when shadowlayers are enabled (and landing that patch on Aurora).

Comment 52

[Chris Lord [:cwiiis]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #51)
> Chris, I think we should fix the regressions in FF16 by only creating
> nsDisplayFixedPositions when shadowlayers are enabled (and landing that
> patch on Aurora).

To clarify, do you mean just for aurora? That sounds fine to me (though I do hope that bug #769541 will fix the last of the regressions). Will still need to land bug 769541 on Aurora too, for android.

If you mean for central too, though I'm hardly in a position to argue, I'd rather we didn't so that we can get better testing and work out any problems.

Comment 53

[Chris Lord [:cwiiis]]

Attachment: Only use nsDisplayFixedPosition when a displayport is set

I didn't know the way to check for shadow layers being enabled without a LayerManager off-hand, but here's a rebased patch that was hanging about in my tree that I guess does what you want using the displayport instead(?)

A quick test shows nsDisplayFixedPosition not being used in desktop, and async zooming still working on mobile, so I guess this works. Applies on top of the patch in bug 769541.

Comment 54

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

(In reply to Chris Lord [:cwiiis] from comment #52)
> To clarify, do you mean just for aurora? That sounds fine to me (though I do
> hope that bug #769541 will fix the last of the regressions). Will still need
> to land bug 769541 on Aurora too, for android.

Yes, only for Aurora.

Comment 55

[Chris Lord [:cwiiis]]

Comment on attachment 645339 [details] [diff] [review]
Only use nsDisplayFixedPosition when a displayport is set

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Possible visual regressions (incorrect or missing rendering) due to display-list creation changes
User impact if declined: User may experience unforseen glitches
Testing completed (on m-c, etc.): Tested locally and pushed to try
Risk to taking this patch (and alternatives if risky): Low risk, I think.
String or UUID changes made by this patch: None

Comment 56

[Alex Keybl [:akeybl]]

Comment on attachment 645339 [details] [diff] [review]
Only use nsDisplayFixedPosition when a displayport is set

[Triage Comment]
Given where we are in the cycle, and the fact that multiple devs have seen these issues in their own work, fast tracking to Aurora 16.

Comment 57

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/efce6a8b9117