470596 - css border breaks video controls

Status: VERIFIED FIXED

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

Description

[Asa Dotzler [:asa]]

Attachment: testcase with left border breaking/masking the player control

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

Comment 1

[Justin Dolske [:Dolske]]

Hmm, XUL layout bug?

FWIW, I see the same problem with my patch for bug 464371, which replaces the <image>s with <button>s.

Comment 2

[Asa Dotzler [:asa]]

who knows XUL layout and can someone please add them to this bug?

Comment 3

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

My guess is that the problem is that nsVideoFrame::BuildDisplayList passes aDirtyRect straight through to the first child without transforming the coordinates.

Comment 4

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

In particular, it should probably be calling BuildDisplayListForChild.

Comment 5

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

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.

Comment 6

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

Attachment: patch

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.

Comment 7

[Justin Dolske [:Dolske]]

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.

Comment 8

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

Comment on attachment 354340 [details] [diff] [review]
patch

Chris D doesn't need to review this.

Comment 9

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

Attachment: test

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

Comment 10

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

Attachment: fix top vs. right padding confusion when placing video controls

This fixes the placement of the controls and makes the above test pass.

Comment 11

[Justin Dolske [:Dolske]]

Comment on attachment 354554 [details] [diff] [review]
test

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

Comment 12

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

Fixed:
http://hg.mozilla.org/mozilla-central/rev/4e07397456a7
http://hg.mozilla.org/mozilla-central/rev/ee565e96b014
http://hg.mozilla.org/mozilla-central/rev/d54715fb83de

Comment 13

[Asa Dotzler [:asa]]

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 (http://hg.mozilla.org/mozilla-central/rev/d54715fb83de) both at this testcase and my original problem blog post and all seems good.  Thanks for the quick response!!

Comment 14

[Asa Dotzler [:asa]]

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.

Comment 15

[Asa Dotzler [:asa]]

re-opening because I really don't think this is fixed. See my previous comment and you can also see an additional testcase at http://weblogs.mozillazine.org/asa/archives/2008/12/theora_screenca.html

Comment 16

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

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/db0a23885b28
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/afbe9643abbe
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/137b53eb619e

I'll try to take a look at Asa's issue shortly, though I may not have time today.

Comment 17

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

Attachment: fix videocontrols mouse transition checks

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?

Comment 18

[Justin Dolske [: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.

Comment 19

[Justin Dolske [:Dolske]]

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

Comment 20

[Neil Deakin]

(In reply to comment #19)
> We don't really have a good way of automating simulated mouse movement...

Does synthesizeMouse not work here?

Comment 21

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

David, don't forget to land this :-)

Comment 22

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

Sorry, I missed that it got review from Neil.

Comment 23

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

http://hg.mozilla.org/mozilla-central/rev/3cca9872e8dd

Comment 24

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

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d88a587615f3

Comment 25

[Aakash Desai [:aakashd]]

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

Comment 26

[Aakash Desai [:aakashd]]

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