1322512 - <audio> with vertical writing mode doesn't render anything visible

Status: VERIFIED FIXED

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

Description

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

STR:
 1. Load attached testcase.

EXPECTED RESULTS:
Audio controls bar should be visible.

ACTUAL RESULTS:
No audio controls are visible.

I think this must've regressed as part of bug 1271765.  It works correctly (albeit with some rotated text on the controls) in Firefox release (version 50).

Comment 1

[Daniel Holbert [:dholbert]]

Requesting tracking, because this is a functional regression in Firefox 53.  We should fix this before bug 1271765 ships to release users.

Comment 2

[Marcia Knous [:marcia]]

Tracking 53+ for this regressions for all the reasons in Comment 1.

Comment 3

[roxleitan]

Last good revision: 46127b3a981bceb0413c8199849f4e47afc949da
First bad revision: 6186126f502ba47e4fb2b6f4d971ea6fd3e66a02

Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=46127b3a981bceb0413c8199849f4e47afc949da&tochange=6186126f502ba47e4fb2b6f4d971ea6fd3e66a02

Comment 4

[Xidorn Quan [:xidorn] UTC+11]

The rendering in release version isn't very sensible either, though.

Comment 5

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Ray, could you drive this to a fix? I actually wonder why |writing-mode| property can be inherit into the anno videocontrols scope... other properties don't inherit, no?

Comment 6

[Xidorn Quan [:xidorn] UTC+11]

I think it is reasonable to make |writing-mode| inherit into whatever controls. UI should be aware of the writing mode they are supposed to use. I think the direction property is also inherited.

Comment 7

[Ray Lin[:ralin]]

Thank you :xidorn, I noticed |writing-mode| and |direction| are inherited as you thought.

> Ray, could you drive this to a fix? I actually wonder why |writing-mode|
> property can be inherit into the anno videocontrols scope... other
> properties don't inherit, no?

Yes, it presents that the shrink wrapping container doesn't get the right size even I've already set a horizontal writing mode into <videocontrol> which should not override by outer scope. I'm looking into this issue now, and will test out the possible writing mode issues to fix it.

Comment 8

[Ray Lin[:ralin]]

Apologize in advance for my understanding in layout might cause some nonsense idea. 

I could understand that `writing-mode` should be inherited from parents, but I wonder why we should care about `writing-mode` in <audio> control? as original control won't rotate according to different `writing-mode`. I think that the original <audio> working fine in vertical mode is because we set a certain nsSize to it regardless of writing mode. Does it make sense to abandon `writing-mode` in reflow code especially for <videocontrols>? 

Also, I've tried to print out the availableSize in line:368[0] but the value looks irrational, so I guess <videocontrol> could not be expanded correctly until we pass reasonable availableSize into reflow code. Could you give me a direction to fix it?

Thank you so much :)


[0] https://dxr.mozilla.org/mozilla-central/rev/567894f026558e6dada617a3998f29aed06ac7d8/layout/generic/nsVideoFrame.cpp#368

Comment 9

[Daniel Holbert [:dholbert]]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #4)
> The rendering in release version isn't very sensible either, though.

It wasn't perfect, but it was usable.

Having said that, the issue is specific to <audio> (not <video>) in a nonstandard-writing-mode setting.  So, pretty edge-casey, and it's probably unlikely that much real world content actually hits it.

Comment 10

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 (comparing intrinsically-sized vs. specified-size <audio> element)

This also only seems broken in intrinsically-sized audio elements. If there's a specified size, the controls show up, as demonstrated here.

(Unlike in testcase 1, I'm referencing an actual Wikipedia-hosted OGG audio file in this testcase, too, to test that the controls actually work when they show up. [They do].)

Comment 11

[Daniel Holbert [:dholbert]]

(In reply to Ray Lin[:ralin] from comment #8)
> I could understand that `writing-mode` should be inherited from parents, but
> I wonder why we should care about `writing-mode` in <audio> control? as
> original control won't rotate according to different `writing-mode`.

We don't need to rotate it to respect the writing mode (i.e. we don't need to make the scrubber flow vertically instead of horizontally or anything like that).  We just need to be sure we render *something*.

> Does it make sense to
> abandon `writing-mode` in reflow code especially for <videocontrols>?

We already basically do that (override any inherited writing-mode in the controls), via the hardcoded "writing-mode: horizontal-tb" that I suggested over in bug 1271765 comment 110, which you added here:
https://dxr.mozilla.org/mozilla-central/rev/567894f026558e6dada617a3998f29aed06ac7d8/toolkit/themes/shared/media/videocontrols.css#8-10

So that's good. Yet, we're still rendering blankness when we should be rendering controls, in testcase 1 here, for some reason.

> Also, I've tried to print out the availableSize in line:368[0] but the value
> looks irrational, so I guess <videocontrol> could not be expanded correctly
> until we pass reasonable availableSize into reflow code. Could you give me a
> direction to fix it?

I'll take a look around in gdb and see what I find out...

Comment 12

[Daniel Holbert [:dholbert]]

So the problem happens when...
 (a) <audio> and its controls have orthogonal Writing Modes
 (b) the <audio> is intrinsically sized.

What happens is:
 - nsHTMLReflowState tries to compute the intrinsic inline-size of the <audio> frame (as part of prepping for reflow).
 - Under the hood, this triggers a call to nsVideoFrame::ComputeSize
 - Several stack-layers inside of that, we call nsLayoutUtils::IntrinsicForAxis(eAxisVertical, ...) on the controls frame. (eAxisVeritcal because that's the inline axis for the <audio> element -- though note that it's the block axis for the controls frame! This is key.)
 - Inside of IntrinsicForAxis, we detect that we're being asked for the *intrinsic block-size* of the controls frame, and it does not have an intrinsic block-size.  So, we throw up our hands and just return the frame's current BSize() (height), since we don't have anything better we can do.  That happens here:
  https://dxr.mozilla.org/mozilla-central/rev/f179934df0c1bab590c558485d419c7910e41325/layout/base/nsLayoutUtils.cpp#5053
Since the controls haven't been reflowed yet, that BSize() is just 0.
 - SO, we end up thinking the controls have an intrinsic height of 0, which means the <audio> frame decides its intrinsic inline-size (height) is also 0.
 - Hence, it's invisible.

SO. How to fix this? One obvious solution is just to force <audio> elements to have "writing-mode: horizontal-tb" with a UA stylesheet rule (just like we do for their controls right now).  That's slightly problematic, though, because it's web-observable.

Alternately, we could just add a special case to nsVideoFrame::ComputeSize() **in the case where we have <audio> and its controls are orthogonal to the audio element**, and return some hardcoded size. That's slightly problematic because it's a cop-out. (However, we need *some sort of cop-out* if we're going to keep the controls auto-sized with a mandated horizontal writing-mode, inside of a auto-sized potentially-orthogonal <audio> control. Orthogonal writing-modes & auto-sizing just don't really play well together, I think.)

Comment 13

[Daniel Holbert [:dholbert]]

Attachment: backtrace of where we decide the intrinsic height is 0

For the record, here's a GDB backtrace of the codepath that ends up deciding on an intrinsic height of 0, btw (which is how we end up rendering nothing visible here) -- to the line of nsLayoutUtils.cpp mentioned/linked above.

Specifically, the nsLayoutUtils code that we hit is:
>      if (intrinsicBCoord.GetUnit() == eStyleUnit_Coord) {
>        result = intrinsicBCoord.GetCoordValue();
>      } else {
>[...]
>        // XXX Unfortunately, we probably don't know this yet, so this is wrong...
>        // but it's not clear what we should do. If aFrame's inline size hasn't
>        // been determined yet, we can't necessarily figure out its block size
>        // either. For now, authors who put orthogonal elements into things like
>        // buttons or table cells may have to explicitly provide sizes rather
>        // than expecting intrinsic sizing to work "perfectly" in underspecified
>        // cases.
>        result = aFrame->BSize();
>      }

Comment 14

[Ray Lin[:ralin]]

Attachment: Bug 1322512 - force <audio controls> to have horizontal writing-mode, so that its control bar is never an orthogonal flow.

Review commit: https://reviewboard.mozilla.org/r/100748/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/100748/

Comment 15

[Daniel Holbert [:dholbert]]

Comment on attachment 8821458 [details]
Bug 1322512 - force <audio controls> to have horizontal writing-mode, so that its control bar is never an orthogonal flow.

https://reviewboard.mozilla.org/r/100748/#review101222

Thanks for stepping up and posting a patch!

As I noted above, this solution is a bit unfortunate since it's observable -- via getComputedStyle(audioElem, "").writingMode only ever returning one thing ("horizontal-tb") from now on, regardless of author styles.  Having said that, it's also a nice targeted fix which is preferable to our current Nightly behavior (sizing these elements to 0 in this case), and it's unlikely that any web content cares about the computed "writing-mode" of audio elements... So it's probably not too bad. 

Three requests:
 (1) Could you file a followup on coming up with a better fix? I'm uneasy leaving this fix in the tree forever, so I'd like to at least track the fact that we should replace it with something better (something that won't break getComputedStyle).

 (2) Commit message wording-nit:
> Bug 1322512 - force <audio> has horizontal writing mode to get rid of being orthogonal to control bar. r=dholbert

Let's reword this as:
Bug 1322512 - force <audio controls> to have horizontal writing-mode, so that its control bar is never an orthogonal flow.

 (3) Please add a reftest -- e.g. with the testcase having <audio controls style="writing-mode: vertical-lr"> and the reference case just having <audio controls>.

::: layout/style/res/html.css:759
(Diff revision 1)
> +audio {
> +  writing-mode: horizontal-tb !important;

Two nits here:
(1) Let's make the selector here "audio [controls]", since that's the only time that it matters. (And then authors who want to provide their own custom controls aren't impacted by this.)

(2) Please add a code-comment to explain why we're doing this -- e.g. "This ensures that intrinsic sizing can reliably shrinkwrap our controls (which are also always horizontal) and produce a reasonable intrinsic size from them."

Comment 16

[Ray Lin[:ralin]]

Comment on attachment 8821458 [details]
Bug 1322512 - force <audio controls> to have horizontal writing-mode, so that its control bar is never an orthogonal flow.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100748/diff/1-2/

Comment 17

[Ray Lin[:ralin]]

Comment on attachment 8821458 [details]
Bug 1322512 - force <audio controls> to have horizontal writing-mode, so that its control bar is never an orthogonal flow.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100748/diff/2-3/

Comment 18

[Ray Lin[:ralin]]

Comment on attachment 8821458 [details]
Bug 1322512 - force <audio controls> to have horizontal writing-mode, so that its control bar is never an orthogonal flow.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100748/diff/3-4/

Comment 19

[Ray Lin[:ralin]]

Comment on attachment 8821458 [details]
Bug 1322512 - force <audio controls> to have horizontal writing-mode, so that its control bar is never an orthogonal flow.

https://reviewboard.mozilla.org/r/100748/#review101222

So appreciate all information you gave :) and thanks for guiding us look under the hood how this happened. 

nit fixed and filed a followup Bug 1325899, could you help to review again? Thanks

Comment 20

[Daniel Holbert [:dholbert]]

Comment on attachment 8821458 [details]
Bug 1322512 - force <audio controls> to have horizontal writing-mode, so that its control bar is never an orthogonal flow.

https://reviewboard.mozilla.org/r/100748/#review101948

Sorry for taking a little while to get to this -- I've been away, and also hoping to myself that I could come up with an approximately-equally-simple & less-hacky alternative solution.  But I don't have an alternative right now, so we should probably proceed with this for the time being.

Two nits:
 (1) Right now, the commit message is wrapped to 2 lines at an arbitrary point. Please unwrap it to 1 line.  (Everything from "Bug NNN" up to "r=whoever" should be on the first line of the commit message, even if it's longer than 80 chars, because version-control tools give special significance to the first line as being the "summary".)

 (2) The reftests don't really belong in "toolkit/content/tests/reftests/", since this bug doesn't have anything to do with toolkit stuff.  It's a layout bug (and its fix is in the layout/ directory), so the reftest belongs in the main layout/reftests directory-tree. Probably simplest to just put the reftest files at layout/reftests/bugs/1322512-1.html and layout/reftests/bugs/1322512-1-ref.html.

(layout/reftests/bugs is a "grab bag" directory -- ideally we'd put this at a more specific directory like layout/reftests/audio, but no such directory seems to exist right now. It seems we seem to have very few reftests for <audio>! I'm glad we're adding this one at least. :))

Please fix both of those nits, and then I'll grant r+. Thanks!

Comment 21

[Ray Lin[:ralin]]

Comment on attachment 8821458 [details]
Bug 1322512 - force <audio controls> to have horizontal writing-mode, so that its control bar is never an orthogonal flow.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/100748/diff/4-5/

Comment 22

[Ray Lin[:ralin]]

Comment on attachment 8821458 [details]
Bug 1322512 - force <audio controls> to have horizontal writing-mode, so that its control bar is never an orthogonal flow.

https://reviewboard.mozilla.org/r/100748/#review101948

I moved the reftest to layout directroy, and fixed commit message. (I didn't notice that vim would auto-wrap even using :wq)

Learned a lot from this bug, thanks for your time :-)

Comment 23

[Daniel Holbert [:dholbert]]

Comment on attachment 8821458 [details]
Bug 1322512 - force <audio controls> to have horizontal writing-mode, so that its control bar is never an orthogonal flow.

https://reviewboard.mozilla.org/r/100748/#review102050

Thanks! r=me

Comment 24

[Daniel Holbert [:dholbert]]

Looks like this had a successful Try run, so I'll go ahead and autoland this.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/56915773d15a
force <audio controls> to have horizontal writing-mode, so that its control bar is never an orthogonal flow. r=dholbert

Comment 26

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/56915773d15a

Comment 27

[Hani Yacoub, Desktop QA]

Tested this issue on Windows 10 x64, Ubuntu 16.04 x64 and Mac OS X 10.11 on Firefox Nightly 53.0a1 and I confirm that it's not reproducible anymore.