Bug 110913 – Mac: Incorrect rendering of <audio> controls

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Christian Biesinger]

As mentioned in https://bugs.webkit.org/show_bug.cgi?id=109775#c16

[Christian Biesinger]

Created attachment 190367 [details]
Patch

[Christian Biesinger]

This almost fixes it, but for really small <audio> elements the rendering is not correct. However, I don't understand why that is.

[Christian Biesinger]

Created attachment 190373 [details]
Patch

[Christian Biesinger]

So the remaining problem is this:
To completely fix this, the timeline <input> needs to also have min-width: 0;
However, just adding that doesn't work because flex box still reserves space for the "current time"/"time remaining" <div>s. Those divs are a special renderer that just does setWidth(0) to hide itself when the container becomes too small. That seems to confuse the flex box layout code for some reason...

[Christian Biesinger]

OK, turns out the solution is to make RenderMediaControlTimeDisplay also override min/maxPreferredWidth and return 0 if it is collapsed.

[Christian Biesinger]

Created attachment 190389 [details]
Patch

[Christian Biesinger]

Comment on attachment 190389 [details]
Patch

This should be all ready for review now.

[Tony Chang]

This is actually the perfect use case for visibility:collapse, but that's not implemented yet.

http://dev.w3.org/csswg/css3-flexbox/#visibility-collapse
https://bugs.webkit.org/show_bug.cgi?id=70782

This change seems OK to me, but Ojan may have a better idea about how to handle this without having to override minPreferredLogicalWidth/maxPreferredLogicalWidth.

[Christian Biesinger]

Ojan is OOO for the rest of the week :(

[WebKit Review Bot]

Comment on attachment 190389 [details]
Patch

Attachment 190389 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/16671100

New failing tests:
media/media-document-audio-repaint.html

[Ojan Vafai]

Comment on attachment 190389 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=190389&action=review

> Source/WebCore/rendering/RenderMediaControlElements.cpp:81
> +        m_collapsed = true;

I'm not sure visibility:collapse would actually fix the preferred width issue. I suppose if you set visibility:collapse *before* doing layout it would work actually. So, yeah, a FIXME to that effect would be nice.

> Source/WebCore/rendering/RenderMediaControlElements.cpp:90
> +LayoutUnit RenderMediaControlTimeDisplay::minPreferredLogicalWidth() const
> +{
> +    if (m_collapsed)
> +        return 0;
> +    return RenderFlexibleBox::minPreferredLogicalWidth();

Ugh. I really don't like this, but I don't see an alternative either. Also, it's theoretically incorrect in some cases (e.g. when you call minPreferredLogicalWidth before doing a layout).

Does the following work?
-Instead of calling setWidth, *before* the RenderFlexibleBox::layout call, clear any overrideLogicalWidth and then set the *override* logical width to 0 (if the container width is to small that is). Then call RenderFlexbileBox::layout. Then you shouldn't need to override min/maxPreferred.

> Source/WebCore/rendering/RenderMediaControlElements.h:54
> +    virtual LayoutUnit minPreferredLogicalWidth() const;
> +    virtual LayoutUnit maxPreferredLogicalWidth() const;

You should be able to just override computeIntrinsicLogicalWidths instead of these two. I think that would be better.

> LayoutTests/ChangeLog:16
> +        Regenerate test results.

FWIW, in the future, no need to add comments like this. This doesn't actually explain anything (e.g. why the new results are correct).

[Christian Biesinger]

Is it a good idea to set visibility:collapse from inside a renderer, during layout...?


Setting the override logical content width didn't help... should RenderFlexibleBox::mainAxisExtentForChild take into account the override width, like RenderDeprecatedFlexibleBox did? (as in contentWidthForChild)

[Christian Biesinger]

Overriding computeIntrinsicLogicalWidths didn't work either

(as a sidenote, I don't understand why this class inherits from RenderFlexibleBox, as opposed to RenderBlock)

[Christian Biesinger]

Created attachment 190586 [details]
Patch

[Christian Biesinger]

In this patch, instead of overriding the functions, I just set m_min|maxPreferredLogicalWidth to zero in layout().

[WebKit Review Bot]

Comment on attachment 190586 [details]
Patch

Attachment 190586 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/16672484

New failing tests:
media/media-document-audio-repaint.html

[Tony Chang]

Comment on attachment 190586 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=190586&action=review

> Source/WebCore/ChangeLog:29
> +        Override minPreferredLogicalWidth and maxPreferredLogicalWidth to return 0 if we're collapsed.
> +        Add a m_collapsed member to indicate if we are collapsed.

This text is out of date.

> Source/WebCore/rendering/RenderMediaControlElements.cpp:80
>          setWidth(0);
> +        m_minPreferredLogicalWidth = m_maxPreferredLogicalWidth = 0;

This looks better.

[Christian Biesinger]

(In reply to comment #16)
> New failing tests:
> media/media-document-audio-repaint.html

I can't reproduce this failure locally....

I'll update the changelog.

[Christian Biesinger]

Created attachment 190611 [details]
Patch

[WebKit Review Bot]

Comment on attachment 190611 [details]
Patch

Attachment 190611 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/16829158

New failing tests:
media/media-document-audio-repaint.html

[Ojan Vafai]

(In reply to comment #13)
> Overriding computeIntrinsicLogicalWidths didn't work either
> 
> (as a sidenote, I don't understand why this class inherits from RenderFlexibleBox, as opposed to RenderBlock)

I think it's just for centering. A perfect example of why we need to implement the alignment APIs for other display types. It's silly to use flexbox just to get vertical centering.

(In reply to comment #12)
> Is it a good idea to set visibility:collapse from inside a renderer, during layout...?
> 
> 
> Setting the override logical content width didn't help... should RenderFlexibleBox::mainAxisExtentForChild take into account the override width, like RenderDeprecatedFlexibleBox did? (as in contentWidthForChild)

Yes, I think that's right. It's a little gross to spread override sizes to other parts of the code, but I think that's better (and more correct) than messing with preferred sizes during layout.

I'm OK with this patch going in as is so we can get the regression fixed, but I'd like to see the override thing done if possible.

[Ojan Vafai]

Also, looks like there's a Chromium linux failure. I'm sure it just needs a rebaseline, but maybe add it to TestExpectations so the tree doesn't turn red when you commit this?

[Build Bot]

Comment on attachment 190611 [details]
Patch

Attachment 190611 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/16782414

New failing tests:
media/nodesFromRect-shadowContent.html

[Build Bot]

Comment on attachment 190611 [details]
Patch

Attachment 190611 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/16824066

New failing tests:
media/nodesFromRect-shadowContent.html

[Ojan Vafai]

(In reply to comment #24)
> (From update of attachment 190611 [details])
> Attachment 190611 [details] did not pass mac-ews (mac):
> Output: http://webkit-commit-queue.appspot.com/results/16824066
> 
> New failing tests:
> media/nodesFromRect-shadowContent.html

Looks like there's a Mac failure as well. Again, probably just needs rebaseline?

[Christian Biesinger]

So, on the test failures.

Mac: I can reproduce it locally, but only sometimes. I have no idea why this test failure it happens - nodesFromRect sometimes returns an additional node? When I tried to find out which node, I was unable to reproduce this failure again.
(Note: not a render tree test, so rebaselining is not an issue for this one)

Linux/Chrome: This failure I can usually reproduce with run-webkit-test, but not when I run in the debugger or in the actual browser. When it happens, it does have actual wrong layout though.

I guess I mark the tests as flaky and hope the problems go away when I make the switch to use override widths...?

[Christian Biesinger]

Created attachment 190798 [details]
Patch

[Christian Biesinger]

Created attachment 190799 [details]
Patch

[Tony Chang]

(In reply to comment #26)
> Linux/Chrome: This failure I can usually reproduce with run-webkit-test, but not when I run in the debugger or in the actual browser. When it happens, it does have actual wrong layout though.

Does the failure happen if you run "DumpRenderTree /path/to/LayoutTests/media/media-document-audio-repaint.html"?  If so,  you can run gdb against DumpRenderTree directly (make sure to pass --no-timeout).

[Christian Biesinger]

(In reply to comment #29)
> (In reply to comment #26)
> > Linux/Chrome: This failure I can usually reproduce with run-webkit-test, but not when I run in the debugger or in the actual browser. When it happens, it does have actual wrong layout though.
> 
> Does the failure happen if you run "DumpRenderTree /path/to/LayoutTests/media/media-document-audio-repaint.html"?  If so,  you can run gdb against DumpRenderTree directly (make sure to pass --no-timeout).

I did try "gdb DumpRenderTree /path/to/LayoutTests/media/media-document-audio-repaint.html" which didn't reproduce the problem. I did not try running DRT directly without gdb, maybe that's worth a try...

[Christian Biesinger]

Comment on attachment 190799 [details]
Patch

After spending some time looking at this with Ojan, the fix seems to be to specify min-width: 0 in the CSS instead of changing the C++ code. That works because then, RenderFlexibleBox will not call maxPreferredContentWidth, so everything should be fine. I'll test that and upload a patch with that.

[Christian Biesinger]

It does fix the bug. But nodesFromRect-shadow.html is now consistently failing - it looks like hit testing finds a shadow input element even when passing false for shadow content?!

Also, it doesn't find the timeline container element in the shadow content, which is a little weird too...

I guess I'll attach what I have

[Christian Biesinger]

Created attachment 191634 [details]
Patch

[Build Bot]

Comment on attachment 191634 [details]
Patch

Attachment 191634 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17082130

New failing tests:
media/nodesFromRect-shadowContent.html

[Build Bot]

Comment on attachment 191634 [details]
Patch

Attachment 191634 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17062089

New failing tests:
media/nodesFromRect-shadowContent.html
editing/selection/selection-modify-crash.html

[Christian Biesinger]

hm... for nodesFromRect-shadowContent.html, there's two levels of shadow content. The <video> has an <input> in its shadow content, and then the <input> has additional shadow content. The hit test finds something in the <input>'s content, and we add its deprecatedShadowAncestorNode() to the result. Sadly, that node is the <input>, but it itself is shadow content.

But I don't know how this is supposed to work...

[Christian Biesinger]

Dimitri: see previous comment - is the code in HitTestResult::addNodeToRectBasedTestResult correct when there's nested shadow content? Should this line be a loop?
560         node = node->deprecatedShadowAncestorNode();

[Dimitri Glazkov (Google)]

(In reply to comment #37)
> Dimitri: see previous comment - is the code in HitTestResult::addNodeToRectBasedTestResult correct when there's nested shadow content? Should this line be a loop?
> 560         node = node->deprecatedShadowAncestorNode();

That shouldn't matter, not in this case. The whole optional skipping of the shadow content for hit testing logic was added a while back in bug 80847, and I still don't understand why it's necessary.

[Eric Carlson]

<rdar://problem/13292684>

[Christian Biesinger]

(In reply to comment #38)
> (In reply to comment #37)
> > Dimitri: see previous comment - is the code in HitTestResult::addNodeToRectBasedTestResult correct when there's nested shadow content? Should this line be a loop?
> > 560         node = node->deprecatedShadowAncestorNode();
> 
> That shouldn't matter, not in this case. The whole optional skipping of the shadow content for hit testing logic was added a while back in bug 80847, and I still don't understand why it's necessary.

The problem in this case is that we don't skip shadow content when we should... nodesFromRect is returning shadow content even when allowShadowContent=false.

And it seems like the proximate cause of this is a nested shadow tree

[Dimitri Glazkov (Google)]

(In reply to comment #40)
> (In reply to comment #38)
> > (In reply to comment #37)
> > > Dimitri: see previous comment - is the code in HitTestResult::addNodeToRectBasedTestResult correct when there's nested shadow content? Should this line be a loop?
> > > 560         node = node->deprecatedShadowAncestorNode();
> > 
> > That shouldn't matter, not in this case. The whole optional skipping of the shadow content for hit testing logic was added a while back in bug 80847, and I still don't understand why it's necessary.
> 
> The problem in this case is that we don't skip shadow content when we should... nodesFromRect is returning shadow content even when allowShadowContent=false.
> 
> And it seems like the proximate cause of this is a nested shadow tree

Got it. Yes, the node->deprecatedShadowAncestorNode() is the wrong way to get to the first non-shadow tree node. The proper way would be to iterate through nested tree scopes until you hit Document, and then return the host of the previous tree scope.

I don't think it's a big deal though. This API isn't exposed to the Web and is only used for testing.

[Christian Biesinger]

Hm... OK. I guess I'll figure out a way to fix this failing unit test without touching HitTestResult (the caller of deprecatedShadowAncestorNode in this case)

[Christian Biesinger]

Although... why wouldn't this also be an issue with document.elementFromPoint? We expose that to web content today.

[Christian Biesinger]

dglazkov explained on irc. My strategy now is to fix the shadow bug separately and then commit this.

[Christian Biesinger]

bug 112068 will fix the shadow content bug, once that's committed the test here should pass.

[Christian Biesinger]

Created attachment 192571 [details]
Patch

Alternatively, this version of the patch changes the testcase such that it passes even without that other patch. Attaching in hopes of getting this regression fixed sooner.

[Christian Biesinger]

Created attachment 192612 [details]
Patch

Dependency got fixed, this should pass all tests now.

[Build Bot]

Comment on attachment 192612 [details]
Patch

Attachment 192612 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17060709

New failing tests:
editing/selection/selection-modify-crash.html

[Christian Biesinger]

Comment on attachment 192612 [details]
Patch

The test passes locally for me, and also passed on mac-wk2. It also doesn't use <video> or <audio>. Going to assume it is unrelated.

[WebKit Review Bot]

Comment on attachment 192612 [details]
Patch

Clearing flags on attachment: 192612

Committed r145588: <http://trac.webkit.org/changeset/145588>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Jer Noble]

This commit broke the "current time" and "remaining time" display in mac inline controls.  The new LayoutTests/platform/mac/media/audio-controls-rendering-expected.txt results are giving false-pass, as those two controls were incorrectly removed from the expected results.

If this does not get fixed soon, I'm going to have to roll this entire patch back out.

[Christian Biesinger]

I give up. This is really tricky to fix and will need more time.

Jer, I don't have commit access - could you roll out this patch and bug 109775's patch while I figure out a better fix locally?

[Jer Noble]

(In reply to comment #53)
> I give up. This is really tricky to fix and will need more time.
> 
> Jer, I don't have commit access - could you roll out this patch and bug 109775's patch while I figure out a better fix locally?

FYI, you can get the sherrifbot to roll out any patch by asking in IRC (see: http://trac.webkit.org/wiki/Keeping%20the%20Tree%20Green).  Anyone can cq+ the resulting patch.  I'll start the ball rolling (hah!).

[Jer Noble]

Turns out rolling out these patches is more difficult than just fixing the bug.  Filed bug 113355 to track the fix.

[Christian Biesinger]

fixed in the bug mentioned in the last comment