1493645 - {inc} Youtube video titles wrongly overlap the content sometimes.

Status: VERIFIED FIXED

(Core :: Layout: Flexbox, defect, P1)

Description

[Emilio Cobos Álvarez (:emilio)]

Attachment: Screenshot_2018-09-24 los monstruitos - YouTube.png

Trying to reduce it now, but for see screenshot for an example.

The condition is that the title almost overlapped before, and after the mutation that inserts some content to the right, the text correctly breaks but the container size doesn't increase.

This is a regression from bug 1490890.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Reduced test-case.

Fairly trivial actually... I'm surprised we didn't have tests for this.

Daniel, could you take a look? Let me know if not and I could debug it otherwise.

Comment 2

[Daniel Holbert [:dholbert]]

Thanks for the testcase! I'll take a look.

One thing I noticed: if I force a thorough relayout (e.g. by opening devtools or switching to responsive design mode), the issue goes away. So this is definitely an incremental reflow bug.  (Not too surprising, given the dynamic nature of the testcase.)

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: testcase 2

Comment 4

[Daniel Holbert [:dholbert]]

Looks like we need to include a flex item's mComputedISize as part of its cache key.

Right now, when we call SizeItemInCrossAxis() on the flex item, we retrieve the stale cached value from the previous time around, and we (incorrectly) think it's still valid because the keys are all the same. So we pull out the BSize from that cached value and return that as the item's cross size.

Really, we should be taking the item's ComputedISize into consideration, since that definitely influences the resulting BSize measurement, as shown by the testcase here.

Comment 5

[Daniel Holbert [:dholbert]]

Also: I am pretty sure we also do *not* need to take the item's AvailableISize into consideration -- i.e. we can ditch that from the key, and replace it with ComputedISize().

Reasoning on why AvailableISize() is unnecessary:
=================================================
AvailableISize tells us the "distance to the end of the line", which is useful in an inline-layout scenario (e.g. when laying out a wide <span>, it'll tell us when that span needs to fragment).

However, it's not useful in a flex-layout scenario -- flex items shouldn't care about it, because they don't (themselves) get fragmented into different inline parts. They might fragment their *children* into inline parts, but only in a way that depends on their own computed-isize-shimmed-into-available-isize -- not in a way that depends on what the flex container told them the available isize was.

I almost want to make the same argument about AvailableBSize, but I suspect that may become relevant when we get to block-axis fragmenting of flex items for printing etc. (bug 939897), so I think it's probably wise to keep it for now.

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: Bug 1493645 part 1: When caching flex item sizes, use computed inline size as part of the cache key. r?emilio

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: Bug 1493645 part 2: When caching flex item sizes, don't bother using available inline size as part of cache key. r?emilio

A flex item's available inline size would be relevant (i.e. would have an
impact on layout) if we were fragmenting the flex item in its inline direction
(e.g. if it were an inline-level box, in an inline-layout context).

But we're not doing that, so its available isize doesn't make a difference. To
the extent that it's been useful at all in this flex-item-caching code up to
this point, we'll now be caching something more-specific (the item's *computed*
inline size) which should serve roughly the same purpose.

Depends on D6991

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9012354 [details]
Bug 1493645 part 2: When caching flex item sizes, don't bother using available inline size as part of cache key. r?emilio

Emilio Cobos Álvarez (:emilio) has approved the revision.

Comment 9

[Daniel Holbert [:dholbert]]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff0fb451b0a814e6357dedd21b3674710923379d

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9012353 [details]
Bug 1493645 part 1: When caching flex item sizes, use computed inline size as part of the cache key. r?emilio

Emilio Cobos Álvarez (:emilio) has approved the revision.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6461130aba58
part 1: When caching flex item sizes, use computed inline size as part of the cache key. r=emilio
https://hg.mozilla.org/integration/autoland/rev/f71dd542e33c
part 2: When caching flex item sizes, don't bother using available inline size as part of cache key. r=emilio

Comment 12

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/6461130aba58
https://hg.mozilla.org/mozilla-central/rev/f71dd542e33c

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

web-platform-tests PR in https://github.com/web-platform-tests/wpt/pull/13244

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The WPT lint failed:

ERROR:lint:css/vendor-imports/mozilla/mozilla-central-reftests/flexbox/flexbox-dyn-resize-001-ref.html:1: CR character in line separator (CR AT EOL)
etc.

Comment 15

[Daniel Holbert [:dholbert]]

Oops, sorry! I am ooto and sans-computer until Monday - maybe Emilio could clean up after me?

Otherwise I can take care of it on Monday.

Comment 16

[Emilio Cobos Álvarez (:emilio)]

Sure, pleasure :)

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/45aa3f51c691
Fix line endings in flexbox-dyn-resize-001.html. r=me

Comment 18

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/45aa3f51c691

Comment 19

[Timea Zsoldos [:zstimi/tzsoldos], Desktop QA]

I have reproduced this issue using Firefox  64.0a1 (2018.09.24) on Win 10 x64.
I can confirm this issue is fixed, I verified using Firefox 64.0b4 on Win 10 x64, Ubuntu 16.04 x64 and Mac OS X 10.13.