Closed Bug 1403115 Opened 7 years ago Closed 7 years ago

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

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla58
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- unaffected
firefox57 --- verified
firefox58 --- verified

People

(Reporter: cgeorgiu, Assigned: emilio)

References

Details

(Keywords: regression)

Attachments

(2 files)

[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
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.
Component: Audio/Video: Playback → Video/Audio Controls
Product: Core → Toolkit
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
...
> [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.
This is more than doubtful that bug 1380154 has anything to do with how the time slider is displayed...
No longer blocks: 1380154
Plus the regression range provided is the backout of that bug...
Attached video 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?
@jryans, ni see Comment 8
Flags: needinfo?(jryans)
The regression range above enables Stylo by default, so it suggests a Stylo bug.

Maybe TYLin can help here, if it is XBL related?
Flags: needinfo?(jryans) → needinfo?(tlin)
Priority: -- → P2
Summary: Black dots displayed in video's seekbar at the right and left side → Stylo: Black dots displayed in video's seekbar at the right and left side
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
Flags: needinfo?(tlin) → needinfo?(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.
Assignee: nobody → emilio
Flags: needinfo?(emilio)
Ok, I think I got this.
Component: Video/Audio Controls → CSS Parsing and Computation
Product: Toolkit → Core
So, it wasn't what I thought initially, so still digging...
Ok, now I got it, all the fishyness gone away while at it, yay! Patch incoming.
Attached file Patch
Attachment #8912643 - Flags: review?(tlin)
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 on attachment 8912643 [details] [review]
Patch

Emilio, Great! Thanks for fixing this promptly. I leave a nit in the PR.
Attachment #8912643 - Flags: review?(tlin) → review+
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.
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: needinfo?(ciprian.georgiu)
Resolution: --- → FIXED
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.)
Status: RESOLVED → VERIFIED
Flags: needinfo?(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
Flags: needinfo?(emilio)
Attachment #8912643 - Flags: approval-mozilla-beta?
Comment on attachment 8912643 [details] [review]
Patch

Fix a stylo regression, taking it.
Should be in 57b4
Attachment #8912643 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
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.
Flags: qe-verify+
Flags: needinfo?(ciprian.georgiu)
Depends on: 1405543
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: