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

VERIFIED FIXED in Firefox 64

Status

()

defect
P1
normal
VERIFIED FIXED
10 months ago
9 months ago

People

(Reporter: emilio, Assigned: dholbert)

Tracking

({regression})

unspecified
mozilla64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox62 unaffected, firefox63 unaffected, firefox64+ verified)

Details

Attachments

(4 attachments, 1 obsolete attachment)

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.
Posted file 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.
Attachment #9011426 - Attachment is obsolete: true
Flags: needinfo?(dholbert)
Attachment #9011427 - Attachment mime type: text/plain → text/html
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.)
Flags: needinfo?(dholbert)
Keywords: regression
Priority: -- → P1
Summary: Youtube video titles wrongly overlap the content sometimes. → {inc} Youtube video titles wrongly overlap the content sometimes.
Posted file testcase 2
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.
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.
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 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.
Attachment #9012354 - Flags: review+
Assignee: nobody → dholbert
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.
Attachment #9012353 - Flags: review+
Pushed by dholbert@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/6461130aba58
https://hg.mozilla.org/mozilla-central/rev/f71dd542e33c
Status: NEW → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
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.
Flags: needinfo?(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.
Flags: needinfo?(dholbert) → needinfo?(emilio)
Sure, pleasure :)
Flags: needinfo?(emilio)
Pushed by emilio@crisal.io:
https://hg.mozilla.org/integration/mozilla-inbound/rev/45aa3f51c691
Fix line endings in flexbox-dyn-resize-001.html. r=me
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.