Bug 105574 - Fix scrollRectToVisible in the presence of transforms
Summary: Fix scrollRectToVisible in the presence of transforms
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-12-20 13:52 PST by Chris Hopman
Modified: 2013-03-20 15:56 PDT (History)
8 users (show)

See Also:


Attachments
Patch (5.99 KB, patch)
2012-12-20 14:17 PST, Chris Hopman
no flags Details | Formatted Diff | Diff
Patch (13.22 KB, patch)
2012-12-21 16:47 PST, Chris Hopman
no flags Details | Formatted Diff | Diff
Patch (14.28 KB, patch)
2013-01-02 10:28 PST, Chris Hopman
no flags Details | Formatted Diff | Diff
Patch (13.19 KB, patch)
2013-01-11 18:54 PST, Chris Hopman
no flags Details | Formatted Diff | Diff
Patch (9.09 KB, patch)
2013-01-14 17:14 PST, Chris Hopman
no flags Details | Formatted Diff | Diff
Now with test results. (38.97 KB, patch)
2013-01-14 18:10 PST, Chris Hopman
no flags Details | Formatted Diff | Diff
Patch (10.54 KB, patch)
2013-01-15 18:42 PST, Chris Hopman
no flags Details | Formatted Diff | Diff
Patch (12.96 KB, patch)
2013-01-16 15:39 PST, Chris Hopman
no flags Details | Formatted Diff | Diff
Rebase :( (13.27 KB, patch)
2013-01-18 13:01 PST, Chris Hopman
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Hopman 2012-12-20 13:52:11 PST
Fix scrollRectToVisible in the presence of transforms
Comment 1 Chris Hopman 2012-12-20 14:17:47 PST
Created attachment 180409 [details]
Patch
Comment 2 Simon Fraser (smfr) 2012-12-20 14:34:11 PST
Comment on attachment 180409 [details]
Patch

This needs a bunch of testcases.
Comment 3 Chris Hopman 2012-12-21 16:47:25 PST
Created attachment 180575 [details]
Patch
Comment 4 Chris Hopman 2012-12-21 16:49:32 PST
(In reply to comment #2)
> (From update of attachment 180409 [details])
> This needs a bunch of testcases.

Sorry about that, meant to uncheck r? until I had some tests.
Comment 5 Build Bot 2012-12-21 18:42:49 PST
Comment on attachment 180575 [details]
Patch

Attachment 180575 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15449621

New failing tests:
editing/input/reveal-selection-transformed-textarea.html
fast/spatial-navigation/snav-div-overflow-scrol-hidden.html
Comment 6 WebKit Review Bot 2012-12-21 19:14:18 PST
Comment on attachment 180575 [details]
Patch

Attachment 180575 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15475172

New failing tests:
platform/chromium/fast/forms/suggestion-picker/date-suggestion-picker-appearance-with-scroll-bar.html
platform/chromium/fast/forms/suggestion-picker/month-suggestion-picker-appearance-with-scroll-bar.html
editing/input/reveal-caret-of-multiline-input.html
platform/chromium/fast/forms/suggestion-picker/week-suggestion-picker-appearance-with-scroll-bar.html
Comment 7 Chris Hopman 2013-01-02 10:28:07 PST
Created attachment 181034 [details]
Patch
Comment 8 WebKit Review Bot 2013-01-02 11:26:16 PST
Comment on attachment 181034 [details]
Patch

Attachment 181034 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15638496

New failing tests:
platform/chromium/fast/forms/suggestion-picker/date-suggestion-picker-appearance-with-scroll-bar.html
platform/chromium/fast/forms/suggestion-picker/month-suggestion-picker-appearance-with-scroll-bar.html
editing/input/reveal-caret-of-multiline-input.html
platform/chromium/fast/forms/suggestion-picker/week-suggestion-picker-appearance-with-scroll-bar.html
Comment 9 Build Bot 2013-01-02 14:28:27 PST
Comment on attachment 181034 [details]
Patch

Attachment 181034 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15635553

New failing tests:
fast/spatial-navigation/snav-div-overflow-scrol-hidden.html
editing/input/reveal-selection-transformed-textarea.html
fast/sub-pixel/sub-pixel-iframe-copy-on-scroll.html
fast/sub-pixel/sub-pixel-accumulates-to-layers.html
fast/sub-pixel/transformed-iframe-copy-on-scroll.html
Comment 10 Chris Hopman 2013-01-11 18:54:57 PST
Created attachment 182455 [details]
Patch
Comment 11 Build Bot 2013-01-11 19:46:41 PST
Comment on attachment 182455 [details]
Patch

Attachment 182455 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15822167

New failing tests:
editing/input/reveal-selection-transformed-overflow-parent.html
editing/input/caret-at-the-edge-of-contenteditable.html
editing/input/reveal-caret-of-multiline-input.html
editing/input/reveal-caret-of-multiline-contenteditable.html
fast/spatial-navigation/snav-div-overflow-scrol-hidden.html
Comment 12 WebKit Review Bot 2013-01-11 20:04:50 PST
Comment on attachment 182455 [details]
Patch

Attachment 182455 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15833157

New failing tests:
platform/chromium/fast/forms/suggestion-picker/week-suggestion-picker-appearance-with-scroll-bar.html
platform/chromium/fast/forms/suggestion-picker/month-suggestion-picker-appearance-with-scroll-bar.html
platform/chromium/fast/forms/suggestion-picker/date-suggestion-picker-appearance-with-scroll-bar.html
editing/input/reveal-caret-of-multiline-input.html
editing/input/reveal-caret-of-multiline-contenteditable.html
editing/input/caret-at-the-edge-of-contenteditable.html
Comment 13 WebKit Review Bot 2013-01-11 21:08:55 PST
Comment on attachment 182455 [details]
Patch

Attachment 182455 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15833178

New failing tests:
platform/chromium/fast/forms/suggestion-picker/week-suggestion-picker-appearance-with-scroll-bar.html
platform/chromium/fast/forms/suggestion-picker/month-suggestion-picker-appearance-with-scroll-bar.html
platform/chromium/fast/forms/suggestion-picker/date-suggestion-picker-appearance-with-scroll-bar.html
editing/input/reveal-caret-of-multiline-input.html
editing/input/reveal-caret-of-multiline-contenteditable.html
editing/input/caret-at-the-edge-of-contenteditable.html
Comment 14 Chris Hopman 2013-01-14 17:14:01 PST
Created attachment 182665 [details]
Patch
Comment 15 Chris Hopman 2013-01-14 17:22:49 PST
Comment on attachment 182665 [details]
Patch

The actual WebCore change is only very slightly changed from Patch #2 (the r+ one) to only convert to IntXXX when needed (I.e. changed to boundingBox() from enclosingBoundingBox()) and to use roundedIntRect() instead of enclosingIntRect() to be closer to the previous behavior.

The main change from patch #2 is that the tests were rewritten (because I thought the others were much too complicated).

Each of the test failures are 1 pixel changes in scroll offsets. At least for the suggestion-pickers this change appears to be for the better (i.e. more of the selected row is visible without exposing anything beyond it).
Comment 16 Simon Fraser (smfr) 2013-01-14 17:30:39 PST
Comment on attachment 182665 [details]
Patch

Why are there no expected files for the new tests?
Comment 17 Chris Hopman 2013-01-14 18:10:33 PST
Created attachment 182672 [details]
Now with test results.
Comment 18 Simon Fraser (smfr) 2013-01-14 18:17:23 PST
Comment on attachment 182672 [details]
Now with test results.

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

Code changes are OK, but I think the tests could be improved.

> LayoutTests/ChangeLog:17
> +        * platform/chromium-linux/editing/input/reveal-caret-of-transformed-input-scrollable-parent-expected.png: Added.
> +        * platform/chromium-linux/editing/input/reveal-caret-of-transformed-input-scrollable-parent-expected.txt: Added.
> +        * platform/chromium-linux/editing/input/reveal-caret-of-transformed-multiline-input-expected.png: Added.
> +        * platform/chromium-linux/editing/input/reveal-caret-of-transformed-multiline-input-expected.txt: Added.

Can you make these ref tests, or fix them to use less text so that the results are more useful between platforms? Also, why do the expected images not use the mock scrollbar theme? Most platforms use that now.
Comment 19 Chris Hopman 2013-01-15 17:25:07 PST
Comment on attachment 182672 [details]
Now with test results.

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

>> LayoutTests/ChangeLog:17
>> +        * platform/chromium-linux/editing/input/reveal-caret-of-transformed-multiline-input-expected.txt: Added.
> 
> Can you make these ref tests, or fix them to use less text so that the results are more useful between platforms? Also, why do the expected images not use the mock scrollbar theme? Most platforms use that now.

I don't see how to make these ref tests. For these I was just trying to closely mimic the other 4 reveal-caret-*** tests, but I could change them to be similar to my earlier tests. That is, I could change them to check pass/fail in javascript by checking that the scroll offsets are reasonable (as the scroll offsets are all we really care about). What do you think about that?

chromium.linux doesn't seem to use the mock theme.
Comment 20 Chris Hopman 2013-01-15 18:42:05 PST
Created attachment 182900 [details]
Patch
Comment 21 WebKit Review Bot 2013-01-15 20:47:21 PST
Comment on attachment 182900 [details]
Patch

Attachment 182900 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15905220

New failing tests:
platform/chromium/fast/forms/suggestion-picker/week-suggestion-picker-appearance-with-scroll-bar.html
platform/chromium/fast/forms/suggestion-picker/month-suggestion-picker-appearance-with-scroll-bar.html
platform/chromium/fast/forms/suggestion-picker/date-suggestion-picker-appearance-with-scroll-bar.html
editing/input/reveal-caret-of-multiline-input.html
editing/input/reveal-caret-of-multiline-contenteditable.html
editing/input/caret-at-the-edge-of-contenteditable.html
Comment 22 Build Bot 2013-01-16 01:27:04 PST
Comment on attachment 182900 [details]
Patch

Attachment 182900 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15884908

New failing tests:
editing/input/caret-at-the-edge-of-contenteditable.html
editing/input/reveal-caret-of-multiline-input.html
editing/input/reveal-caret-of-multiline-contenteditable.html
fast/spatial-navigation/snav-div-overflow-scrol-hidden.html
Comment 23 Build Bot 2013-01-16 02:08:49 PST
Comment on attachment 182900 [details]
Patch

Attachment 182900 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15902358

New failing tests:
editing/input/caret-at-the-edge-of-contenteditable.html
editing/input/reveal-caret-of-multiline-input.html
editing/input/reveal-caret-of-multiline-contenteditable.html
fast/spatial-navigation/snav-div-overflow-scrol-hidden.html
Comment 24 Chris Hopman 2013-01-16 15:39:24 PST
Created attachment 183045 [details]
Patch
Comment 25 Chris Hopman 2013-01-17 13:31:25 PST
(In reply to comment #24)
> Created an attachment (id=183045) [details]
> Patch

This is just updating TestExpectations for the failing tests that need rebaselining. As mentioned above, the failing tests are all a 1px difference in scroll offsets (for the suggestion-picker tests, the new behavior is "better" and for the rest neither is clearly so).
Comment 26 WebKit Review Bot 2013-01-18 12:46:17 PST
Comment on attachment 183045 [details]
Patch

Rejecting attachment 183045 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:
ts/platform/chromium/TestExpectations
Hunk #1 FAILED at 4383.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/chromium/TestExpectations.rej
patching file LayoutTests/platform/mac/TestExpectations
Hunk #1 FAILED at 1288.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/mac/TestExpectations.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', '--force', '--reviewer', 'Simon Fraser']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Full output: http://queues.webkit.org/results/15944688
Comment 27 Chris Hopman 2013-01-18 13:01:20 PST
Created attachment 183534 [details]
Rebase :(
Comment 28 WebKit Review Bot 2013-01-18 13:45:51 PST
Comment on attachment 183534 [details]
Rebase :(

Clearing flags on attachment: 183534

Committed r140202: <http://trac.webkit.org/changeset/140202>
Comment 29 WebKit Review Bot 2013-01-18 13:45:57 PST
All reviewed patches have been landed.  Closing bug.
Comment 31 Simon Fraser (smfr) 2013-01-18 16:40:38 PST
Actually that seems to been caused by something else (bug  107340).
Comment 32 Zan Dobersek 2013-01-19 02:36:30 PST
(In reply to comment #23)
> (From update of attachment 182900 [details])
> Attachment 182900 [details] did not pass mac-ews (mac):
> Output: http://queues.webkit.org/results/15902358
> 
> New failing tests:
> editing/input/caret-at-the-edge-of-contenteditable.html
> editing/input/reveal-caret-of-multiline-input.html
> editing/input/reveal-caret-of-multiline-contenteditable.html
> fast/spatial-navigation/snav-div-overflow-scrol-hidden.html

All these tests seem fine to rebaseline except the last, spatival navigation failure. Here's the diff:
--- /Volumes/Data/slave/mountainlion-release-tests-wk1/build/layout-test-results/fast/spatial-navigation/snav-div-overflow-scrol-hidden-expected.txt
+++ /Volumes/Data/slave/mountainlion-release-tests-wk1/build/layout-test-results/fast/spatial-navigation/snav-div-overflow-scrol-hidden-actual.txt
@@ -11,8 +11,8 @@
 PASS gFocusedDocument.activeElement.getAttribute("id") is "f3"
 PASS gFocusedDocument.activeElement.getAttribute("id") is "f3"
 PASS gFocusedDocument.activeElement.getAttribute("id") is "f4"
-PASS gFocusedDocument.activeElement.getAttribute("id") is "f4"
-PASS gFocusedDocument.activeElement.getAttribute("id") is "f5"
+FAIL gFocusedDocument.activeElement.getAttribute("id") should be f4. Was f5.
+FAIL gFocusedDocument.activeElement.getAttribute("id") should be f5. Was f6.
 PASS gFocusedDocument.activeElement.getAttribute("id") is "f6"
 PASS gFocusedDocument.activeElement.getAttribute("id") is "f9"
 This test is testing that div with overflow:auto would scroll, by div with overflow:hidden would not.

Ideas? Besides Mac, it's also failing on GTK, EFL and Qt.
Comment 33 Chris Hopman 2013-01-22 10:11:54 PST
(In reply to comment #32)
> (In reply to comment #23)
> > (From update of attachment 182900 [details] [details])
> > Attachment 182900 [details] [details] did not pass mac-ews (mac):
> > Output: http://queues.webkit.org/results/15902358
> > 
> > New failing tests:
> > editing/input/caret-at-the-edge-of-contenteditable.html
> > editing/input/reveal-caret-of-multiline-input.html
> > editing/input/reveal-caret-of-multiline-contenteditable.html
> > fast/spatial-navigation/snav-div-overflow-scrol-hidden.html
> 
> All these tests seem fine to rebaseline except the last, spatival navigation failure. Here's the diff:
> --- /Volumes/Data/slave/mountainlion-release-tests-wk1/build/layout-test-results/fast/spatial-navigation/snav-div-overflow-scrol-hidden-expected.txt
> +++ /Volumes/Data/slave/mountainlion-release-tests-wk1/build/layout-test-results/fast/spatial-navigation/snav-div-overflow-scrol-hidden-actual.txt
> @@ -11,8 +11,8 @@
>  PASS gFocusedDocument.activeElement.getAttribute("id") is "f3"
>  PASS gFocusedDocument.activeElement.getAttribute("id") is "f3"
>  PASS gFocusedDocument.activeElement.getAttribute("id") is "f4"
> -PASS gFocusedDocument.activeElement.getAttribute("id") is "f4"
> -PASS gFocusedDocument.activeElement.getAttribute("id") is "f5"
> +FAIL gFocusedDocument.activeElement.getAttribute("id") should be f4. Was f5.
> +FAIL gFocusedDocument.activeElement.getAttribute("id") should be f5. Was f6.
>  PASS gFocusedDocument.activeElement.getAttribute("id") is "f6"
>  PASS gFocusedDocument.activeElement.getAttribute("id") is "f9"
>  This test is testing that div with overflow:auto would scroll, by div with overflow:hidden would not.
> 
> Ideas? Besides Mac, it's also failing on GTK, EFL and Qt.

Hm, this actually looks like correct behavior to me (i.e. the test should be updated). The test appears to be checking that f7 and f8 are skipped (as they are the elements in the hidden overflow). I don't know about spatial-navigation, so I don't know why an element is sometimes repeated.

Chromium doesn't use spatial-navigation, but maybe I can try to look at this with one of the other ports.
Comment 34 Ryosuke Niwa 2013-01-29 13:38:38 PST
Are you rebaselining these tests? If also, please do so ASAP. We shouldn't be leaving test expectations for days just to rebaseline them later.
Comment 35 Chris Hopman 2013-01-30 15:09:20 PST
(In reply to comment #34)
> Are you rebaselining these tests? If also, please do so ASAP. We shouldn't be leaving test expectations for days just to rebaseline them later.

smfr@ had told me that marking them in TestExpectations would be enough and someone else would then rebaseline. I am happy to do it myself if you can point me to some instructions on how.
Comment 36 Ryosuke Niwa 2013-03-20 15:56:42 PDT
Rebaselined the tests in http://trac.webkit.org/changeset/146397.