Bug 25648 – When the video controls are updated the whole video is redisplayed (RenderSlider repaint).

• Component: Layout and Rendering
• Status: ASSIGNED

[Pierre d'Herbemont]

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

[Simon Fraser (smfr)]

<rdar://problem/6870332>

[Simon Fraser (smfr)]

Created attachment 30143 [details]
Testcase with slider

This testcase shows over-painting by the slider when inline, and even when it's a block.

[Hin-Chung Lam]

I cannot assign the bug to myself.. but I'm working on this now.

[Hin-Chung Lam]

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?

[Hin-Chung Lam]

Created attachment 40144 [details]
Prevent excessive relayouts

Attempt to solve the problem of excessive relayouts by limiting relayout ib the subtree rooted at RenderSlider.

[Hin-Chung Lam]

Created attachment 40146 [details]
Prevent excessive relayouts

Fixing style issues of previous patch.

[Eric Seidel (no email)]

Which tests is this covered by?  You should ideally list them in the ChangeLog.

[Hin-Chung Lam]

(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

[Eric Seidel (no email)]

Ah, OK.  It wasn't clear from the ChangeLog that failing tests would now start passing after this.

[Hin-Chung Lam]

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.

[Eric Seidel (no email)]

We have the ability to test repaint bugs.  Mitz added that to DRT iirc.

[Hin-Chung Lam]

OK, I pulled this off, and make sure it is covered by repaint tests.

[Simon Fraser (smfr)]

Are we expecting a new patch?

[Hin-Chung Lam]

Created attachment 40476 [details]
Prevent excessive relayouts

[Simon Fraser (smfr)]

What happened to the objectIsRelayoutBoundary() suggestion?

[Hin-Chung Lam]

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.

[Hin-Chung Lam]

Created attachment 40478 [details]
Prevent excessive relayouts

Fixing style nit of previous patch.

[Eric Seidel (no email)]

Comment on attachment 40478 [details]
Prevent excessive relayouts

Tabs in ChangeLog.

[Hin-Chung Lam]

Oops.. that was careless.. will fix it

[Hin-Chung Lam]

Created attachment 40641 [details]
Prevent excessive relayouts

Uploading a new patch and removed tabs in the change log.

[Dave Hyatt]

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.

[Hin-Chung Lam]

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.

[Maciej Stachowiak]

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?

[Eric Seidel (no email)]

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.

[Hin-Chung Lam]

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?

[Hin-Chung Lam]

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

[Hin-Chung Lam]

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

[Hin-Chung Lam]

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.

[Hin-Chung Lam]

Created attachment 49452 [details]
patch

[Hin-Chung Lam]

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.

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

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

[Hin-Chung Lam]

It has some binary files in the patch. I was thinking to do a git commit, does the commit queue try binary patches?

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

Comment on attachment 49453 [details]
patch

r- due to missing EWS results.

[James Robinson]

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

[James Robinson]

Created attachment 50114 [details]
SVN-generated patch to try to make the EWS bots happy

[Hin-Chung Lam]

Thanks for doing that.

[James Robinson]

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.

[Hin-Chung Lam]

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

[Hin-Chung Lam]

Created attachment 50244 [details]
Patch

[Hin-Chung Lam]

(In reply to comment #46)
> Created an attachment (id=50244) [details]
> Patch

Added binary files in the patch.

[Dave Hyatt]

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.