1192302 - Show/hide video controls not working in latest Nightly

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[ccd0]

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.

Comment 1

[Alice0775 White]

[Tracking Requested - why for this release]:

Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=25af02694bad&tochange=6f97ff142c89

Suspect:  Bug 1180118

Comment 2

[Cameron McCormack (:heycam)]

This must have something to do with not traversing down into the <video>'s shadow tree content to propagate the eRestyle_SomeDescendants.

Comment 3

[jen strickland :jenstrickland]

Tracked for 42, due to regression and interference with expected behavior for video.

Comment 4

[Ralph Giles (:rillian)]

This doesn't seem like our area. Can someone in DOM take a look?

Comment 5

[Ralph Giles (:rillian)]

k17e thinks layout is closer.

Comment 6

[Cameron McCormack (:heycam)]

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.

Comment 7

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e9911a866372

Comment 8

[Cameron McCormack (:heycam)]

Attachment: Part 1: Make MustCheckUndisplayedContent take its frame as an argument.

Comment 9

[Cameron McCormack (:heycam)]

Attachment: Part 2: Traverse the frame tree when processing eRestyle_SomeDescendants.

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.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8656375 [details] [diff] [review]
Part 1: Make MustCheckUndisplayedContent take its frame as an argument.

r=me

Comment 11

[Boris Zbarsky [:bzbarsky]]

> Does that sound right?

Yes (re insertion point elements not ever being returned from GetFlattenedTreeParent).

Comment 12

[Boris Zbarsky [:bzbarsky]]

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

Comment 13

[Cameron McCormack (:heycam)]

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

Comment 14

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=33fbcafb338f

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/82c3fc440262
https://hg.mozilla.org/integration/mozilla-inbound/rev/e17583a5022f

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/82c3fc440262
https://hg.mozilla.org/mozilla-central/rev/e17583a5022f

Comment 17

[Cameron McCormack (:heycam)]

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

Comment 18

[Sylvestre Ledru [:Sylvestre]]

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.

Comment 19

[Cameron McCormack (:heycam)]

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?

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 22

[Boris Zbarsky [:bzbarsky]]

I'm actually pretty scared of all these style changes at this stage, but I do think this patch is safe enough...

Comment 23

[Sylvestre Ledru [:Sylvestre]]

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!

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1228198a8cc1
https://hg.mozilla.org/releases/mozilla-aurora/rev/423d8aa88b3a

Comment 25

[Camelia Badau [:cbadau], Desktop Test Engineering]

Verified fixed using latest Aurora 43.0a2 (buildID: 20151007004048) and Firefox 42 Beta 4 (buildID: 20151005144425).