Closed
Bug 470596
Opened 16 years ago
Closed 16 years ago
css border breaks video controls
Categories
(Toolkit :: Video/Audio Controls, defect, P2)
Toolkit
Video/Audio Controls
Tracking
()
VERIFIED
FIXED
mozilla1.9.1b3
People
(Reporter: asa, Assigned: dbaron)
Details
(Keywords: verified1.9.1)
Attachments
(5 files)
(deleted),
text/html
|
Details | |
(deleted),
patch
|
roc
:
review+
roc
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Dolske
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
roc
:
review+
roc
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Dolske
:
review+
enndeakin
:
review+
|
Details | Diff | Splinter Review |
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?
Comment 1•16 years ago
|
||
Hmm, XUL layout bug? FWIW, I see the same problem with my patch for bug 464371, which replaces the <image>s with <button>s.
Updated•16 years ago
|
Flags: blocking1.9.1?
Assignee | ||
Comment 3•16 years ago
|
||
My guess is that the problem is that nsVideoFrame::BuildDisplayList passes aDirtyRect straight through to the first child without transforming the coordinates.
Assignee | ||
Comment 4•16 years ago
|
||
In particular, it should probably be calling BuildDisplayListForChild.
Assignee | ||
Comment 5•16 years ago
|
||
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.
Assignee | ||
Comment 6•16 years ago
|
||
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)
Comment 7•16 years ago
|
||
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] patch Chris D doesn't need to review this.
Flags: wanted1.9.1?
Flags: blocking1.9.1?
Flags: blocking1.9.1+
Priority: -- → P2
Assignee | ||
Comment 9•16 years ago
|
||
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)
Assignee | ||
Comment 10•16 years ago
|
||
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+
Updated•16 years ago
|
Attachment #354554 -
Flags: review?(dolske) → review+
Comment 11•16 years ago
|
||
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...
Assignee | ||
Comment 12•16 years ago
|
||
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
Status: NEW → RESOLVED
Closed: 16 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
Reporter | ||
Comment 13•16 years ago
|
||
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!!
Reporter | ||
Comment 14•16 years ago
|
||
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.
Reporter | ||
Comment 15•16 years ago
|
||
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 16•16 years ago
|
||
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.
Assignee | ||
Comment 17•16 years ago
|
||
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 18•16 years ago
|
||
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+
Comment 19•16 years ago
|
||
(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.
Assignee | ||
Updated•16 years ago
|
Whiteboard: [needs 1.9.1 landing] → [needs review]
Updated•16 years ago
|
Attachment #355123 -
Flags: review?(enndeakin) → review+
Comment 20•16 years ago
|
||
(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]
Assignee | ||
Comment 23•16 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/3cca9872e8dd
Status: REOPENED → RESOLVED
Closed: 16 years ago → 16 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing] → [needs 1.9.1 landing]
Assignee | ||
Comment 24•16 years ago
|
||
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d88a587615f3
Keywords: fixed1.9.1
Whiteboard: [needs 1.9.1 landing]
Target Milestone: mozilla1.9.2a1 → mozilla1.9.1b3
Comment 25•16 years ago
|
||
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
Status: RESOLVED → VERIFIED
Updated•16 years ago
|
Keywords: fixed1.9.1 → verified1.9.1
Comment 26•16 years ago
|
||
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
Updated•15 years ago
|
Component: Video/Audio → Video/Audio Controls
Product: Core → Toolkit
QA Contact: video.audio → video.audio
Target Milestone: mozilla1.9.1b3 → ---
Version: Trunk → unspecified
Updated•15 years ago
|
Target Milestone: --- → mozilla1.9.1b3
You need to log in
before you can comment on or make changes to this bug.
Description
•