Bug 70124 – [Chromium Skia on Mac] Improve focus ring

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Cary Clark]

[Chromium Skia on Mac] Improve focus ring

[Cary Clark]

Created attachment 111040 [details]
Patch

[Cary Clark]

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.

[Eric Seidel (no email)]

Comment on attachment 111040 [details]
Patch

This will need to update lots of unitttests, no?

[Cary Clark]

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.

[Eric Seidel (no email)]

Comment on attachment 111040 [details]
Patch

OK.  Thanks.

[WebKit Review Bot]

Comment on attachment 111040 [details]
Patch

Clearing flags on attachment: 111040

Committed r97519: <http://trac.webkit.org/changeset/97519>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Dimitri Glazkov (Google)]

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?

[Dimitri Glazkov (Google)]

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.

[Cary Clark]

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

[Cary Clark]

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

[Adam Barth]

Comment on attachment 111040 [details]
Patch

I'm mostly rubberstamping this patch.

[Cary Clark]

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.

[Cary Clark]

Comment on attachment 111040 [details]
Patch

turning on patch early so I can monitor it all day

[WebKit Review Bot]

Comment on attachment 111040 [details]
Patch

Clearing flags on attachment: 111040

Committed r97971: <http://trac.webkit.org/changeset/97971>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.