If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

YouTube "create a new playlist" is half visible in + option (due to mis-grouping of flex items in a display:-webkit-box)

RESOLVED FIXED in Firefox 51

Status

()

Core
Layout
RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: sonia, Assigned: dholbert)

Tracking

(Blocks: 1 bug)

46 Branch
mozilla52
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox49 affected, fennec52+, firefox50 affected, firefox51 verified, firefox52 verified)

Details

(Whiteboard: [webcompat])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(7 attachments)

(Reporter)

Description

a year ago
User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/53.0.2785.143 Safari/537.36

Steps to reproduce:

1. Open Youtube in mozilla firefox for andriod.
2. Play any video on youtube.
3. Click on add button.
4. On clicking the button , the "create playlist" option is not clearly visible on the device.


Actual results:

On clicking the button , the "create playlist" option is not clearly visible on the device.


Expected results:

"Create playlist" should be fully visible for access on clicking the Add button.
Tested using LG G4 (Android 6.0) build 52.0a1 (2016-10-12).
I am able to reproduce this following the steps and here is a screenshot https://i.imgur.com/Hd2Ikdh.png.

Updated

11 months ago
tracking-fennec: --- → ?
This looks to be content. Mike is this something with gecko or the website?
Flags: needinfo?(miket)
This looks like fallout from our -webkit- flexbox support.

<a href="#" onclick="return koya.onEvent(arguments[0]||window.event,'39_2')" id="koya_elem_39_3">
  <li>
    <span class="_mmc _mclb"></span>
    Create a new playlist
  </li>
</a>

._mxb li {
    height: 40px;
    color: #767676;
    line-height: 40px;
    border-top: none;
    border-right: none;
    border-bottom: 1px solid #dcdcdc;
    border-left: none;
    overflow: hidden;
    display: -webkit-box;
    -webkit-line-clamp: 1;
    -webkit-box-orient: vertical;
    text-overflow: ellipsis;
}


If I kill `display: -webkit-box`, it appears as expected (in Chrome as well). 

-webkit-line-clamp: 1 here doesn't appear to have any effect.

Daniel, is there something we could do smarter here, or would you recommend we ask YouTube to remove the ancient flexbox CSS?
Flags: needinfo?(miket) → needinfo?(dholbert)
(Assignee)

Comment 4

11 months ago
(In reply to Mike Taylor [:miketaylr] from comment #3)
> This looks like fallout from our -webkit- flexbox support.

Yup. This seems to be fallout from us applying modern flexbox rules to this legacy-flexbox content (via our imperfect emulation of -webkit-box).  Specifically, we're creating two flex items (one for the <span>, one for the text), whereas chrome only creates one.

This looks like something that should've been fixed by bug 1236400, though.  DOM inspector shows that the span is "display:inline-block" and the raw text is just raw text, and bug 1236400's testcase 1 shows that we are smart enough to group those into the same flex item at least in that testcase's -webkit-box scenario.

So, needs further investigation...
(Assignee)

Comment 5

11 months ago
Created attachment 8802802 [details]
testcase 1

Here's a reduced testcase. In current Nightly, the testcase's words ("same" & "line?") are rendered on different lines (as two different flex items).  In Chrome, they render on one line (one flex item)

The thing that separates this from bug 1236400 is "overflow:hidden" on the flex container -- if I remove that, the testcase renders as expected (with the text on one line)
(Assignee)

Updated

11 months ago
Blocks: 1238668
Component: Audio/Video → Layout
Depends on: 1236400
Product: Firefox for Android → Core
(Assignee)

Comment 6

11 months ago
Yeah, so the problem is that the nsFlexContainerFrame here (the scrolled frame) is associated with a "moz-scrolled-content" style context, which (kinda paradoxically) has "display:block", which is just one of the quirks of how scrolled frames work.

And that breaks the "IsWebkitBox" check that we added to nsCSSFrameConstructor in bug 1236400 (which is a check on the "display" value of our nsFlexContainerFrame).

We need to expose nsFlexContainerFrame.cpp's "IsLegacyBox" function and call that from nsCSSFrameConstructor, instead of using its own incomplete IsWebkitBox function.
Assignee: nobody → dholbert
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Thanks Daniel (I forgot 1238668 exists).

Updated

11 months ago
Whiteboard: [webcompat]
(Assignee)

Comment 8

11 months ago
I have a patch for this locally, which I've verified fixes the issue on the testcase & on YouTube.  Just need to add an automated test, and then I should have it up for review tomorrow morning.

We should consider backporting this to Aurora 51 and Beta 50, since this manifested for users as a regression in Firefox 49 (since that's the release where we let the webkit pref ship, via bug 1259345).
Flags: needinfo?(dholbert)
(Assignee)

Updated

11 months ago
Flags: needinfo?(dholbert)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 14

11 months ago
Overview:
 * Part 0 just adds a reftest (variant of an existing one)
 * Parts 1 and 2 are idempotent -- they don't affect behavior at all.
(Part 1 simplifies the API of a helper-method & exposes it publicly; Part 2 restructures its internal logic slightly so we can verify a post-condition when we return true.)

 * Part 3 fixes the anonymous-flex-item bug that this YouTube page was tripping over, by using the more-robust & now-exposed nsFlexContianerFrame helper method. (see comment 6)
 * Part 4 fixes an additional bug that's needed for the reftest to render correctly (specifically for the "-webkit-flex-pack: justify" in the reftest to take effect) -- just the standard inherit-container-config-properties-through-to-anonymous-box stuff.

And part 4 also marks the new reftest as passing, because it does pass at that point (but not before that point).
Flags: needinfo?(dholbert)

Comment 15

11 months ago
mozreview-review
Comment on attachment 8803041 [details]
Bug 1309119 part 0: Add reftest (variant of existing reftest webkit-box-anon-flex-items-1.html, with "overflow:hidden").

https://reviewboard.mozilla.org/r/87278/#review86324
Attachment #8803041 - Flags: review?(mats) → review+

Comment 16

11 months ago
mozreview-review
Comment on attachment 8803042 [details]
Bug 1309119 part 1: Expose nsFlexContainerFrame helper-function IsLegacyBox as a static method.

https://reviewboard.mozilla.org/r/87280/#review86328
Attachment #8803042 - Flags: review?(mats) → review+

Comment 17

11 months ago
mozreview-review
Comment on attachment 8803043 [details]
Bug 1309119 part 2: Make nsFlexContainerFrame::IsLegacyBox assert that legacy -webkit-box elements are backed by nsFlexContainerFrame.

https://reviewboard.mozilla.org/r/87282/#review86332

::: layout/generic/nsFlexContainerFrame.cpp:96
(Diff revision 1)
> +  bool isLegacyBox = false;
>    // Trivial case: just check "display" directly.
>    if (IsDisplayValueLegacyBox(styleDisp)) {
> -    return true;
> +    isLegacyBox = true;

Alternatively:
bool isLegacyBox = IsDisplayValueLegacyBox(styleDisp);
if (!isLegacyBox && styleDisp->mDisplay == mozilla::StyleDisplay::Block)

::: layout/generic/nsFlexContainerFrame.cpp:111
(Diff revision 1)
>      if (IsDisplayValueLegacyBox(parentStyleContext->StyleDisplay())) {
> -      return true;
> +      isLegacyBox = true;

Seems simpler to just do:
isLegacyBox = IsDisplayValueLegacyBox(parentStyleContext->StyleDisplay());
Attachment #8803043 - Flags: review?(mats) → review+

Comment 18

11 months ago
mozreview-review
Comment on attachment 8803044 [details]
Bug 1309119 part 3: Change nsCSSFrameConstructor to use nsFlexContainerFrame::IsLegacyBox instead of its own less-complete version.

https://reviewboard.mozilla.org/r/87284/#review86334
Attachment #8803044 - Flags: review?(mats) → review+

Comment 19

11 months ago
mozreview-review
Comment on attachment 8803045 [details]
Bug 1309119 part 4: Make the scrollframe, button, & fieldset anonymous inner boxes inherit -webkit-box container CSS properties.

https://reviewboard.mozilla.org/r/87286/#review86336

::: layout/style/res/forms.css:46
(Diff revision 1)
>    /* CSS Align */
>    align-content: inherit;
>    align-items: inherit;
>    justify-content: inherit;
>    justify-items: inherit;
> +  /* -webkit-box container (aliased from -webkit versions to -moz versions) */

Should we insert this new section after the "Flex container" section instead?
They seem somewhat semantically connected.
(ditto for the other two chunks)
Attachment #8803045 - Flags: review?(mats) → review+
(Assignee)

Comment 20

11 months ago
mozreview-review-reply
Comment on attachment 8803043 [details]
Bug 1309119 part 2: Make nsFlexContainerFrame::IsLegacyBox assert that legacy -webkit-box elements are backed by nsFlexContainerFrame.

https://reviewboard.mozilla.org/r/87282/#review86332

> Alternatively:
> bool isLegacyBox = IsDisplayValueLegacyBox(styleDisp);
> if (!isLegacyBox && styleDisp->mDisplay == mozilla::StyleDisplay::Block)

Thanks - I agree that's nicer. (I was erring on the side of one-liner replacements for the "return" statements here.)

I'll fix this up before landing.

> Seems simpler to just do:
> isLegacyBox = IsDisplayValueLegacyBox(parentStyleContext->StyleDisplay());

Agreed; I'll fix this as well.
(Assignee)

Comment 21

11 months ago
mozreview-review-reply
Comment on attachment 8803045 [details]
Bug 1309119 part 4: Make the scrollframe, button, & fieldset anonymous inner boxes inherit -webkit-box container CSS properties.

https://reviewboard.mozilla.org/r/87286/#review86336

> Should we insert this new section after the "Flex container" section instead?
> They seem somewhat semantically connected.
> (ditto for the other two chunks)

Sure, that makes sense.

(I initially inserted them alongside CSS Align, because "-webkit-box-pack" was what I needed in the testcase, and that's analogous to CSS Align "justify-content". But yeah, it makes more sense to group the broader category of -webkit-box stuff up with flexbox.)
(Assignee)

Comment 22

11 months ago
mozreview-review
Comment on attachment 8803043 [details]
Bug 1309119 part 2: Make nsFlexContainerFrame::IsLegacyBox assert that legacy -webkit-box elements are backed by nsFlexContainerFrame.

https://reviewboard.mozilla.org/r/87282/#review86350

::: layout/generic/nsFlexContainerFrame.cpp:116
(Diff revision 1)
> -  return false;
> +  if (isLegacyBox) {
> +    NS_ASSERTION(aFrame->GetType() == nsGkAtoms::flexContainerFrame,
> +                 "legacy box with unexpected frame type");
> +  }

BTW -- while I'm tweaking this patch, I'm also folding this (new) if-check-and-assertion together, so it ends up like so:
  NS_ASSERTION(!isLegacyBox ||
               aFrame->GetType() == nsGkAtoms::flexContainerFrame,
               "legacy box with unexpected frame type");

(Not bothering to request re-review for this simplification, but just mentioning it here since it's one change that I'm making that's not part of the review feedback.)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 28

11 months ago
Sure, that's probably more how we usually write those.  Either way is fine though.
(Assignee)

Comment 29

11 months ago
Sounds good. Thanks for the review!

(Side note: I'll be landing on inbound, rather than autoland, since the test-changes depend on a minor cleanup patch that I landed on inbound earlier today, in bug 1231682. And that cleanup patch hasn't made it to autoland yet, so rebasing would fail if I tried to use autoland here.)

Comment 30

11 months ago
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/35f88c160489
part 0: Add reftest (variant of existing reftest webkit-box-anon-flex-items-1.html, with "overflow:hidden"). r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/b05f66ffb552
part 1: Expose nsFlexContainerFrame helper-function IsLegacyBox as a static method. r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/e437737fedc8
part 2: Make nsFlexContainerFrame::IsLegacyBox assert that legacy -webkit-box elements are backed by nsFlexContainerFrame. r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/40f9955c5b70
part 3: Change nsCSSFrameConstructor to use nsFlexContainerFrame::IsLegacyBox instead of its own less-complete version. r=mats
https://hg.mozilla.org/integration/mozilla-inbound/rev/06d4b9187508
part 4: Make the scrollframe, button, & fieldset anonymous inner boxes inherit -webkit-box container CSS properties. r=mats
(Assignee)

Updated

11 months ago
Summary: create a new playlist is half visible in + option → YouTube "create a new playlist" is half visible in + option (due to mis-grouping of flex items in a display:-webkit-box)

Comment 31

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/35f88c160489
https://hg.mozilla.org/mozilla-central/rev/b05f66ffb552
https://hg.mozilla.org/mozilla-central/rev/e437737fedc8
https://hg.mozilla.org/mozilla-central/rev/40f9955c5b70
https://hg.mozilla.org/mozilla-central/rev/06d4b9187508
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
status-firefox52: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
(Assignee)

Comment 32

11 months ago
Comment on attachment 8803044 [details]
Bug 1309119 part 3: Change nsCSSFrameConstructor to use nsFlexContainerFrame::IsLegacyBox instead of its own less-complete version.

Approval Request Comment
[Feature/regressing bug #]: bug 1259345 (which is where we shipped support for webkit-prefixed CSS). Specifically, this is a bug in our support for "display:-webkit-box" as a modified version of modern flexbox.

[User impact if declined]: Broken UI in mobile YouTube playlist UI (see screenshot at https://imgur.com/Hd2Ikdh )

[Describe test coverage new/current, TreeHerder]: New tests included; old tests for existing behavior.

[Risks and why]: Low-risk. Makes our "-webkit-box" emulation a bit more consistent with ourselves (between "overflow:hidden" & "overflow:visible" containers) & with what webkit-derived browsers do (which we're emulating here).

[String/UUID change made/needed]: None.
Attachment #8803044 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 33

11 months ago
(Approval request is for the full patch-stack.)
tracking-fennec: ? → 52+

Updated

11 months ago
status-firefox51: --- → affected

Comment 34

11 months ago
Hi :sorina, 
could you help verify if this issue is fixed as expected on the latest Nightly build? Thanks!
Flags: needinfo?(sorina.florean)
Created attachment 8805030 [details]
Screenshot_2016-10-27-08-50-28.png

Hi all,
Tested on latest Nightly with LG G4 (Android 6.0. and 5.1).
The issue is fixed as in the screenshot.
Flags: needinfo?(sorina.florean)

Updated

11 months ago
status-firefox52: fixed → verified

Comment 36

11 months ago
Comment on attachment 8803044 [details]
Bug 1309119 part 3: Change nsCSSFrameConstructor to use nsFlexContainerFrame::IsLegacyBox instead of its own less-complete version.

Fix an issue related to UI in mobile YouTube playlist. Take it in 51 aurora.
Attachment #8803044 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 37

11 months ago
part 1 etc has problems to uplift to aurora

grafting 370809:b05f66ffb552 "Bug 1309119 part 1: Expose nsFlexContainerFrame helper-function IsLegacyBox as a static method. r=mats"
merging layout/generic/nsFlexContainerFrame.cpp
merging layout/generic/nsFlexContainerFrame.h
warning: conflicts while merging layout/generic/nsFlexContainerFrame.cpp! (edit, then use 'hg resolve --mark')
warning: conflicts while merging layout/generic/nsFlexContainerFrame.h! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')
Flags: needinfo?(dholbert)
(Assignee)

Comment 38

11 months ago
Yeah, that's because bug 984869 (which isn't on central) made some changes in the contextual code.

I'm rebasing the patches and I'll push the fixed versions to aurora myself. Thanks / sorry for the trouble!
(Assignee)

Comment 39

11 months ago
(In reply to Daniel Holbert [:dholbert] from comment #38)
> Yeah, that's because bug 984869 (which isn't on central) made some changes
> in the contextual code.

Sorry, meant to say "which isn't on aurora"
(Assignee)

Comment 40

11 months ago
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/dd31ecc9eeefaef17ea23dcbe9745251415069f1
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/2fcc860cc4edfadaeb44fdc8c399bd9bdb11a3e7
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/4dbc0b8a95eac7028763933b18bbbe8662565492
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/c0119370cb9afef3ebdf797aa430fa66cb39b5af
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/3ddd4755dae1d4ef70d60c7d12f75c3491f3ca25
status-firefox50: --- → affected
status-firefox51: affected → fixed
Flags: needinfo?(dholbert)
(Assignee)

Updated

11 months ago
status-firefox49: --- → affected
Verified as fixed in build 51.0a2 (2016-11-04);
Device: LG G4 (Android 5.1).
status-firefox51: fixed → verified
You need to log in before you can comment on or make changes to this bug.