Fix scrollRectToVisible in the presence of transforms
Created attachment 180409 [details] Patch
Comment on attachment 180409 [details] Patch This needs a bunch of testcases.
Created attachment 180575 [details] Patch
(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 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 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
Created attachment 181034 [details] Patch
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 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
Created attachment 182455 [details] Patch
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 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 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
Created attachment 182665 [details] Patch
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 on attachment 182665 [details] Patch Why are there no expected files for the new tests?
Created attachment 182672 [details] Now with test results.
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 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.
Created attachment 182900 [details] Patch
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 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 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
Created attachment 183045 [details] Patch
(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 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
Created attachment 183534 [details] Rebase :(
Comment on attachment 183534 [details] Rebase :( Clearing flags on attachment: 183534 Committed r140202: <http://trac.webkit.org/changeset/140202>
All reviewed patches have been landed. Closing bug.
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
Actually that seems to been caused by something else (bug 107340).
(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.
(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.
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.
(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.
Rebaselined the tests in http://trac.webkit.org/changeset/146397.