Closed Bug 1192302 Opened 4 years ago Closed 4 years ago
Show/hide video controls not working in latest Nightly
4.09 KB, patch
|Details | Diff | Splinter Review|
19.30 KB, patch
|Details | Diff | Splinter Review|
Status: UNCONFIRMED → NEW
Component: Untriaged → Audio/Video
Ever confirmed: true
Product: Firefox → Core
[Tracking Requested - why for this release]: Pushlog: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=25af02694bad&tochange=6f97ff142c89 Suspect: Bug 1180118
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.
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?
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...
(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.
I'm actually pretty scared of all these style changes at this stage, but I do think this patch is safe enough...
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+
Verified fixed using latest Aurora 43.0a2 (buildID: 20151007004048) and Firefox 42 Beta 4 (buildID: 20151005144425).
You need to log in before you can comment on or make changes to this bug.