Closed Bug 1192302 Opened 4 years ago Closed 4 years ago

Show/hide video controls not working in latest Nightly

Categories

(Core :: CSS Parsing and Computation, defect)

42 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla43
Tracking Status
firefox41 --- unaffected
firefox42 + verified
firefox43 --- verified

People

(Reporter: admin, Assigned: heycam)

References

Details

(Keywords: regression)

Attachments

(2 files)

User Agent: Mozilla/5.0 (X11; Linux i686; rv:42.0) Gecko/20100101 Firefox/42.0
Build ID: 20150807030210

Steps to reproduce:

Test case here:
http://jsfiddle.net/ejbpy7vn/
In the few tests I've done, the bug seems only to occur for videos in an HTML document and not videos opened in their own tab.



Actual results:

Showing/hiding the video controls does not work, whether when done via the right click menu or via Javascript with video.controls = true.  The right click menu option changes to "hide" or "show" as appropriate, but the controls do not appear/disappear.



Expected results:

video.controls = true should add controls to the video, and the show/hide controls item in the right click menu should toggle the presence of the video controls.
Status: UNCONFIRMED → NEW
Component: Untriaged → Audio/Video
Ever confirmed: true
Product: Firefox → Core
This must have something to do with not traversing down into the <video>'s shadow tree content to propagate the eRestyle_SomeDescendants.
Assignee: nobody → cam
Status: NEW → ASSIGNED
Tracked for 42, due to regression and interference with expected behavior for video.
This doesn't seem like our area. Can someone in DOM take a look?
Component: Audio/Video → DOM
k17e thinks layout is closer.
Component: DOM → Layout
Confirmed that the problem is that the current eRestyle_SomeDescendants handling, which just traverses down the flattened content tree, is insufficient.  We need to instead traverse down the frame tree -- like the normal restyle process does -- and then down the content tree once we encounter undisplayed nodes.  That will lead us into the video element's native anonymous content for the controls.
Component: Layout → CSS Parsing and Computation
A few things I wanted to note:

* Apologies for needing to add 9 methods to handle this frame/content tree traversal.  I started trying to abstract out the traversal so that we could use it for restyling and for this eRestyle_SomeDescendants propagation, but it was getting ugly.  So we unfortunately have a bit of duplication; I've added comments above the parallel RestyleBlah/ConditionallyRestyleBlah methods to point out that you probably want to change both at once.

* I am not certain that it's necessary to check XBL/Web Components insertion point elements to see if they're a restyle root.  In fact it might be wrong to do this check -- I think GetFlattenedTreeParent, which RestyleTracker::FindClosestRestyleRoot uses, will never return the insertion element.  Does that sound right?

* It's probably not necessary to follow the same traversal pattern of placeholder/oof frames that RestyleContentChildren does, but I thought it better to keep the two methods as similar as possible.

* I don't *think* I need to handle null frame mContent pointers, since we should only find frames with null mContent when restyling from the root of the frame tree, which only happens in RebuildAllStyleData, which always runs with eRestyle_ForceDescendants and thus we'll never get eRestyle_Stop(WithStyleChange), which are the cases where we call ConditionallyRestyleChildren.
Attachment #8656378 - Flags: review?(bzbarsky)
Comment on attachment 8656375 [details] [diff] [review]
Part 1: Make MustCheckUndisplayedContent take its frame as an argument.

r=me
Attachment #8656375 - Flags: review?(bzbarsky) → review+
> Does that sound right?

Yes (re insertion point elements not ever being returned from GetFlattenedTreeParent).
Comment on attachment 8656378 [details] [diff] [review]
Part 2: Traverse the frame tree when processing eRestyle_SomeDescendants.

r=me, I think.  My confidence in having caught a bug if there is one is low.  :(
Attachment #8656378 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] from comment #11)
> > Does that sound right?
> 
> Yes (re insertion point elements not ever being returned from
> GetFlattenedTreeParent).

OK great, I will remove those sections updating the restyle root then.

(In reply to Boris Zbarsky [:bz] from comment #12)
> Comment on attachment 8656378 [details] [diff] [review]
> Part 2: Traverse the frame tree when processing eRestyle_SomeDescendants.
> 
> r=me, I think.  My confidence in having caught a bug if there is one is low.
> :(

:(  Though I shouldn't be using you as a bug checker!  We'll be uplifting this to beta to avoid the Amazon performance pain persisting much longer.  I'll get that in ASAP to maximize the testing coverage.
https://hg.mozilla.org/mozilla-central/rev/82c3fc440262
https://hg.mozilla.org/mozilla-central/rev/e17583a5022f
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Comment on attachment 8656375 [details] [diff] [review]
Part 1: Make MustCheckUndisplayedContent take its frame as an argument.

(for both patches)

Approval Request Comment
[Feature/regressing bug #]: bug 1180118
[User impact if declined]: incorrect styling of native anonymous content, including <video> controls
[Describe test coverage new/current, TreeHerder]: patches been on m-c for a few days, manually verified that it resolves the video controls issue
[Risks and why]: Moderate: restyle is complicated in the way it traverses the tree, and if wrong could result in incorrectly styled content like in the bug.  The alternative is to back out bug 1180118 from Aurora, but in bug 1172239 we'll be wanting to uplift bug 1180118 (and its two regression fixes) to Beta to fix a performance issue.
[String/UUID change made/needed]: N/A
Attachment #8656375 - Flags: approval-mozilla-aurora?
Attachment #8656378 - Flags: approval-mozilla-aurora?
Cameron, I am not sure I want to take this patch in aurora. 42 is a special release and this could cause some unexpected regressions.

I would prefer the backout of 1180118 to mitigate the risks.
Hmm, I guess I'm more concerned with perpetuating the poor Amazon performance for another release than potentially having to fix regressions in this code on Aurora/Beta over the coming weeks.  (And this applies to bug 1180120 and bug 1202512 too, the other two dependencies of bug 1172239 not in 42.)  roc/dbaron/bz, what do you think of the tradeoffs here?
Flags: needinfo?(roc)
Flags: needinfo?(dbaron)
Flags: needinfo?(bzbarsky)
Personally I think fixing Amazon perf is extremely important and we should take some risk for it.

I'm a bit more worried about bug 1202512 than this bug actually...
Flags: needinfo?(roc)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #20)
> Personally I think fixing Amazon perf is extremely important and we should
> take some risk for it.

I agree.  I think at this stage of the release we should still be trying to get the Amazon fixes, and the regression fixes for them, uplifted to aurora.  But definitely sooner rather than later.
Flags: needinfo?(dbaron)
I'm actually pretty scared of all these style changes at this stage, but I do think this patch is safe enough...
Flags: needinfo?(bzbarsky)
Comment on attachment 8656375 [details] [diff] [review]
Part 1: Make MustCheckUndisplayedContent take its frame as an argument.

OK, the 3 DE having the same opinion on this. Let's do it then!
Attachment #8656375 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8656378 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: qe-verify+
Verified fixed using latest Aurora 43.0a2 (buildID: 20151007004048) and Firefox 42 Beta 4 (buildID: 20151005144425).
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.