Closed Bug 1242172 Opened 8 years ago Closed 8 years ago

Transient layout problem with resized images

Categories

(Core :: Layout, defect)

42 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox42 --- affected
firefox43 --- affected
firefox44 --- wontfix
firefox45 + wontfix
firefox46 + wontfix
firefox47 + wontfix
firefox48 --- fixed

People

(Reporter: gpothier, Unassigned)

References

()

Details

(Keywords: regression)

Attachments

(6 files, 1 obsolete file)

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:43.0) Gecko/20100101 Firefox/43.0
Build ID: 20160106231004

Steps to reproduce:

A div contains and img element whose height is 100%. As the height of the div is smaller than the height of the original image, the image is resized. As a result, the image element has the correct (resized) size, however the containing div has the width of the original image. The weird thing is that this issue is transient and disappears after the DOM is changed, and remains disappeard after the DOM is back to original.
This is demonstrated by this codepen: http://codepen.io/anon/pen/xZYprB
1. Load the codepen. The images are huge because they are not yet resized, and only the top of the first image is displayed
2. On the last two CSS selectors, remove the extra "c" from .carousel. Now the images are resized, but they appear far apart, because the containing div element has the width of the original image.
3. In the html, replace the letter "a" in the "preview" div by a letter "b". Now the images appear next to each other, as the should. They stay this way after putting back the original "a". 

Please note this problem happens outside codepen, so this is not a codepen issue.


Actual results:

The images are far apart at first, but after changing the html they appear next to each other.


Expected results:

The image should always appear next to each other.
Attached image card-002.jpg
Attached file index.html
IE11 has the same behavior as FF43.
Forget my previous comment, sorry.

Regression range:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=9bca608ab65a3b1b5d95f91854f027169709f261&tochange=a1829935d87e6f2b5b4d64c0da52ca733a5b0662

Suspected bug: bug 1172239
Blocks: 1172239
Flags: needinfo?(roc)
Keywords: regression
OS: Unspecified → All
Hardware: Unspecified → All
Version: 43 Branch → 42 Branch
Status: UNCONFIRMED → NEW
Ever confirmed: true
Recent regression, tracking. Adding wontfix for 44 since we have already shipped and this doesn't sound too serious.
Attached file Test case with named selectors (obsolete) —
(In reply to gpothier from comment #0)
> Please note this problem happens outside codepen, so this is not a codepen
> issue.

Can you post a test that shows the issue without manually renaming selectors in codepen? The file I attached seems to show the intended rendering. Thanks!
Flags: needinfo?(gpothier)
Attachment #8720993 - Attachment is obsolete: true
Hi Jet, actually as far as I can tell, the file you uploaded does not render correctly: 
1. Open the file: it displays ok
2. Increase the height of the window: space appears between the images. Conversely, if you decrease the height of the window, the images overlap.
3. Reload the page with the current height: it displays ok again

The width of the div remains constant when one changes the height of the window, which is incorrect.
Flags: needinfo?(gpothier)
Getting late in 46 aurora, wontfixing since no one is assigned. 
It would still be good to fix this regression for 47.
Attached file test10.html
gpothier is quite right. Here's a simpler version of Jet's testcase that shows the bug. Resizing vertically, the images should change horizontal position, but they don't. Reload the page to see the correct horizontal positions.

This is a bit mystifying since the patches in bug 1172239 only affect style-change reflows, which I don't think are relevant here. (Except for one change to nsGfxScrollFrame which just means setting IsBResize in more case...)
Flags: needinfo?(roc)
Amusingly, Chrome has the exact same bug.
Comment on attachment 8724645 [details]
MozReview Request: Bug 1242172. Invalidate intrinsic ISizes that depend on viewport BSize when the viewport is resized. r=dbaron

https://reviewboard.mozilla.org/r/37107/#review33647

I guess I either missed or forgot when we introduced the concept of intrinsic widths depending on block-sizes.  I guess the Web depends on it, though, or we wouldn't be hitting compat bugs related to it.  Yikes.

::: layout/base/nsLayoutUtils.cpp:5181
(Diff revision 1)
> +    nsIFrame::ChildListIterator lists(f);
> +    for (; !lists.IsDone(); lists.Next()) {

Please declare lists in the for-initializer.

::: layout/base/nsPresShell.cpp:1842
(Diff revision 1)
> +      // For BSize changes driven by style, RestyleManager handles this.
> +      // For height:auto BSizes (i.e. layout-controlled), descendant
> +      // intrinsic sizes can't depend on them. So the only other case is
> +      // viewport-controlled BSizes which we handle here.
> +      nsLayoutUtils::MarkIntrinsicISizesDirtyIfDependentOnBSize(rootFrame);

Can you check the old height (save it before calling SetVisibleArea) and condition this on the height actually changing?  We certainly have width-only resizes in a number of cases (e.g., anything involving history or bookmarks sidebar, or similar).

(Not as important as optimizing for height-only resizes, but seems like it might still be worth it.)

Then again, if it's not as trivial as I think it looks, maybe not worth it?

::: layout/generic/nsGfxScrollFrame.cpp:430
(Diff revision 1)
> -  return (mHelper.mScrolledFrame->GetStateBits() & NS_FRAME_CONTAINS_RELATIVE_BSIZE) ||
> +  return mHelper.mScrolledFrame->HasAnyStateBits(
> +      NS_FRAME_CONTAINS_RELATIVE_BSIZE | NS_FRAME_DESCENDANT_INTRINSIC_ISIZE_DEPENDS_ON_BSIZE) ||

This seems much broader than what's needed, although I guess that's not really an issue with the code right here.

It seems to me that we set NS_FRAME_DESCENDANT_INTRINSIC_ISIZE_DEPENDS_ON_BSIZE more aggressively than needed in that we don't check that the specified block-size that we're doing it for is a percentage block-size rather than a length.

It also seems to me that we propagate NS_FRAME_DESCENDANT_INTRINSIC_ISIZE_DEPENDS_ON_BSIZE up the tree more aggressively than needed, since we ought to be able to stop propagating after reaching an element whose block-size does not depend on the block-size of its containing block (although after marking that one).

I guess this is ok for now, although it might be good to file those as followups if you agree.
Attachment #8724645 - Flags: review?(dbaron) → review+
Daniel: can you help get these patches into the tree?
Flags: needinfo?(dholbert)
Sure, I'll take a look tomorrow morning.
(In reply to David Baron [:dbaron] ⌚️UTC-7 from comment #14)
> Please declare lists in the for-initializer.

Fixed here.

> Can you check the old height (save it before calling SetVisibleArea) and
> condition this on the height actually changing?  We certainly have
> width-only resizes in a number of cases (e.g., anything involving history or
> bookmarks sidebar, or similar).

Fixed here (just doing the check for "is height changing" up-front).

> This seems much broader than what's needed, although I guess that's not
> really an issue with the code right here.
> [...]
> I guess this is ok for now, although it might be good to file those as
> followups if you agree.

I spun off bug 1257610 as a followup for both concerns here.

This interdiff is pretty trivial, but I'll tag dbaron for review just to be on the safe side, since today is the first time I've looked at this bug/patch.

(I verified that the included reftests still pass with these changes, BTW.)
Attachment #8731805 - Flags: review?(dbaron)
Flags: needinfo?(dholbert) → in-testsuite+
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #10)
> Getting late in 46 aurora, wontfixing since no one is assigned. 
> It would still be good to fix this regression for 47.

Note: now that the fix has landed, we're solidly into the 47-aurora timeframe.

Given that the patch is nontrivial, the patch-author is no longer actively working on Gecko, and we shipped several releases with this bug before it was even reported (and a few more releases since then): I think we should just let this ride the trains as normal, with no special backporting.  That'll give us more time to catch & address any potential regressions that this might cause.

Liz, are you ok considering this "status-firefox47: wontfix"?
Flags: needinfo?(lhenry)
https://hg.mozilla.org/mozilla-central/rev/c8f66b7c4be3
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
I think it's fine for this fix to ride with 48. Thanks Daniel for dealing with this!
Flags: needinfo?(lhenry)
You need to log in before you can comment on or make changes to this bug.