Bug 64553 – [Mac] REGRESSION(r90089): search field focus ring painted multiple times

• Status: ASSIGNED
• Component: Forms

[Daniel Bates]

Created attachment 100848 [details]
Example

When typing into an <input type="search"> the focus rings appears to be drawn multiple times. This is particularly noticeable at the top and bottom-right corner of the focus ring which an extra line and a smear mark, respectively. The extra line seems to grow longer as you type more.

[Daniel Bates]

Created attachment 100849 [details]
[Screenshot] Type one character into search field

[Daniel Bates]

Created attachment 100850 [details]
[Screenshot] Type two words into search field

Notice the rendering artifacts around the top and bottom-right corner of the focus ring.

[Daniel Bates]

For completeness, this issue can be seen in Mac nightly r90991. This issue isn't seen in shipping Safari Version 5.0.5 (6533.21.1) on Mac.

[Alexey Proskuryakov]

<rdar://problem/9778524>

[Alexey Proskuryakov]

See also: bug 10049.

[Kent Tamura]

input[type="search"]:focus {
  outline-width: 6px;
}

fixes the problem.  Is this a correct fix?

I'm not sure what is the root cause.
I investigated how RenderTextControlSIngleLine::layout() and paint() were called.
When a user types a key, the followings are called:

r90088:
layout(), layout(), PaintPhaseBlockBackgound, PaintPhaseChildBlockBackgrounds, PaintPhaseFloat, PaintPhaseForeground, then PaintPhaseOutline.

r90089:
layout(), PaintPhaseBlockBackgound, PaintPhaseChildBlockBackgrounds, PaintPhaseFloat, PaintPhaseForeground, then PaintPhaseOutline.

It seems that there are no differences of the number of outline paintings.

[Kent Tamura]

(In reply to comment #6)
> input[type="search"]:focus {
>   outline-width: 6px;
> }
> fixes the problem.  Is this a correct fix?

I found this fix was incomplete.

> r90088:
> layout(), layout(), PaintPhaseBlockBackgound, PaintPhaseChildBlockBackgrounds, PaintPhaseFloat, PaintPhaseForeground, then PaintPhaseOutline.
> 
> r90089:
> layout(), PaintPhaseBlockBackgound, PaintPhaseChildBlockBackgrounds, PaintPhaseFloat, PaintPhaseForeground, then PaintPhaseOutline.

The repainting areas were different:
r90088:
PaintInfo: context=0x7fff5fbfadc0 (7,41) 131x25 phase=BlockBackground root=0x0
PaintInfo: context=0x7fff5fbfadc0 (7,41) 131x25 phase=ChildBlockBackgrounds root=0x0
PaintInfo: context=0x7fff5fbfadc0 (7,41) 131x25 phase=Float root=0x0
PaintInfo: context=0x7fff5fbfadc0 (7,41) 131x25 phase=Foreground root=0x0
PaintInfo: context=0x7fff5fbfadc0 (7,41) 131x25 phase=Outline root=0x0

r90089 or later:
PaintInfo: context=0x7fff5fbfadc0 (24,39) 114x27 phase=BlockBackground root=0x0
PaintInfo: context=0x7fff5fbfadc0 (24,39) 114x27 phase=ChildBlockBackgrounds root=0x0
PaintInfo: context=0x7fff5fbfadc0 (24,39) 114x27 phase=Float root=0x0
PaintInfo: context=0x7fff5fbfadc0 (24,39) 114x27 phase=Foreground root=0x0
PaintInfo: context=0x7fff5fbfadc0 (24,39) 114x27 phase=Outline root=0x0

[Jon Lee]

Using ToT, I am only seeing this on Snow Leopard, and not Lion. Also, I can repro the first screenshot (with the white line), but not the second (with the extra smudges). Has anything changed for your side?

[Kent Tamura]

Well, I understand what's happening.

1. A user types into a search field.
2. The inner editable element is updated.
3. A rectangle larger than the rectangle of the inner editable element is invalidated.
  See RenderBox::clippedOverflowRectForRepaint(). We inflate the rectangle by RenderStyle::maximulOutlineSize() even if the style has no outline.
4. Clear the inflated rectangle.
  The search field border is cleared, but the focus ring outside of the inflated rectangle is not cleared.
5. Clip the graphics context with the inflated rectangle
6. Draw NSSearchFieldCell with the focus ring.  Surprisingly the focus ring ignores the clipping. So the outer part of focus ring is drawn onto an existing focus ring.

I guess Lion respects clipping in focus ring drawing.

I'll investigate how to fix this issue.

[Kent Tamura]

Created attachment 102249 [details]
Patch

[Kent Tamura]

(In reply to comment #10)
> Created an attachment (id=102249) [details]
> Patch

I confirmed the test failed without the patch.

[WebKit Review Bot]

Comment on attachment 102249 [details]
Patch

Attachment 102249 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9266200

New failing tests:
fast/repaint/search-field-cancel.html

[Kent Tamura]

Created attachment 102321 [details]
Patch 2

search-field-cance.html should be updated.

[WebKit Review Bot]

Attachment 102321 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

ERROR: FAILURES FOR <linux, x86, release, cpu>
ERROR: Line:19 Test lacks BUG modifier. editing/selection/empty-cell-right-click.html
ERROR: Line:20 Test lacks BUG modifier. editing/selection/dump-as-markup.html
ERROR: Line:21 Test lacks BUG modifier. fast/js/array-sort-modifying-tostring.html
ERROR: Line:22 Test lacks BUG modifier. fast/overflow/lots-of-sibling-inline-boxes.html
ERROR: Line:23 Test lacks BUG modifier. inspector/cookie-resource-match.html
LayoutTests/platform/qt/test_expectations.txt:19:  Test lacks BUG modifier. editing/selection/empty-cell-right-click.html  [test/expectations] [2]
LayoutTests/platform/qt/test_expectations.txt:20:  Test lacks BUG modifier. editing/selection/dump-as-markup.html  [test/expectations] [2]
LayoutTests/platform/qt/test_expectations.txt:21:  Test lacks BUG modifier. fast/js/array-sort-modifying-tostring.html  [test/expectations] [2]
LayoutTests/platform/qt/test_expectations.txt:22:  Test lacks BUG modifier. fast/overflow/lots-of-sibling-inline-boxes.html  [test/expectations] [2]
LayoutTests/platform/qt/test_expectations.txt:23:  Test lacks BUG modifier. inspector/cookie-resource-match.html  [test/expectations] [2]
Total errors found: 5 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Seidel (no email)]

Comment on attachment 102321 [details]
Patch 2

Looking at the pixel results, it looks like we're now invalidating more on the left hand side of the box, no?

[Eric Seidel (no email)]

Comment on attachment 102321 [details]
Patch 2

Please explain the pixel result change (since it looks like we're painting more, not less).  Otherwise I'm happhy to review this once you do.