STR: (Win7_64, Nightly 44, 32bit, ID 20151027030239, new profile) 1. Open attached testcase Result: Video controls are double struck-through AND underlined. Strike-through is there because of text-decoration applied to <video>, underline is there because of <a> element Expectations: No strike-through and no underline should be visible in video controls Leaving unconfirmed because, who knows, maybe that's how it supposed to work.

Hmm. Not sure if this is right. It does seem strange. Gerv

Our response is kind of "don't do that then". We're not too fussed about the bleed through. It is weird since they're not part of the content dom, but being able to style the default controls doesn't seem bad. Please reopen if there's a spec or security issue with this.

I just regularly see that on web pages when <video> is a child of <a> element. Nowadays web developers test their stuff only on 1 browser (+ maybe IE), so it may not look good on firefox. Well, if this experience is "by-design" then I have no questions.

Hmm, I thought we fixed this in bug 554717. But, oh, you can't simply turn off text-decoration. D: https://developer.mozilla.org/en-US/docs/Web/CSS/text-decoration Appears the usual workaround for this is to use |display: inline-block|? (eg bug 786946)

Well, not so simple here. :( Doing that on any of .durationLabel, .durationBox, or .controlsOverlay (in videocontrols.css) has no effect, the duration label remains underlined. I don't know why... Something to do with the controls being XUL, and weirdness -moz-box / HTML interation? Gijs: any clue here? Or do we just need someone from layout/CSS to help?

(In reply to Justin Dolske [:Dolske] from comment #5) > Well, not so simple here. :( Doing that on any of .durationLabel, > .durationBox, or .controlsOverlay (in videocontrols.css) has no effect, the > duration label remains underlined. I don't know why... Something to do with > the controls being XUL, and weirdness -moz-box / HTML interation? > > Gijs: any clue here? Or do we just need someone from layout/CSS to help? I've poked at this for half an hour but I can't figure out a magic incantation to make this stop. I've tried setting inline-block on most things in the hierarchy, and nothing helped. Setting it on the <videocontrols> themselves just crashes, actually... The best I came up with is forcing <video>'s text-decoration to none, but that doesn't fix inheritance from links or other things, it just happens to help with this testcase. dholbert, any idea why this is so difficult? Are we missing an obvious solution?

(I also tried setting the text-decoration's color to transparent in the XUL, but that also didn't help, which makes no sense to me at all, because the testcase sets it to white on the <video> and that affects the underline caused by the parent <a> tag...)

(In reply to :Gijs Kruitbosch from comment #6) > I've tried setting inline-block on most > things in the hierarchy, and nothing helped. Yeah, you'd have to target something at the right point in the hierarchy (between the <video> and the controls). (Maybe there's nothing between the <video>/controls, though, so the controls are really the only thing you can target for this; not sure.) > Setting it on the > <videocontrols> themselves just crashes, actually... That seems undesirable. We may need to fix that in order to fix this bug. > The best I came up with is forcing <video>'s text-decoration to none, but > that doesn't fix inheritance from links or other things, it just happens to > help with this testcase. Right, that won't help if the <video> has parents which add additional layers of text-decoration. > dholbert, any idea why this is so difficult? Are we missing an obvious > solution? I think the only obvious solution is to shoehorn in a new level of block/inline-block between the <video> and its controls. (which you've already tried... we probably need to figure out the crash you mentioned.) (In reply to :Gijs Kruitbosch from comment #7) > (I also tried setting the text-decoration's color to transparent in the XUL, > but that also didn't help Right, that'd only style the text-decoration that you'd be adding at that level -- not the text-decorations added by the ancestors. > which makes no sense to me at all, because the > testcase sets it to white on the <video> and that affects the underline > caused by the parent <a> tag...) (No -- the underline from the parent <a> tag is white because the testcase has "a { color: white; }". If I disable that rule, then the underline from the <a> turns blue, as you might expect.)

(We could also conceivably add a special case for this at the platform level, as suggested in bug 786946 comment 6. Probably better if we can avoid that, though...)

Actually, RE "The best I came up with is forcing <video>'s text-decoration to none" -- that should work, *if* you *also* set <video> to be display:inline-block by default. That's what we do for buttons, at least. (per https://hg.mozilla.org/mozilla-central/rev/3a73e65c935e ) Does that work?

(Huh, local testing shows that the <a> underline still propagates through, even if the <video> is display:inline-block. I wonder why.)

(In reply to Daniel Holbert [:dholbert] from comment #11) > (Huh, local testing shows that the <a> underline still propagates through, > even if the <video> is display:inline-block. I wonder why.) I bet that changing the default display value for all <video> isn't web-compatible, though, if it isn't already display: inline-block... Though it's surprising that didn't help. I bet XUL isn't helpful here. :-( (In reply to Daniel Holbert [:dholbert] from comment #8) > (In reply to :Gijs Kruitbosch from comment #6) > > I've tried setting inline-block on most > > things in the hierarchy, and nothing helped. > > Yeah, you'd have to target something at the right point in the hierarchy > (between the <video> and the controls). (Maybe there's nothing between the > <video>/controls, though, so the controls are really the only thing you can > target for this; not sure.) > > > Setting it on the > > <videocontrols> themselves just crashes, actually... > > That seems undesirable. We may need to fix that in order to fix this bug. Can you reproduce this?

(In reply to :Gijs Kruitbosch from comment #12) > (In reply to Daniel Holbert [:dholbert] from comment #11) > > (Huh, local testing shows that the <a> underline still propagates through, > > even if the <video> is display:inline-block. I wonder why.) > > I bet that changing the default display value for all <video> isn't > web-compatible, though, if it isn't already display: inline-block... Though > it's surprising that didn't help. I bet XUL isn't helpful here. :-( Indeed, this is XUL's fault. It seems to be a bug in the way nsTextBoxFrame::DrawText walks up the tree looking for decorations. The similar non-XUL code in nsTextFrame::GetTextDecorations walks up the tree & bails when it hits the first non-"display:inline" thing. But nsTextBoxFrame::DrawText doesn't bail out like that -- it looks all the way up the tree, through any number of block layers, and adds *all* the text-decorations from *all* ancestors. That seems like a bug. > (In reply to Daniel Holbert [:dholbert] from comment #8) > > > Setting ["display:inline-block"] on the > > > <videocontrols> themselves just crashes, actually... > > > > That seems undesirable. We may need to fix that in order to fix this bug. > > Can you reproduce this? Haven't tried yet, though I'm not sure it actually matters after all, because of the XUL bug noted above.

(not sure it matters until we've fixed that XUL bug, I mean) And RE web-compat of changing the default "display" for <video> -- that's a good point, though note that we did manage to change the default "display" of <button> in Bug 731447 in the Firefox 15 timeframe (long after <buttons> were released). There may not be any rendering impact, since <video> isn't a container. I'll spin off a helper-bug for the XUL text-decoration propagation-through-blocks issue here...

I can reproduce the crash that you mentioned (setting "display: inline-block;" on .mediaControlsFrame). It's preceded by this assertion: ###!!! ASSERTION: A box layout method was called but InitBoxMetrics was never called: 'metrics', file layout/generic/nsFrame.cpp, line 8878 ...and we crash in nsFrame::BoxReflow() when we try to use BoxMetrics() [which wasn't initialized, per the assertion]. This happens because nsVideoFrame reflows its controls by calling "nsBoxFrame::LayoutChildAt", i.e. it does a XUL box reflow -- but the child doesn't *think* it's going to get a XUL box reflow, because IsBoxWrapped returns false (because neither the now-"display:inline-block" child nor the nsVideoFrame are XUL frames) And, I filed bug 1222096 for the XUL text-decoration over-propagation.

This patch (not actually intended to land) fixes IsBoxWrapped to consider a nsVideoFrame parent a "box", so that we could make the video controls 'display:inline-block' without crashing, per above. This breaks video layout a bit, though (unsurprisingly, since we use XUL for sizing/positioning the controls, I imagine). If we *really* wanted to make nsVideoFrame's controls a non-XUL thing, we'd really want to rip out nsVideoFrame's (XUL-flavored) LayoutChildAt call & the code around it, and replace it with non-XUL reflow code.

(In reply to Daniel Holbert [:dholbert] from comment #16) > If we *really* wanted to make nsVideoFrame's controls a non-XUL thing, we'd > really want to rip out nsVideoFrame's (XUL-flavored) LayoutChildAt call & > the code around it, and replace it with non-XUL reflow code. I would like to deXULify the videocontrols soonish -- we started back in bug 726240 and ran into some complications, but now that XUL's days are more clearly numbered... At the very least we should probably convert the easy pieces into HTML, and keep the XUL <progressmeter> / <scale> until we can do the harder work to suitably replace them. This bug (1218974) isn't urgent, so if moving the controls to more HTML would improve the world or simplify fixing it, we can make that happen.

Cool -- I think it's not worth investing too much in fixing this bug here, then. (And in particular, it's not worth trying to change the default display value of <video> to 'inline-block' [and take a potential web-compat risk], because once we've changed to HTML videocontrols, we can just use "display:inline-block" at a more granular & abstracted-away level like on the controls themselves.)

(In reply to Justin Dolske [:Dolske] from comment #17) > I would like to deXULify the videocontrols soonish Filed bug 1222273 on this btw.

I think I'm a little unclear on how bug 1222273, bug 1222096, and comment 18 intersect... Are you saying we should skip fixing bug 1222096 because converting the controls to HTML avoids the problem? If so, upon further reflection it won't be quite be that easy. One of the affected text labels is inside the XUL <scale> gunk that's a pain to port to HTML. So even if we HTMLized everything except that, we'd still be left with at least one prominent label affected by text-decoration.

(In reply to Justin Dolske [:Dolske] from comment #20) > I think I'm a little unclear on how bug 1222273, bug 1222096, and comment 18 > intersect... Are you saying we should skip fixing bug 1222096 because > converting the controls to HTML avoids the problem? Not that we should skip it, but that it'll mean we won't depend on fixing bug 1222096 here anymore. (If there's no XUL between <video> & its controls' text, then we don't need to worry about bug 1222096's weirdness with XUL text anymore here.) > If so, upon further reflection it won't be quite be that easy. One of the > affected text labels is inside the XUL <scale> gunk that's a pain to port to > HTML. So even if we HTMLized everything except that, we'd still be left with > at least one prominent label affected by text-decoration. Ah, ok. Yeah, my "HTML will fix this" spirit was predicated on the assumption that we'd be HTML-ifying all of the pieces that have text.