Bug 70124

Summary: [Chromium Skia on Mac] Improve focus ring
Product: WebKit Reporter: Cary Clark <caryclark>
Component: New BugsAssignee: 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 Flags
Patch none

Description Cary Clark 2011-10-14 11:36:57 PDT
[Chromium Skia on Mac] Improve focus ring
Comment 1 Cary Clark 2011-10-14 11:54:25 PDT
Created attachment 111040 [details]
Patch
Comment 2 Cary Clark 2011-10-14 11:56:01 PDT
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 3 Eric Seidel (no email) 2011-10-14 12:10:01 PDT
Comment on attachment 111040 [details]
Patch

This will need to update lots of unitttests, no?
Comment 4 Cary Clark 2011-10-14 12:23:46 PDT
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 5 Eric Seidel (no email) 2011-10-14 14:15:51 PDT
Comment on attachment 111040 [details]
Patch

OK.  Thanks.
Comment 6 WebKit Review Bot 2011-10-14 15:23:31 PDT
Comment on attachment 111040 [details]
Patch

Clearing flags on attachment: 111040

Committed r97519: <http://trac.webkit.org/changeset/97519>
Comment 7 WebKit Review Bot 2011-10-14 15:23:35 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Dimitri Glazkov (Google) 2011-10-14 16:25:59 PDT
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?
Comment 9 Dimitri Glazkov (Google) 2011-10-14 16:27:11 PDT
It's weird, but this is breaking CG builds too.
Comment 10 epoger 2011-10-17 10:47:39 PDT
(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 11 Cary Clark 2011-10-17 10:52:56 PDT
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.
Comment 12 epoger 2011-10-18 11:35:06 PDT
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
Comment 13 Cary Clark 2011-10-19 04:58:28 PDT
The errors that caused this patch to break the CG build have been fixed. Please re-review.
Comment 14 epoger 2011-10-19 07:51:28 PDT
(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 15 Adam Barth 2011-10-19 13:28:03 PDT
Comment on attachment 111040 [details]
Patch

I'm mostly rubberstamping this patch.
Comment 16 Cary Clark 2011-10-19 13:32:23 PDT
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 17 Cary Clark 2011-10-20 05:13:37 PDT
Comment on attachment 111040 [details]
Patch

turning on patch early so I can monitor it all day
Comment 18 WebKit Review Bot 2011-10-20 06:18:00 PDT
Comment on attachment 111040 [details]
Patch

Clearing flags on attachment: 111040

Committed r97971: <http://trac.webkit.org/changeset/97971>
Comment 19 WebKit Review Bot 2011-10-20 06:18:05 PDT
All reviewed patches have been landed.  Closing bug.