Bug 110913

Summary: Mac: Incorrect rendering of <audio> controls
Product: WebKit Reporter: Christian Biesinger <cbiesinger>
Component: MediaAssignee: Christian Biesinger <cbiesinger>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, dglazkov, dino, eric.carlson, eric, esprehn+autocc, feature-media-reviews, jer.noble, macpherson, menard, ojan.autocc, ojan, rniwa, simon.fraser, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 111790, 112068    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Christian Biesinger 2013-02-26 14:55:01 PST
As mentioned in https://bugs.webkit.org/show_bug.cgi?id=109775#c16
Comment 1 Christian Biesinger 2013-02-26 14:57:53 PST
Created attachment 190367 [details]
Patch
Comment 2 Christian Biesinger 2013-02-26 14:58:35 PST
This almost fixes it, but for really small <audio> elements the rendering is not correct. However, I don't understand why that is.
Comment 3 Christian Biesinger 2013-02-26 15:23:02 PST
Created attachment 190373 [details]
Patch
Comment 4 Christian Biesinger 2013-02-26 16:12:10 PST
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...
Comment 5 Christian Biesinger 2013-02-26 16:26:12 PST
OK, turns out the solution is to make RenderMediaControlTimeDisplay also override min/maxPreferredWidth and return 0 if it is collapsed.
Comment 6 Christian Biesinger 2013-02-26 16:35:15 PST
Created attachment 190389 [details]
Patch
Comment 7 Christian Biesinger 2013-02-26 16:35:51 PST
Comment on attachment 190389 [details]
Patch

This should be all ready for review now.
Comment 8 Tony Chang 2013-02-26 16:50:40 PST
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.
Comment 9 Christian Biesinger 2013-02-26 16:52:48 PST
Ojan is OOO for the rest of the week :(
Comment 10 WebKit Review Bot 2013-02-26 18:15:17 PST
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
Comment 11 Ojan Vafai 2013-02-26 21:12:46 PST
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).
Comment 12 Christian Biesinger 2013-02-27 12:45:26 PST
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)
Comment 13 Christian Biesinger 2013-02-27 13:04:55 PST
Overriding computeIntrinsicLogicalWidths didn't work either

(as a sidenote, I don't understand why this class inherits from RenderFlexibleBox, as opposed to RenderBlock)
Comment 14 Christian Biesinger 2013-02-27 13:17:59 PST
Created attachment 190586 [details]
Patch
Comment 15 Christian Biesinger 2013-02-27 13:20:02 PST
In this patch, instead of overriding the functions, I just set m_min|maxPreferredLogicalWidth to zero in layout().
Comment 16 WebKit Review Bot 2013-02-27 14:05:25 PST
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
Comment 17 Tony Chang 2013-02-27 14:26:13 PST
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.
Comment 18 Christian Biesinger 2013-02-27 14:59:56 PST
(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.
Comment 19 Christian Biesinger 2013-02-27 15:02:38 PST
Created attachment 190611 [details]
Patch
Comment 20 WebKit Review Bot 2013-02-27 16:55:28 PST
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
Comment 21 Ojan Vafai 2013-02-27 23:35:17 PST
(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.
Comment 22 Ojan Vafai 2013-02-27 23:36:06 PST
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?
Comment 23 Build Bot 2013-02-28 08:34:01 PST
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
Comment 24 Build Bot 2013-02-28 08:55:59 PST
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
Comment 25 Ojan Vafai 2013-02-28 10:32:31 PST
(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?
Comment 26 Christian Biesinger 2013-02-28 13:26:35 PST
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...?
Comment 27 Christian Biesinger 2013-02-28 13:31:58 PST
Created attachment 190798 [details]
Patch
Comment 28 Christian Biesinger 2013-02-28 13:43:44 PST
Created attachment 190799 [details]
Patch
Comment 29 Tony Chang 2013-03-01 08:30:50 PST
(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).
Comment 30 Christian Biesinger 2013-03-01 08:32:22 PST
(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...
Comment 31 Christian Biesinger 2013-03-05 17:46:59 PST
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.
Comment 32 Christian Biesinger 2013-03-05 18:42:57 PST
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
Comment 33 Christian Biesinger 2013-03-05 18:47:14 PST
Created attachment 191634 [details]
Patch
Comment 34 Build Bot 2013-03-06 00:15:59 PST
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
Comment 35 Build Bot 2013-03-06 13:25:11 PST
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
Comment 36 Christian Biesinger 2013-03-08 13:56:35 PST
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...
Comment 37 Christian Biesinger 2013-03-08 17:19:08 PST
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();
Comment 38 Dimitri Glazkov (Google) 2013-03-09 08:54:48 PST
(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.
Comment 39 Eric Carlson 2013-03-11 11:15:21 PDT
<rdar://problem/13292684>
Comment 40 Christian Biesinger 2013-03-11 12:18:38 PDT
(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
Comment 41 Dimitri Glazkov (Google) 2013-03-11 13:17:33 PDT
(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.
Comment 42 Christian Biesinger 2013-03-11 13:26:22 PDT
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)
Comment 43 Christian Biesinger 2013-03-11 13:37:10 PDT
Although... why wouldn't this also be an issue with document.elementFromPoint? We expose that to web content today.
Comment 44 Christian Biesinger 2013-03-11 14:00:36 PDT
dglazkov explained on irc. My strategy now is to fix the shadow bug separately and then commit this.
Comment 45 Christian Biesinger 2013-03-11 14:41:00 PDT
bug 112068 will fix the shadow content bug, once that's committed the test here should pass.
Comment 46 Christian Biesinger 2013-03-11 15:11:57 PDT
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.
Comment 47 Christian Biesinger 2013-03-11 18:14:06 PDT
Created attachment 192612 [details]
Patch

Dependency got fixed, this should pass all tests now.
Comment 48 Build Bot 2013-03-12 07:50:07 PDT
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
Comment 49 Christian Biesinger 2013-03-12 14:05:08 PDT
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.
Comment 50 WebKit Review Bot 2013-03-12 14:55:32 PDT
Comment on attachment 192612 [details]
Patch

Clearing flags on attachment: 192612

Committed r145588: <http://trac.webkit.org/changeset/145588>
Comment 51 WebKit Review Bot 2013-03-12 14:55:38 PDT
All reviewed patches have been landed.  Closing bug.
Comment 52 Jer Noble 2013-03-25 14:38:46 PDT
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.
Comment 53 Christian Biesinger 2013-03-25 17:35:38 PDT
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?
Comment 54 Jer Noble 2013-03-25 22:29:43 PDT
(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!).
Comment 55 Jer Noble 2013-03-26 17:20:20 PDT
Turns out rolling out these patches is more difficult than just fixing the bug.  Filed bug 113355 to track the fix.
Comment 56 Christian Biesinger 2013-04-03 15:15:56 PDT
fixed in the bug mentioned in the last comment