Bug 110913

Created attachment 190367 [details] Patch

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

Created attachment 190373 [details] Patch

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...

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

Created attachment 190389 [details] Patch

Comment on attachment 190389 [details] Patch This should be all ready for review now.

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.

Ojan is OOO for the rest of the week :(

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 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).

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)

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

Created attachment 190586 [details] Patch

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

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 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.

(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.

Created attachment 190611 [details] Patch

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

(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.

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 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 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

(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?

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...?

Created attachment 190798 [details] Patch

Created attachment 190799 [details] Patch

(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).

(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 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.

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

Created attachment 191634 [details] Patch

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 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

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...

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();

(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.

<rdar://problem/13292684>

(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

(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.

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)

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

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

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

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.

Created attachment 192612 [details] Patch Dependency got fixed, this should pass all tests now.

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 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 on attachment 192612 [details] Patch Clearing flags on attachment: 192612 Committed r145588: <http://trac.webkit.org/changeset/145588>

All reviewed patches have been landed. Closing bug.

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.

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?

(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!).

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

fixed in the bug mentioned in the last comment