Summary: | [Chromium Skia on Mac] Improve focus ring | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Cary Clark <caryclark> | ||||
Component: | New Bugs | Assignee: | Cary Clark <caryclark> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | abarth, dglazkov, epoger, eric, webkit.review.bot | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Bug Depends on: | 70156 | ||||||
Bug Blocks: | |||||||
Attachments: |
|
Description
Cary Clark
2011-10-14 11:36:57 PDT
Created attachment 111040 [details]
Patch
Hi Eric. I know this is similar to a patch you approved before. I think I got this one right and understand the code better this time around. Let me know if I can improve the CL comment to better explain my reasoning. Comment on attachment 111040 [details]
Patch
This will need to update lots of unitttests, no?
For the moment, the Skia on Mac unit tests have been disabled in test_expectations, so this won't break it. I verified with Elliot that there's no immediate regression. Once this fix goes in, he'll make sure that whatever rebaselining needs to happen goes in before Skia on Mac is re-enabled on the developer channel. Comment on attachment 111040 [details]
Patch
OK. Thanks.
Comment on attachment 111040 [details] Patch Clearing flags on attachment: 111040 Committed r97519: <http://trac.webkit.org/changeset/97519> All reviewed patches have been landed. Closing bug. This broke a bunch of tests on the bots. http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Mac10.6%20%28CG%29/builds/1826/steps/webkit_tests/logs/stdio What should I do? It's weird, but this is breaking CG builds too. (In reply to comment #8) > This broke a bunch of tests on the bots. > > http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Mac10.6%20%28CG%29/builds/1826/steps/webkit_tests/logs/stdio > > What should I do? It looks to me like the failures were actually caused by a different CL, most likely http://trac.webkit.org/changeset/97517/ . I think CL 97519 (Cary's patch in this bug) can be safely reapplied. Reasoning: - The patch here should only affect Skia builds, not CG builds. So we should definitely investigate further. - Looking at http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Mac10.6%20%28CG%29/builds/1825/steps/webkit_tests/logs/stdio and http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Mac10.6%20%28CG%29/builds/1826/steps/webkit_tests/logs/stdio , I confirmed that the bot went red between these two runs. - Looking at those same pages, here are the window of CLs that could be responsible: Run 1825: webkit r97514, chromium r105594 Run 1826: webkit r97526, chromium r105604 - Looking at the flakiness dashboard for a random sampling of the IMAGE diffs the bot alerted us to, the diffs appear to be in the red underline underneath incorrectly spelled words. (Nothing to do with focus ring.) http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=editing%2Fselection%2F13804.html&showLargeExpectations=true&showExpectations=true http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=editing%2Fstyle%2Fcreate-block-for-style-003.html&showLargeExpectations=true&showExpectations=true - CL 97517, which is within the same window as Cary's 97519, affects spell correction underlining. Thus I think it is the likely cause of these IMAGE diffs. Comment on attachment 111040 [details]
Patch
My apologies for not allowing more time to monitor the checkin and address any concerns. Please review and I will commit in the morning when I can make sure there's no regression for a full day.
In http://trac.webkit.org/changeset/97775 , we checked in new chromium-mac baselines for the following tests to account for the different behavior. Once Cary checks in this patch, we will have to generate a new set of baselines for these tests (and there are actually a bunch more tests that also show focus rings and will also require new baselines) SELECTION BOX IS ONE LINE VS TWO: editing/inserting/editable-inline-element.html EDGE OF FOCUS RING: editing/pasteboard/4641033.html editing/pasteboard/4944770-1.html editing/pasteboard/4944770-2.html editing/pasteboard/5478250.html editing/pasteboard/drop-text-without-selection.html editing/pasteboard/pasting-tabs.html editing/selection/3690703-2.html editing/selection/3690703.html editing/selection/3690719.html editing/selection/4397952.html editing/selection/caret-before-select.html (SHOWS IT BEST) editing/selection/caret-rtl-2-left.html editing/selection/caret-rtl-2.html editing/selection/caret-rtl-right.html editing/selection/caret-rtl.html editing/selection/extend-selection-bidi.html editing/selection/fake-doubleclick.html editing/selection/move-past-trailing-space.html editing/selection/replaced-boundaries-3.html editing/selection/select-box.html editing/selection/select-element-paragraph-boundary.html editing/selection/select-from-textfield-outwards.html editing/selection/selection-3748164-fix.html editing/style/5046875-1.html editing/style/5046875-2.html fast/css/focus-ring-outline-width.html fast/css/outline-auto-location.html fast/events/autoscroll.html fast/events/context-no-deselect.html fast/forms/input-appearance-focus.html fast/forms/input-appearance-number-rtl.html fast/forms/input-appearance-selection.html fast/forms/input-double-click-selection-gap-bug.html fast/forms/input-placeholder-visibility-2.html fast/forms/input-text-double-click.html fast/forms/input-text-drag-down.html fast/forms/input-text-option-delete.html fast/forms/input-text-scroll-left-on-blur.html fast/forms/listbox-hit-test-zoomed.html fast/forms/plaintext-mode-2.html fast/forms/select-listbox-multiple-no-focusring.html fast/forms/tabbing-input-iframe.html fast/forms/textarea-scrolled-type.html fast/forms/textfield-outline.html fast/forms/validation-message-appearance.html fast/frames/take-focus-from-iframe.html fast/inline/continuation-outlines-with-layers-2.html fast/inline/continuation-outlines-with-layers.html fast/inline/continuation-outlines.html fast/text/international/rtl-caret.html fast/transforms/transformed-caret.html The errors that caused this patch to break the CG build have been fixed. Please re-review. (In reply to comment #13) > The errors that caused this patch to break the CG build have been fixed. Please re-review. As I wrote in comment #10, I don't think this patch ever broke the CG build. At any rate, it's ready for re-review. :-) Comment on attachment 111040 [details]
Patch
I'm mostly rubberstamping this patch.
Comment on attachment 111040 [details]
Patch
I'm going to hold off on the commit until the morning when I can verify that nothing gets broke.
Comment on attachment 111040 [details]
Patch
turning on patch early so I can monitor it all day
Comment on attachment 111040 [details] Patch Clearing flags on attachment: 111040 Committed r97971: <http://trac.webkit.org/changeset/97971> All reviewed patches have been landed. Closing bug. |