1403115 - Stylo: Black dots displayed in video's seekbar at the right and left side

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect, P2)

Description

[Ciprian Georgiu, Desktop QA]

[Affected versions]:
- latest Nightly 58.0a1 (2017-09-25)
- Beta 57.0b3 (20170925150345)

[Affected platforms]:
- Mac OS X 10.11
- Ubuntu 16.04 x86

[Steps to reproduce]:
1. Start Firefox.
2. Go to https://www.quirksmode.org/html5/videos/big_buck_bunny.webm.
3. Click on the video seekbar.

[Expected result]:
- There are no black dots displayed at the right and left side of the seekbar.

[Actual result]:
- Black dots are displayed at the right and left side of the seekbar.

[Regression range]:
- I cannot reproduce this on 56.0 (Build ID 20170925181605). I will follow up with a regression range asap.

[Additional notes]:
- screencast showing this issue: https://drive.google.com/open?id=0B6qEQD4BO4Ald2R1YjdNT1l3ODA
- please note that this issue is reproducible only on Ubuntu and OS X. I can't see this behavior on Windows 7 and 10.
- other video links where this bug can be repro: https://goo.gl/WZJ5NY, http://www.html5videoplayer.net/videos/toystory.mp4

Comment 1

[Jean-Yves Avenard [:jya]]

funny I was just looking at the file as it's a technically invalid file.

However, this looks like a toolkit issue to me. The bar happens to be exactly where the time slider ends, can't be a coincidence.

Comment 3

[Ray Lin[:ralin]]

Yeh, click on time slider, volume controls as well, should not show the focus ring. It looks like a recent regression, let me try to remove it.

Thanks for your help Ciprian, jya

Comment 4

[Ciprian Georgiu, Desktop QA]

...
> [Regression range]:
> - I cannot reproduce this on 56.0 (Build ID 20170925181605). I will follow
> up with a regression range asap.

Last good revision: 1864ce9a909fd7059102ca1e02702515f249aec8
First bad revision: d1ba080ed5b0a365444c00cc038f4e8732f1f33e

Pushlog: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=1864ce9a909fd7059102ca1e02702515f249aec8&tochange=d1ba080ed5b0a365444c00cc038f4e8732f1f33e


The mozregression pushlog points me bug 1380154, however I am not sure if it actually regressed this behavior described in comment 0. It doesn't seem to be related, besides being filed against OS X and Linux.

Comment 5

[Jean-Yves Avenard [:jya]]

This is more than doubtful that bug 1380154 has anything to do with how the time slider is displayed...

Comment 6

[Jean-Yves Avenard [:jya]]

Plus the regression range provided is the backout of that bug...

Comment 7

[Alice0775 White]

I can reproduce the focus ring on Windows10.
Once navigate within the document with tab key, then click seekbar. But I think this is intentional behavior.

Comment 8

[Alice0775 White]

Attachment: sample video

STR:
1. Open any video file
2. Press [Tab] key twice　(Mouse hover over the video if no video contorols is visible)
   --- Observe focus ring around fullscreen icon, this is as expected
3. Click on seek bar
   --- Observe black vertical line at both side of the seek bar, this is bug

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=5f721a664bf64fed99184a866b60c24a6afcb3a0&tochange=62cebea1d1578461a423a9ca7848706455db9ea5

@jryans
Your bunch of patch seems to cause the problem, Can you look at this?

Comment 9

[Alice0775 White]

@jryans, ni see Comment 8

Comment 10

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

The regression range above enables Stylo by default, so it suggests a Stylo bug.

Maybe TYLin can help here, if it is XBL related?

Comment 11

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Yes. This issue is related to stylo with the XBL stylesheet :(

scrubber and volumeControl [1] use the following rules [2] in videocontrols.css to get rid of the default style of the focus ring.

  .volumeControl::-moz-focus-outer,
  .scrubber::-moz-focus-outer {
    border: 0;
  }

Both are <input type="range">, and it seems nsRangeFrame::Init() will get the pseuo element's style at [3]. I wonder if it will get the ::-moz-focus-outer style from XBL stylesheets correctly? Other pseudo elements' style from videocontrols.css do seem work fine though.

Emilio, you know pseudo elements better than I do. Perhaps you might know what's wrong here?

[1] http://searchfox.org/mozilla-central/rev/3dbb47302e114219c53e99ebaf50c5cb727358ab/toolkit/content/widgets/videocontrols.xml#163,177
[2] http://searchfox.org/mozilla-central/rev/3dbb47302e114219c53e99ebaf50c5cb727358ab/toolkit/themes/shared/media/videocontrols.css#180-183
[3] http://searchfox.org/mozilla-central/rev/3dbb47302e114219c53e99ebaf50c5cb727358ab/layout/forms/nsRangeFrame.cpp#88-91

Comment 12

[Emilio Cobos Álvarez (:emilio)]

I don't know what's wrong off-hand, but I see a couple fishy things over the XBL stylist code, so I can take a look.

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Ok, I think I got this.

Comment 14

[Emilio Cobos Álvarez (:emilio)]

So, it wasn't what I thought initially, so still digging...

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Ok, now I got it, all the fishyness gone away while at it, yay! Patch incoming.

Comment 16

[Emilio Cobos Álvarez (:emilio)]

Attachment: Patch

Comment 17

[Emilio Cobos Álvarez (:emilio)]

In particular, the problem here was that we were using the wrong `MatchingMode` for non-element-backed pseudos.

The patch fixes all sorts of fishyness (like the lack of slow selector flags), and performance issues (like the lack of bloom filter and such).

Comment 18

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Comment on attachment 8912643 [details] [review]
Patch

Emilio, Great! Thanks for fixing this promptly. I leave a nit in the PR.

Comment 19

[Bobby Holley (:bholley)]

Thanks emilio! This merged to autoland as https://hg.mozilla.org/integration/autoland/rev/44053b9ef955

Ciprian: please verify once the above is merged to Nightly.

Comment 20

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

I can verify that this is fixed on 58.0a1 (2017-09-28).

Emilio, could we help uplift your patch to 57?  (It's good to fix the visual artifact for video controls when stylo is enabled.)

Comment 21

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8912643 [details] [review]
Patch

Approval Request Comment
[Feature/Bug causing the regression]: stylo
[User impact if declined]: dots in the video seekbar
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: see steps in the bug.
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: not risky
[Why is the change risky/not risky?]: Makes the XBL path for rule matching consistent with all the others.
[String changes made/needed]: none

Comment 22

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8912643 [details] [review]
Patch

Fix a stylo regression, taking it.
Should be in 57b4

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e0067d975ceb

Comment 24

[Oana Botisan, Desktop QA]

I managed to reproduce the bug using beta 57.0b3 on Mac OS x10.11 and Ubuntu 16.04x64. 
I retested everything using the same platforms in Latest Nightly and Beta 57.0b4 and the bug is not reproducing anymore.