Bug 111000 - Selection gaps don't repaint correctly with transforms
Summary: Selection gaps don't repaint correctly with transforms
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Shezan Baig
URL: http://bloomberg.github.com/chromium....
Keywords:
Depends on:
Blocks:
 
Reported: 2013-02-27 11:46 PST by Shezan Baig
Modified: 2013-04-13 10:35 PDT (History)
12 users (show)

See Also:


Attachments
Patch (60.33 KB, patch)
2013-03-13 15:35 PDT, Shezan Baig
no flags Details | Formatted Diff | Diff
Patch (76.25 KB, patch)
2013-03-13 21:38 PDT, Shezan Baig
no flags Details | Formatted Diff | Diff
Patch (125.73 KB, patch)
2013-03-13 21:58 PDT, Shezan Baig
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch (4.14 KB, patch)
2013-03-14 13:53 PDT, Shezan Baig
buildbot: commit-queue-
Details | Formatted Diff | Diff
Patch (4.13 KB, patch)
2013-03-14 18:58 PDT, Shezan Baig
no flags Details | Formatted Diff | Diff
Patch (from blink checkin) (15.40 KB, patch)
2013-04-11 18:42 PDT, Shezan Baig
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Shezan Baig 2013-02-27 11:46:55 PST
The invalidate rect is not being computed correctly.
Comment 1 Shezan Baig 2013-02-27 11:47:59 PST
I have a patch for this, but I'm having difficulty getting it to reproduce in the test runner (seems like it is invalidating the entire screen when run in test runner, so the issue is not being reproduced)
Comment 2 Shezan Baig 2013-03-13 15:35:40 PDT
Created attachment 193010 [details]
Patch
Comment 3 Shezan Baig 2013-03-13 15:37:46 PDT
In a follow up patch, I plan to remove GapRects completely since it seems like the left/center/right components are not used anywhere.  It's always used in its united form.
Comment 4 WebKit Review Bot 2013-03-13 18:20:11 PDT
Comment on attachment 193010 [details]
Patch

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

New failing tests:
fast/repaint/block-selection-gap-in-composited-layer.html
fast/repaint/japanese-rl-selection-repaint.html
fast/repaint/japanese-rl-selection-repaint-in-regions.html
fast/repaint/selection-gap-transform.html
Comment 5 WebKit Review Bot 2013-03-13 19:08:45 PDT
Comment on attachment 193010 [details]
Patch

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

New failing tests:
fast/repaint/block-selection-gap-in-composited-layer.html
fast/repaint/japanese-rl-selection-repaint.html
fast/repaint/japanese-rl-selection-repaint-in-regions.html
fast/repaint/selection-gap-transform.html
Comment 6 Shezan Baig 2013-03-13 21:38:12 PDT
Created attachment 193060 [details]
Patch
Comment 7 Shezan Baig 2013-03-13 21:58:15 PDT
Created attachment 193063 [details]
Patch

Add expected results for cr-linux
Comment 8 WebKit Review Bot 2013-03-13 23:46:03 PDT
Comment on attachment 193063 [details]
Patch

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

New failing tests:
fast/repaint/japanese-rl-selection-repaint.html
Comment 9 Shezan Baig 2013-03-14 07:29:42 PDT
Looks like it is invalidating one extra line than necessary in the japanese-rl-selection-repaint.html.  Might be due to pixel snapping in computeRectForRepaint, I need to investigate some more
Comment 10 Shezan Baig 2013-03-14 13:53:05 PDT
Created attachment 193179 [details]
Patch

This is a much simpler patch, I removed a lot of the "cleanup" code.  I will make another bug to perform this cleanup, along with the removal of GapRects, which doesn't seem useful because the left/center/right components are always united.

I also simplified the test significantly and made it text-based instead of pixel based.
Comment 11 Shezan Baig 2013-03-14 13:58:35 PDT
Added bug 112379 for the cleanup work
Comment 12 Build Bot 2013-03-14 17:09:00 PDT
Comment on attachment 193179 [details]
Patch

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

New failing tests:
fast/repaint/selection-gap-transform.html
Comment 13 Ryosuke Niwa 2013-03-14 17:10:39 PDT
Comment on attachment 193179 [details]
Patch

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

> LayoutTests/fast/repaint/selection-gap-transform.html:1
> +<head>

Missing DOCTYPE.
Comment 14 Shezan Baig 2013-03-14 17:26:37 PDT
Hmm, not sure why the test is failing on mac.  I can't build on this platform, is there a way to get the actual DumpRenderTree output from the mac ews?
Comment 15 Build Bot 2013-03-14 17:37:32 PDT
Comment on attachment 193179 [details]
Patch

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

New failing tests:
fast/repaint/selection-gap-transform.html
Comment 16 Shezan Baig 2013-03-14 18:58:54 PDT
Created attachment 193218 [details]
Patch
Comment 17 Julien Chaffraix 2013-03-18 13:40:23 PDT
Comment on attachment 193218 [details]
Patch

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

> Source/WebCore/rendering/RenderBlock.cpp:3367
> +    mapLocalToContainer(repaintContainer, transformState, ApplyContainerFlip | UseTransforms);

I would be supportive of testing the ApplyContainerFlip as part of this change if it's not covered. Your change is correct from this perspective.

> LayoutTests/fast/repaint/selection-gap-transform.html:4
> +    <script src="resources/text-based-repaint.js" type="text/javascript" charset="utf-8"></script>
> +    <script type="text/javascript" charset="utf-8">

Please remove the unneeded attributes (type, charset).

> LayoutTests/fast/repaint/selection-gap-transform.html:21
> +    <div style="-webkit-transform: translate(50px, 50px);">
> +        <div id="target" style="background-color: red; width: 100px; height: 100px; position: absolute;"><br/></div><br/>

I would add:
* a fixed positioned case
* a container is transformed and fixed positioned case
(in different tests though)

as boxes with transforms act as containing block for fixed elements too.
Comment 18 Shezan Baig 2013-04-10 07:42:23 PDT
I will work on this bug under the blink repo using this crbug:

https://code.google.com/p/chromium/issues/detail?id=229887

Once it has landed in Blink, I'll post a patch here.
Comment 19 Shezan Baig 2013-04-11 18:42:54 PDT
Created attachment 197708 [details]
Patch (from blink checkin)

This is the patch that was committed into Blink.  Uploading it here in case WebKit folks want to pick it up too.
Comment 20 WebKit Commit Bot 2013-04-11 19:37:05 PDT
Comment on attachment 197708 [details]
Patch (from blink checkin)

Clearing flags on attachment: 197708

Committed r148258: <http://trac.webkit.org/changeset/148258>
Comment 21 WebKit Commit Bot 2013-04-11 19:37:08 PDT
All reviewed patches have been landed.  Closing bug.
Comment 22 Simon Fraser (smfr) 2013-04-12 22:47:22 PDT
The tests are failing sometimes: http://build.webkit.org/results/Apple%20MountainLion%20Debug%20WK2%20(Tests)/r148331%20(8829)/results.html