Summary: | When the video controls are updated the whole video is redisplayed (RenderSlider repaint). | ||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Pierre d'Herbemont <pdherbemont> | ||||||||||||||||||||||||||
Component: | Layout and Rendering | Assignee: | Hin-Chung Lam <hclam> | ||||||||||||||||||||||||||
Status: | ASSIGNED --- | ||||||||||||||||||||||||||||
Severity: | Normal | CC: | dglazkov, emacemac7, eric.carlson, eric, hclam, jamesr, mitz, simon.fraser | ||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||
Hardware: | Mac | ||||||||||||||||||||||||||||
OS: | OS X 10.5 | ||||||||||||||||||||||||||||
Bug Depends on: | 44907 | ||||||||||||||||||||||||||||
Bug Blocks: | |||||||||||||||||||||||||||||
Attachments: |
|
Created attachment 30143 [details]
Testcase with slider
This testcase shows over-painting by the slider when inline, and even when it's a block.
I cannot assign the bug to myself.. but I'm working on this now. So the plan for this bug would be changing the implementation of RenderSlider to get rid of the element for the slider thumb and draw it manually. But I have some questions implementing this. If element for thumb is removed, there's no RenderObject for the thumb and thus css style cannot be applied to the thumb as it won't have RenderStyle. Is this the right behavior? Created attachment 40144 [details]
Prevent excessive relayouts
Attempt to solve the problem of excessive relayouts by limiting relayout ib the subtree rooted at RenderSlider.
Created attachment 40146 [details]
Prevent excessive relayouts
Fixing style issues of previous patch.
Which tests is this covered by? You should ideally list them in the ChangeLog. (In reply to comment #7) > Which tests is this covered by? You should ideally list them in the ChangeLog. These layout tests should all pass: LayoutTests/fast/forms/slider-delete-while-dragging-thumb.html LayoutTests/fast/forms/slider-mouse-events.html LayoutTests/fast/forms/slider-onchange-event.html LayoutTests/fast/forms/slider-padding.html LayoutTests/fast/forms/slider-thumb-shared-style.html LayoutTests/fast/forms/slider-thumb-stylability.html LayoutTests/fast/forms/slider-transformed.html LayoutTests/fast/forms/slider-zoomed.html LayoutTests/fast/forms/thumbslider-crash.html LayoutTests/fast/forms/thumbslider-no-parent-slider.html LayoutTests/fast/forms/range-default-value.html LayoutTests/fast/forms/range-reset.html LayoutTests/fast/forms/range-thumb-height-percentage.html LayoutTests/media/audio-delete-while-slider-thumb-clicked.html Ah, OK. It wasn't clear from the ChangeLog that failing tests would now start passing after this. This doesn't fix those layout tests but instead shouldn't break them. Since this is to limit the scope of repaints, it's a bug fix but shouldn't change layout. We have the ability to test repaint bugs. Mitz added that to DRT iirc. OK, I pulled this off, and make sure it is covered by repaint tests. Are we expecting a new patch? Created attachment 40476 [details]
Prevent excessive relayouts
What happened to the objectIsRelayoutBoundary() suggestion? objectIsRelayoutBoundary() is used to checkout if parent is subtree root to stop propagating relayouts. But in the case of slider, the relayout is initiated from the slider itself but not from the thumb so adding a check to objectIsRelayoutBoundary() won't help. We could initiate the relayout from the thumb instead, but given that the thumb is only child of slider, and needs of layout is from the slider itself, we can relayout only the tree rooted at slider by calling scheduleRelayout() in an active manner instead of preventing relayout to propagate to parents. This way there won't be an extra check in the critical objectIsRelayoutBoundary() method. Created attachment 40478 [details]
Prevent excessive relayouts
Fixing style nit of previous patch.
Comment on attachment 40478 [details]
Prevent excessive relayouts
Tabs in ChangeLog.
Oops.. that was careless.. will fix it Created attachment 40641 [details]
Prevent excessive relayouts
Uploading a new patch and removed tabs in the change log.
Comment on attachment 40641 [details]
Prevent excessive relayouts
I do not understand why you would have to schedule a relayout explicitly. That's just not really done.
Created attachment 41612 [details]
Prevent excessive relayouts
Sorry about the delay. I've been trying to understand how layouts are scheduled.
In my previous patch I explicitly do a scheduleRelayout() so that RenderSlider::layout() is called and sets the position and size of the thumb appropriately, and also perform appropriate repaints.
In this patch I make RenderSlider the relayout boundary, and in addition to calling setNeedsLayout(true, false) I'm calling m_thumb->renderer()->setNeedsLayout(true) so that a layout is scheduled and rooted at RenderSlider. Please correct me if this is not a good way of causing a layout at RenderSlider.
Comment on attachment 41612 [details]
Prevent excessive relayouts
Didn't mean to r- this patch - reflagging.
(In reply to comment #22) > In this patch I make RenderSlider the relayout boundary, and in addition to > calling setNeedsLayout(true, false) I'm calling > m_thumb->renderer()->setNeedsLayout(true) so that a layout is scheduled and > rooted at RenderSlider. Please correct me if this is not a good way of causing > a layout at RenderSlider. Why is it not enough to mark the thumb for layout? That is, why is it necessary to explicitly mark the slider for layout? Comment on attachment 41612 [details]
Prevent excessive relayouts
Looks like this patch would require an update were it to use the commit-queue. the ews bots failed to apply it.
If you mark thumb to relayout, the request will propogate to RenderView and full relayout is resulted. If you set RenderSlider as relayour boudnary and mark thumb to relayout, the request will stop at RenderSlider. But there will be no relayout at all because RenderSlider::needsRelayout() returns false. So we need these: 1. RenderSlider::needsRelayout() returns true so that a relayout is reshcduled 2. RenderSlider is a relayout boundary so that relayout is only scheduled for this subtree 3. RenderThumb is marked for layout so the whole tree is relayouted with RenderSlider being the root (In reply to comment #26) > If you mark thumb to relayout, the request will propogate to RenderView and > full relayout is resulted. > > If you set RenderSlider as relayour boudnary and mark thumb to relayout, the > request will stop at RenderSlider. But there will be no relayout at all because > RenderSlider::needsRelayout() returns false. Do you mean needsLayout()? Why would it still return false? (In reply to comment #27) > (In reply to comment #26) > > If you mark thumb to relayout, the request will propogate to RenderView and > > full relayout is resulted. > > > > If you set RenderSlider as relayour boudnary and mark thumb to relayout, the > > request will stop at RenderSlider. But there will be no relayout at all because > > RenderSlider::needsRelayout() returns false. > > Do you mean needsLayout()? Why would it still return false? Yes, I meant needsLayout(). RenderSlider is a relayout boundary, thumb which is its child is marked for relayout but RenderSlider is not marked so. See RenderObject::markContainingBlocksForLayout() in RenderObject.h 996: last = o; 997: if (scheduleRelayout && objectIsRelayoutBoundary(last)) 998: break; So if the current node's container is a relayout boundary, the container node is not reached and marked. Comment on attachment 41612 [details] Prevent excessive relayouts (In reply to comment #28) > (In reply to comment #27) > > (In reply to comment #26) > > > If you mark thumb to relayout, the request will propogate to RenderView and > > > full relayout is resulted. > > > > > > If you set RenderSlider as relayour boudnary and mark thumb to relayout, the > > > request will stop at RenderSlider. But there will be no relayout at all because > > > RenderSlider::needsRelayout() returns false. > > > > Do you mean needsLayout()? Why would it still return false? > > Yes, I meant needsLayout(). > > RenderSlider is a relayout boundary, thumb which is its child is marked for > relayout but RenderSlider is not marked so. > > See RenderObject::markContainingBlocksForLayout() in RenderObject.h > > 996: last = o; > 997: if (scheduleRelayout && objectIsRelayoutBoundary(last)) > 998: break; By this time, o has been marked for layout. You should only do the following: 1. Make the slider a relayout boundary, like you did 2. Short-circuit the width calculation in RenderSlider::layout(), like you did 3. In updateFromElement(), mark the thumb’s renderer for layout. Do not mark the slider itself. Also, the assertion you added there is pointless. 4. In setValueForPosition(), mark the thumb’s renderer for layout. Again, do not mark the slider itself. Finally, when I tried doing the above I ran into a repaint issue, which is caused by RenderSlider::layout() pushing (wrong) layout state. The LayoutStateMaintainer initialization strangely passes size() as the offset. I don’t think there’s any need to push layout state there. (In reply to comment #29) > (From update of attachment 41612 [details]) > (In reply to comment #28) > > (In reply to comment #27) > > > (In reply to comment #26) > > > > If you mark thumb to relayout, the request will propogate to RenderView and > > > > full relayout is resulted. > > > > > > > > If you set RenderSlider as relayour boudnary and mark thumb to relayout, the > > > > request will stop at RenderSlider. But there will be no relayout at all because > > > > RenderSlider::needsRelayout() returns false. > > > > > > Do you mean needsLayout()? Why would it still return false? > > > > Yes, I meant needsLayout(). > > > > RenderSlider is a relayout boundary, thumb which is its child is marked for > > relayout but RenderSlider is not marked so. > > > > See RenderObject::markContainingBlocksForLayout() in RenderObject.h > > > > 996: last = o; > > 997: if (scheduleRelayout && objectIsRelayoutBoundary(last)) > > 998: break; > > By this time, o has been marked for layout. > > You should only do the following: > 1. Make the slider a relayout boundary, like you did > 2. Short-circuit the width calculation in RenderSlider::layout(), like you did > 3. In updateFromElement(), mark the thumb’s renderer for layout. Do not mark > the slider itself. Also, the assertion you added there is pointless. > 4. In setValueForPosition(), mark the thumb’s renderer for layout. Again, do > not mark the slider itself. > > Finally, when I tried doing the above I ran into a repaint issue, which is > caused by RenderSlider::layout() pushing (wrong) layout state. The > LayoutStateMaintainer initialization strangely passes size() as the offset. I > don’t think there’s any need to push layout state there. OK, I'll try that. Created attachment 49112 [details]
prevent excessive relayouts
I am concerned that making the slider a relayout boundary may not always be correct. I think this case may break, i.e. the slider’s height won’t change when the thumb’s height does (on hover): <style> input { -webkit-appearance: none; border: solid; } input::-webkit-slider-thumb { -webkit-appearance: none; background: blue; width: 20px; height: 20px; } input:hover::-webkit-slider-thumb { height: 40px; } </style> <input type="range"> Comment on attachment 49112 [details] prevent excessive relayouts I was curious about the case I mentioned in comment #32, so I applied this patch and tested it. I discovered two things: 1) That case fails. It is easier to see if you enclose then <input> element in a <div> with a border or a background. You should address this by refining the check in objectIsRelayoutBoundary. 2) I was wrong about there not being a need to push layout state. It is needed, but the offset is wrong. Instead of using size(), it should use IntSize(x(), y()) as usual. Sorry about misleading you. Created attachment 49452 [details]
patch
Created attachment 49453 [details]
patch
I have changed the patch according to the comments:
1. Added the LayoutStateMaintainer back with offset being IntSize(x(), y())
2. Instead of tweaking the condition of objectisRelayoutBoundary(), make RenderSlider to perform setNeedsLayout(true) when there is a mismatch between the current height and the style of the thumb. This happens to me to be the same as without the obj->isSlider() check in objectIsRelayoutBoundary()
Also included your test case into the layout test.
Comment on attachment 49453 [details]
patch
This patch does not apply meaning we have no useful results from the EWS bots and it cannot be commit-queued. :(
Hunk #1 succeeded at 1 with fuzz 3.
patching file LayoutTests/fast/repaint/slider-thumb-change-height.html
patching file LayoutTests/fast/repaint/slider-update-value.html
patching file LayoutTests/platform/mac/fast/repaint/slider-thumb-change-height-expected.checksum
patch: **** Only garbage was found in the patch input.
patching file LayoutTests/platform/mac/fast/repaint/slider-thumb-change-height-expected.txt
patching file LayoutTests/platform/mac/fast/repaint/slider-update-value-expected.checksum
patch: **** Only garbage was found in the patch input.
patching file LayoutTests/platform/mac/fast/repaint/slider-update-value-expected.txt
patching file WebCore/ChangeLog
Comment on attachment 49453 [details]
patch
Please create patches using either git diff --binary, or better yet, just "webkit-patch upload" as that will run the right git/svn commands for you. :)
It has some binary files in the patch. I was thinking to do a git commit, does the commit queue try binary patches? It's not a question of the commit-queue. The EWS bots can't process your patch either. And yes, webkit-patch and the commit-queue and the EWS bots all handle binary patches. Comment on attachment 49453 [details]
patch
r- due to missing EWS results.
The EWS rejected the patch because it contains .checksum files and was generated from git, not because of any problem with the patch. See https://bugs.webkit.org/show_bug.cgi?id=35804. The easiest way to get this processed would be to generate the patch from an SVN checkout and just reupload it. In fact, I'll go ahead and do that (since I've already generated one). Created attachment 50114 [details]
SVN-generated patch to try to make the EWS bots happy
Thanks for doing that. Actually I was mistaken, the reason the patch died was because it didn't include the .png files. The script dies in a non-obvious way in this fashion. Both of the latest two patches are incorrect (in the same way), they don't have the image results. Eric said how to generate a patch with the images included, please upload a patch following those directions. (In reply to comment #44) > Actually I was mistaken, the reason the patch died was because it didn't > include the .png files. The script dies in a non-obvious way in this fashion. > Both of the latest two patches are incorrect (in the same way), they don't have > the image results. Eric said how to generate a patch with the images included, > please upload a patch following those directions. Will do. Doing a svn checkout now. Created attachment 50244 [details]
Patch
(In reply to comment #46) > Created an attachment (id=50244) [details] > Patch Added binary files in the patch. Comment on attachment 50244 [details]
Patch
I don't think you can get away with making a slider into a layout boundary. The thumb can visually overflow the bounds of the slider itself, unlike text controls, which are guaranteed to clip their contents. In order for a slider to truly function as a layout root, it would have to clip the thumb. The hacks to prevent size changes by querying layout root seem bogus to me as well.
|
- Create a file with a media element: <video src="LayoutTests/media/content/test.mp4" autoplay controls loop ></video> - Disable video repaint callback in the MediaPlayerPrivate code. - Open Quartz Debug, and enable flash on screen update Notice how the whole video gets repainted, instead of juste the controls rect.