Bug 111000

Summary:

Selection gaps don't repaint correctly with transforms

Product:

WebKit

Reporter:

Shezan Baig <shezbaig.wk>

Component:

Layout and Rendering

Assignee:

Shezan Baig <shezbaig.wk>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

buildbot, commit-queue, dglazkov, enrica, eric, esprehn+autocc, hyatt, jchaffraix, ojan.autocc, rniwa, simon.fraser, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

URL:

http://bloomberg.github.com/chromium.bb/repros/selectionGapTransform.html

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	webkit.review.bot: commit-queue-
Patch	buildbot: commit-queue-
Patch	none
Patch (from blink checkin)	none

Shezan Baig

Reported 2013-02-27 11:46:55 PST

The invalidate rect is not being computed correctly.

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
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.

Shezan Baig

Comment 1 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)

Shezan Baig

Comment 2 2013-03-13 15:35:40 PDT

Created attachment 193010 [details] Patch

Shezan Baig

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

WebKit Review Bot

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

WebKit Review Bot

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

Shezan Baig

Comment 6 2013-03-13 21:38:12 PDT

Created attachment 193060 [details] Patch

Shezan Baig

Comment 7 2013-03-13 21:58:15 PDT

Created attachment 193063 [details] Patch Add expected results for cr-linux

WebKit Review Bot

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

Shezan Baig

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

Shezan Baig

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

Shezan Baig

Comment 11 2013-03-14 13:58:35 PDT

Added bug 112379 for the cleanup work

Build Bot

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

Ryosuke Niwa

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

Shezan Baig

Comment 14 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?

Build Bot

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

Shezan Baig

Comment 16 2013-03-14 18:58:54 PDT

Created attachment 193218 [details] Patch

Julien Chaffraix

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

Shezan Baig

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

Shezan Baig

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

WebKit Commit Bot

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

WebKit Commit Bot

Comment 21 2013-04-11 19:37:08 PDT

All reviewed patches have been landed. Closing bug.

Simon Fraser (smfr)

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

Ryosuke Niwa

Comment 23 2013-04-13 10:35:35 PDT

(In reply to comment #22) > The tests are failing sometimes: http://build.webkit.org/results/Apple%20MountainLion%20Debug%20WK2%20(Tests)/r148331%20(8829)/results.html They're always failing :( http://webkit-test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fast%2Frepaint%2Fselection-gap-fixed-child.html%2Cfast%2Frepaint%2Fselection-gap-flipped-fixed-child.html

Note You need to log in before you can comment on or make changes to this bug.