1512066 - Periscope.tv volume/mute icon not visible since bug 1317870

Status: VERIFIED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Mike Taylor [:miketaylr]]

(spun out from https://github.com/webcompat/web-bugs/issues/22011)

Since bug 1317870, we no longer see the mute/volume icon on periscope.tv:

Last good revision: 4e9dc6118ccad009700da01c9bcb189c8800e0ca  
First bad revision: fda2edef30ab3bb950b635d05470f5635a75a6e9  
Pushlog:  
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=4e9dc6118ccad009700da01c9bcb189c8800e0ca&tochange=fda2edef30ab3bb950b635d05470f5635a75a6e9

<div class="VideoOverlayRedesign-MuteButton">
  <button class="Touchable is-custom MuteButton MuteButton--redesign">
    <div class="Pill Pill--withIcon Pill--withText Pill--animate Pill--longOnHover Pill--disableHoverColor">
      <div class="Pill-content">
        <svg viewBox="0 0 33 23" width="33" height="23">(snip)</svg>
        <div class="sc-jwKygS fonOCa">
          <div class="sc-lhVmIH hWoYWw">
            <div class="sc-elJkPf cozKdS">
              <div class="sc-jtRfpW fbASvS" style="--percent:1;"></div>
            </div>
            <div class="sc-btzYZH kvxKJR" style="--percent:1;"></div>
          </div>
          <input class="sc-bYSBpT kytPOj" tabindex="-1" type="range" min="0" max="100" value="100">
        </div>
      </div>
    </div>
  </button>
</div>

Comment 1

[Mike Taylor [:miketaylr]]

Attachment: nightly65.png

Comment 2

[Mike Taylor [:miketaylr]]

Attachment: beta64.png

Comment 3

[Mats Palmgren (inactive)]

Is it possible to obtain the CSS rules for the elements involved?
I'm guessing there's a grid/flexbox layout in there somewhere and we
changed how the <input type="range"> is flexing/stretching somehow.

Comment 4

[Daniel Holbert [:dholbert]]

Yeah -- I can load the site from comment 0 and hit "play" and then inspect the mute button.  (Does that not work for you?  Maybe there are region restrictions, not sure.)

It's flexbox all the way down. The <button> itself has "display:flex":
=====
.MuteButton {
[...]
    display: flex;
}
=====


...and then it has 2 layers of "div" flex containers inside it (class="Pill" and "Pill-content"), with the SVG "mute" icon and the <input type="range"> as children of "Pill-content".  Those 2 divs both have "display: flex;align-items: center;", while the button has the initial/default "align-items" value, which is normal (which basically means stretch).

Comment 5

[Daniel Holbert [:dholbert]]

The stylesheet in question lives here:
https://assets.pscp.tv/css/stylesheet.7a934e357ece955a35bba79e954e2f21.css

And the rules that matter are for .MuteButton (4th find-in-page match), .Pill (1st find-in-page match), and .Pill-content (1st find-in-page match).

Comment 6

[Daniel Holbert [:dholbert]]

hackaround that works on the website side:  If I add "flex:none" to the <svg> element for the mute icon, then I get expected results.

It looks like the "collapsed"/"expanded" states of this button just have different widths, and the components have to shrink (using flex layout) in the collapsed state.  And in Firefox release, the <input> element does all of the shrinking, whereas in Nightly, the <input> and the <svg> both cooperatively shrink.  Hence, you can work around this by explicitly preventing the <svg> element from shrinking (with flex:none whose main effect in this case is to set flex-shrink:0)

Comment 7

[Daniel Holbert [:dholbert]]

Aha -- actually, in Firefox release, *neither the svg element nor the input* shrink when their outer container (the button) shrinks. In fact, their parent flex container -- .Pill-content -- remains the same width the whole time (even as its own parent, .Pill, gets bigger/smaller).  Apparently in Firefox release, we think .Pill-content has a large automatic minimum size (mostly taken from the preferred content width of the input element, I think).

I can *introduce* the bug in Firefox release by styling .Pill-content with "min-width:0", which prompts it to shrink and squash its contents (the same way it does in Firefox Nightly).

So, for contrast, in Firefox Nightly, .Pill-content behaves as if it has a small (near-0) minimum size, by default -- so it shrinks when its parent shrinks, and it forces its own flex items (the icon and the range-element) to shrink as well when that happens.

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

Comment 9

[Daniel Holbert [:dholbert]]

In Firefox nightly (65), testcase 1 renders with the upper orange box being quite skinny (fitting inside the black box).

In all other engines*: testcase 1 renders with the upper orange box being just as wide as the lower orange box. (In other words, its implied minimum width is assumed to be the same as its content width.)

* I tested Firefox release (63), Chrome 72, Edge 18, and Safari 12

Comment 10

[Daniel Bodea [:danibodea]]

The STR on which the bug was reported can be found by following the GitHub link from comment 0; Here it is:

STR1:
1.Navigate to: https://www.pscp.tv/w/1jMJgOWBljPJL or other stream on pscp.tv
2. Hover over circular volume control icon at the bottom of the screen
3. You may have to "Click to un-mute" the first time
4. Click to change the volume
Expected:
On mouse hover, the volume control slider should appear, as it does in Chrome and Safari
Actual:
Volume control slider doesn't appear in Firefox, can only mute/unmute


Another more STR is attached in comment 8; Here it is:
STR2:
https://bugzilla.mozilla.org/attachment.cgi?id=9029541


I do not know whether these test cases are complementary or if any of the is sufficient, but considering all the above, I will remove the testcase-wanted tag.

Comment 11

[Daniel Holbert [:dholbert]]

Attachment: semi-reference-case 1: normal <input> (i.e. not a range), for comparison

Comment 12

[Mats Palmgren (inactive)]

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

Comment 13

[Mats Palmgren (inactive)]

Attachment: fix+test

I was mistaken about the min-content size in bug 1317870.  It shouldn't be zero, except for the "percentage-isize quirk".

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b64bce5682de
Make <input type=range> min-content size the same as its max-content size (not zero).  r=jwatt

Comment 15

[Mats Palmgren (inactive)]

Comment on attachment 9030031 [details] [diff] [review]
fix+test

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1317870

User impact if declined: broken layout of <input type=range> in some cases

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: No

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): trivial fix

String changes made/needed:

Comment 16

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/14473 for changes under testing/web-platform/tests

Comment 17

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/b64bce5682de

Comment 18

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged

Comment 19

[Daniel Bodea [:danibodea]]

This issue is fixed in Nightly v66.0a1 from 2018-12-12. Tested using both the test cases, as described in comment 10. The issue was tested on Windows 10 and Mac OS X 10.13.6.

Marking only the firefox66 version flag as verified since it's probably going to be uplifted to beta.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9030031 [details] [diff] [review]
fix+test

[Triage Comment]
Fixes broken layout of <input type=range> in some cases. Verified by QA and adds new automated test coverage. Approved for 65.0b5.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8ed79de6c5ba

Comment 22

[Daniel Bodea [:danibodea]]

Using first STR from comment 10, it appears fixed in Beta v65.0b5, while it reproduces in Beta v65.0b4.
Using second STR (test case) from comment 10, it appears fixed in Beta v65.0b5, while it reproduces in Beta v65.0b4.
The STR (test case) from comment 11 is probably incorrect because it does not reproduce the issue in Beta v65.0b4, or I don't know how to use it properly.

Based on the above I will mark this issue as VERIFIED. 
Please note: If the test case from comment 11 is not incorrect, then I would need more information on how to use it. Please NI me if needed.

Comment 23

[Daniel Holbert [:dholbert]]

(In reply to Bodea Daniel [:danibodea] from comment #22)
> The STR (test case) from comment 11 is probably incorrect because it does
> not reproduce the issue in Beta v65.0b4, or I don't know how to use it
> properly.

To clarify: the attachment from comment 11 ("semi-reference-case 1...for comparison") was only posted for comparison against "testcase 1", as a rough approximation for how testcase 1 is expected to render.

You're correct that this file (the "semi-reference-case") never reproduced the issue. That's expected.

> Based on the above I will mark this issue as VERIFIED. 

Thanks!