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

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[sonia]

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.

Comment 1

[Sorina Florean [:sflorean]]

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.

Comment 2

[Kevin Brosnan [Ex-Mozilla]]

This looks to be content. Mike is this something with gecko or the website?

Comment 3

[Mike Taylor [:miketaylr]]

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?

Comment 4

[Daniel Holbert [:dholbert]]

(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...

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: 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)

Comment 6

[Daniel Holbert [:dholbert]]

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.

Comment 7

[Mike Taylor [:miketaylr]]

Thanks Daniel (I forgot 1238668 exists).

Comment 8

[Daniel Holbert [:dholbert]]

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).

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: Bug 1309119 part 0: Add reftest (variant of existing reftest webkit-box-anon-flex-items-1.html, with "overflow:hidden").

This reftest currently has a "fails" annotation, which will be removed by a
later patch in this series (the final part of the fix).

Review commit: https://reviewboard.mozilla.org/r/87278/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/87278/

Comment 10

[Daniel Holbert [:dholbert]]

Attachment: Bug 1309119 part 1: Expose nsFlexContainerFrame helper-function IsLegacyBox as a static method.

Review commit: https://reviewboard.mozilla.org/r/87280/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/87280/

Comment 11

[Daniel Holbert [:dholbert]]

Attachment: Bug 1309119 part 2: Make nsFlexContainerFrame::IsLegacyBox assert that legacy -webkit-box elements are backed by nsFlexContainerFrame.

Review commit: https://reviewboard.mozilla.org/r/87282/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/87282/

Comment 12

[Daniel Holbert [:dholbert]]

Attachment: Bug 1309119 part 3: Change nsCSSFrameConstructor to use nsFlexContainerFrame::IsLegacyBox instead of its own less-complete version.

Review commit: https://reviewboard.mozilla.org/r/87284/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/87284/

Comment 13

[Daniel Holbert [:dholbert]]

Attachment: Bug 1309119 part 4: Make the scrollframe, button, & fieldset anonymous inner boxes inherit -webkit-box container CSS properties.

(We already do this for the properties that we use to configure modern flexbox.)

Review commit: https://reviewboard.mozilla.org/r/87286/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/87286/

Comment 14

[Daniel Holbert [:dholbert]]

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).

Comment 15

[Mats Palmgren (inactive)]

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

Comment 16

[Mats Palmgren (inactive)]

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

Comment 17

[Mats Palmgren (inactive)]

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());

Comment 18

[Mats Palmgren (inactive)]

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

Comment 19

[Mats Palmgren (inactive)]

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)

Comment 20

[Daniel Holbert [:dholbert]]

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.

Comment 21

[Daniel Holbert [:dholbert]]

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.)

Comment 22

[Daniel Holbert [:dholbert]]

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 23

[Daniel Holbert [:dholbert]]

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").

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87278/diff/1-2/

Comment 24

[Daniel Holbert [:dholbert]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87280/diff/1-2/

Comment 25

[Daniel Holbert [:dholbert]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87282/diff/1-2/

Comment 26

[Daniel Holbert [:dholbert]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87284/diff/1-2/

Comment 27

[Daniel Holbert [:dholbert]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87286/diff/1-2/

Comment 28

[Mats Palmgren (inactive)]

Sure, that's probably more how we usually write those.  Either way is fine though.

Comment 29

[Daniel Holbert [:dholbert]]

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

[Pulsebot]

Pushed by [e-mail]:
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

Comment 31

[Ryan VanderMeulen [:RyanVM]]

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

Comment 32

[Daniel Holbert [:dholbert]]

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.

Comment 33

[Daniel Holbert [:dholbert]]

(Approval request is for the full patch-stack.)

Comment 34

[Gerry Chang [:gchang]]

Hi :sorina, 
could you help verify if this issue is fixed as expected on the latest Nightly build? Thanks!

Comment 35

[Sorina Florean [:sflorean]]

Attachment: 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.

Comment 36

[Gerry Chang [:gchang]]

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.

Comment 37

[Carsten Book [:Tomcat]]

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')

Comment 38

[Daniel Holbert [:dholbert]]

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!

Comment 39

[Daniel Holbert [:dholbert]]

(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"

Comment 40

[Daniel Holbert [:dholbert]]

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

Comment 41

[Sorina Florean [:sflorean]]

Verified as fixed in build 51.0a2 (2016-11-04);
Device: LG G4 (Android 5.1).