Closed Bug 470596 Opened 14 years ago Closed 14 years ago

css border breaks video controls


(Toolkit :: Video/Audio Controls, defect, P2)






(Reporter: asa, Assigned: dbaron)


(Keywords: verified1.9.1)


(5 files)

If you add a CSS border-bottom or border-left to the <video> tag, it shifts the play button's image but not the actual control. It's as if the video's box and the image in the player control both respect the growing border by shifting to the right but the clickable area of the player control does not so it gets masked. 

This interaction between the controls and the style applied to the video tag is bad enough that it makes styling the video tag basically broken so I think it needs to be fixed for 1.9.1
Flags: wanted1.9.1?
Hmm, XUL layout bug?

FWIW, I see the same problem with my patch for bug 464371, which replaces the <image>s with <button>s.
who knows XUL layout and can someone please add them to this bug?
Flags: blocking1.9.1?
My guess is that the problem is that nsVideoFrame::BuildDisplayList passes aDirtyRect straight through to the first child without transforming the coordinates.
In particular, it should probably be calling BuildDisplayListForChild.
It's actually not quite that simple, since it's currently forcing its child to have a stacking context, and BuildDisplayListForChild wouldn't do that.
Attached patch patchSplinter Review
Well, here's the simple fix.

I tried writing an automated test, but gave up.  I couldn't figure out how to get at the anonymous content to synthesize events to the mute button.
Attachment #354340 - Flags: superreview?(roc)
Attachment #354340 - Flags: review?(chris.double)
See /toolkit/content/tests/widgets/test_videocontrols.html ?

That fires clicks at the play and pause buttons. It's relative to the <video>, so I don't know a border would make it fail (without the patch). If it doesn't, maybe firing the click at a wrapper DIV would work.
Attachment #354340 - Flags: superreview?(roc)
Attachment #354340 - Flags: superreview+
Attachment #354340 - Flags: review?(chris.double)
Attachment #354340 - Flags: review+
Comment on attachment 354340 [details] [diff] [review]

Chris D doesn't need to review this.
Flags: wanted1.9.1?
Flags: blocking1.9.1?
Flags: blocking1.9.1+
Attached patch testSplinter Review
Here's a test that adds to that test; I think it's somewhat preferable to put this in the same test file because it keeps the tests that hard-code the position of the controls together.

However, this test currently fails, because of a separate bug, where adding padding-right to a video element makes the controls move *down* by the amount of the padding-right.  I still need to figure that one out.  (It passing if I change the 20px padding-right to 0.)
Attachment #354554 - Flags: review?(dolske)
This fixes the placement of the controls and makes the above test pass.
Attachment #354556 - Flags: superreview?(roc)
Attachment #354556 - Flags: review?(roc)
Attachment #354556 - Flags: superreview?(roc)
Attachment #354556 - Flags: superreview+
Attachment #354556 - Flags: review?(roc)
Attachment #354556 - Flags: review+
Attachment #354554 - Flags: review?(dolske) → review+
Comment on attachment 354554 [details] [diff] [review]

Looks fine. I suppose I should add fooButtonX and fooButtonY constants to the test at some point, so the various numerical offsets are a bit less confusing...
Closed: 14 years ago
OS: Mac OS X → All
Hardware: x86 → All
Resolution: --- → FIXED
Whiteboard: [needs 1.9.1 landing]
Target Milestone: --- → mozilla1.9.2a1
Version: unspecified → Trunk
I'll let someone else flip the verified bit based on the automated test or other testing but I wanted to let you all know that my problem case, as attached in my initial comment, is FIXED.  I tested with the Mac tinderbox build generated by this check-in ( both at this testcase and my original problem blog post and all seems good.  Thanks for the quick response!!
there's a problem with this fix or some other recent change. 

I only get the controls if I mouse into the video rectangle from a side without the border (the left edge in the testcase.) 

Steps: load the testcase.  then, bring the pointer into the video rect from the left edge across the green border and see no video controls appear. Now bring the mouse in from any other side and see it works fine.
re-opening because I really don't think this is fixed. See my previous comment and you can also see an additional testcase at
Resolution: FIXED → ---
The code for whether to handle mouseout/mouseover in videocontrols.xml was assuming that the videocontrols covered the same as the border area of the video element, when in fact it covers the content area of the video element.  In particular, when there was a border:
 * a mouse movement between the padding/border of the video and outside the video's border-box was not received, since we weren't listening for it (we only had listeners on the videocontrols)
 * a mouse movement between the padding/border of the video and its content box (where the videocontrols element is) was suppressed due to the checks that this patch changes.

  This changes the mouse event suppression check to check for whether something is the controls or a descendant rather than the video or a descendant.  (If we wanted video-or-descendant, we'd have to use mouseover/mouseout listeners on the video element instead of the videocontrols element, but making the controls appear when you end up inside the border seems right to me, although I don't have strong opinions.)

Any thoughts on how to test this?  Do you have any automated tests for detecting when the controls are present?
Attachment #355123 - Flags: review?(dolske)
Comment on attachment 355123 [details] [diff] [review]
fix videocontrols mouse transition checks

Hmm, the code was like this prior to bug 461680, but I changed it because I was seeing video <---> videocontrols transitions that were unwanted. I don't see that with this patch, so either something else fixed that problem (the .visibility changes, maybe?) or I was seeing things.

I'm ok with the change, I guess Enn should r/rs it though.
Attachment #355123 - Flags: review?(enndeakin)
Attachment #355123 - Flags: review?(dolske)
Attachment #355123 - Flags: review+
(In reply to comment #17)

> Any thoughts on how to test this?  Do you have any automated tests for
> detecting when the controls are present?

Litmus? We don't really have a good way of automating simulated mouse movement... Earlier versions of test_videocontrols.html tossed a fake mouseover event at the <video>, but that's not really a high-fidelity test, and making the test reliable is tricky due to timing issues with the control fading.
Whiteboard: [needs 1.9.1 landing] → [needs review]
Attachment #355123 - Flags: review?(enndeakin) → review+
(In reply to comment #19)
> We don't really have a good way of automating simulated mouse movement...

Does synthesizeMouse not work here?
David, don't forget to land this :-)
Assignee: nobody → dbaron
Whiteboard: [needs review] → [needs landing]
Sorry, I missed that it got review from Neil.
Closed: 14 years ago14 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing] → [needs 1.9.1 landing]
Keywords: fixed1.9.1
Whiteboard: [needs 1.9.1 landing]
Target Milestone: mozilla1.9.2a1 → mozilla1.9.1b3
verified FIXED on build 

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3) Gecko/20090305 Firefox/3.1b3 ID:20090305133223
verified FIXED on trunk on build

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090313 Minefield/3.2a1pre ID:20090313031058
Component: Video/Audio → Video/Audio Controls
Product: Core → Toolkit
QA Contact: →
Target Milestone: mozilla1.9.1b3 → ---
Version: Trunk → unspecified
Target Milestone: --- → mozilla1.9.1b3
You need to log in before you can comment on or make changes to this bug.