Bug 105574 – Fix scrollRectToVisible in the presence of transforms

• Component: New Bugs
• Status: RESOLVED FIXED

[Chris Hopman]

Fix scrollRectToVisible in the presence of transforms

[Chris Hopman]

Created attachment 180409 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 180409 [details]
Patch

This needs a bunch of testcases.

[Chris Hopman]

Created attachment 180575 [details]
Patch

[Chris Hopman]

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

[Build Bot]

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

[WebKit Review Bot]

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

[Chris Hopman]

Created attachment 181034 [details]
Patch

[WebKit Review Bot]

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

[Build Bot]

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

[Chris Hopman]

Created attachment 182455 [details]
Patch

[Build Bot]

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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Chris Hopman]

Created attachment 182665 [details]
Patch

[Chris Hopman]

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

[Simon Fraser (smfr)]

Comment on attachment 182665 [details]
Patch

Why are there no expected files for the new tests?

[Chris Hopman]

Created attachment 182672 [details]
Now with test results.

[Simon Fraser (smfr)]

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.

[Chris Hopman]

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.

[Chris Hopman]

Created attachment 182900 [details]
Patch

[WebKit Review Bot]

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

[Build Bot]

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

[Build Bot]

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

[Chris Hopman]

Created attachment 183045 [details]
Patch

[Chris Hopman]

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

[WebKit Review Bot]

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

[Chris Hopman]

Created attachment 183534 [details]
Rebase :(

[WebKit Review Bot]

Comment on attachment 183534 [details]
Rebase :(

Clearing flags on attachment: 183534

Committed r140202: <http://trac.webkit.org/changeset/140202>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Fraser (smfr)]

This may have broken an editing test on Mac:
http://build.webkit.org/results/Apple%20MountainLion%20Release%20WK1%20(Tests)/r140201%20(5856)/editing/selection/move-by-word-visually-multi-line-pretty-diff.html

[Simon Fraser (smfr)]

Actually that seems to been caused by something else (bug  107340).

[Zan Dobersek]

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

[Chris Hopman]

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

[Ryosuke Niwa]

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.

[Chris Hopman]

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

[Ryosuke Niwa]

Rebaselined the tests in http://trac.webkit.org/changeset/146397.