1366163 - stylo: rendering of <video controls> and <audio controls> is wrong

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P2)

Description

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: incomplete_rendering_of_video_controls_.png

With the WIP patches in bug 1290276 comment 22, I got an incomplete rendering of <video controls>. (See the attachment.)

At least "-moz-context-properties" is not supported yet (Bug 1366162). I'm not sure what else is missing.

Comment 1

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: stylo: Fix pseudo element matching for rules in XBL stylesheets (bug 1366163)

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

Comment 2

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

After bug 1372876 is fixed, the remaining issue for the <video controls> is the :hover rule in [1] for the -moz-range-thumb [2].

.scrubber:hover::-moz-range-thumb,
.volumeControl:hover::-moz-range-thumb {
  background-color: #48a0f7;
}

[1] http://searchfox.org/mozilla-central/rev/fdb811340ac4e6b93703d72ee99217f3b1d250ac/toolkit/themes/shared/media/videocontrols.css#234-237
[2] https://developer.mozilla.org/en-US/docs/Web/CSS/::-moz-range-thumb

Comment 3

[Cameron McCormack (:heycam)]

Should that remaining issue be fixed by bug 1375969?

Comment 4

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Yes. Based on my local build, the issue in comment 2 has been fixed by bug 1375969. <video controls> looks the same on gecko and stylo now. Thanks emilio!

Comment 5

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

We still have some reftests failed like layout/reftests/bugs/449149-1a.html for <audio controls>.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=48484b37866c8674e8066a2e95826bb0653d0999&selectedJob=111173618

Comment 6

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Gecko and stylo generate different frame subtree for "positionDurationBox" [1] in "data:text/html,<audio controls>". See https://pastebin.mozilla.org/9028852

stylo generated a "-moz-anonymous-flex-item" block with many newline-only text frame in it, but gecko doesn't. I haven't figured out why.

[1] http://searchfox.org/mozilla-central/rev/30a47c4339bd397b937abdb2305f99d3bb537ba6/toolkit/content/widgets/videocontrols.xml#167-169

Comment 7

[Emilio Cobos Álvarez [:emilio]]

Whitespace-only flex items are a bug, per spec, so this is a stylo issue.

I suspect that we're messing up the TextIsOnlyWhitespace cached state, somehow. That path is different for stylo and non-stylo.

Comment 8

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

The difference of the frame tree described in comment 6 might be because servo doesn't blockify the children if the parent's display is "flex" where gecko has the fixup implemented in [1].

Emilio, do you have any idea how to fix that in servo?

[1] http://searchfox.org/mozilla-central/source/layout/style/GeckoStyleContext.cpp#936-942,958-977

Comment 9

[Emilio Cobos Álvarez [:emilio]]

I'm pretty sure servo does this in https://github.com/servo/servo/blob/master/components/style/style_adjuster.rs#L53.

Comment 10

[Emilio Cobos Álvarez [:emilio]]

That being said, it does indeed seem like we're skipping it incorrectly, I can take a look at it if you want.

Comment 11

[Emilio Cobos Álvarez [:emilio]]

I think the check in GeckoElement::skip_root_and_item_based_display_fixup is just incorrect...

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8dbe9482d80008a29606f68c34d70fa07ccd1a5d

Comment 12

[Emilio Cobos Álvarez [:emilio]]

Ok, that seemed to work :)

Thanks a lot for finding that Ting-Yu!

Comment 13

[Emilio Cobos Álvarez [:emilio]]

I'm going to go ahead and submit that patch since I've heard Ting-Yu is on North Korea, so won't probably be available in a bit.

Comment 14

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1366163: Fix skip_root_and_item_based_display_fixup.

The check isn't doing what the comment is doing.

XBL subtrees for video controls all have the
NODE_IS_IN_NATIVE_ANONYMOUS_SUBTREE, but not all of them are NAC themselves, so
we may incorrectly miss blockification.

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

Comment 15

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1366163: Update test expectations.

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

Comment 16

[Cameron McCormack (:heycam)]

Comment on attachment 8894663 [details]
Bug 1366163: Fix skip_root_and_item_based_display_fixup.

https://reviewboard.mozilla.org/r/165816/#review171082

Oops, thanks for fixing. :-)

Comment 17

[Cameron McCormack (:heycam)]

Comment on attachment 8894664 [details]
Bug 1366163: Update test expectations.

https://reviewboard.mozilla.org/r/165818/#review171084

Comment 18

[Emilio Cobos Álvarez [:emilio]]

https://hg.mozilla.org/integration/autoland/rev/f55869ce0fe5

Comment 19

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8894664 [details]
Bug 1366163: Update test expectations.

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

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/40004a53d1d1
Update test expectations. r=heycam

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/40004a53d1d1

Comment 22

[Aki Sasaki (not active)]

https://hg.mozilla.org/projects/date/rev/40004a53d1d1a28b30f565ffc1329b1f28dfffd2
Bug 1366163: Update test expectations. r=heycam