Seeing is believing

Deleted: flicker_634318.mp4 219 KB

	flicker_634318.mp4 219 KB View Download

Filed with Apple. OpenRadar copy: https://openradar.appspot.com/27702632

We still may want to investigate on our end.

Moving this nonessential bug to the next milestone.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

I’m still seeing this in 10.12db5 16A286a.

Given that most NSTextViews appear to be working correctly, I suspect Apple won't look into it unless we provide a more detailed root cause.

Commenting out the line "[super didChangeText];" in -[AutocompleteTextFieldEditor didChangeText]" fixes the problem. I've been digging into AppKit to try to get more information about the nature of the problem.

WRT the radar report, I think it's plausible that they might fix it.  I am almost certain that we in the past had similar problems with a seldom-overridden method like this, and they fixed it (likely with no acknowledgement that the bug ever even existed).  It might be worthwhile to mention the code in question that is overriding a method.

Another thing to try would be to comment out the OnDidChange() line, and see if that removes the flicker.  It's possible that the problem is an interaction between the Cocoa code attempting to update the cell and our code attempting to update the cell.  Maybe someone isn't doing needs-display stuff correctly.

tldr: Probably a subtle AppKit bug. Workaround available.

Preamble: Here's (one way) that text entry works on macOS. The application sticks an NSTextField into the NSView hierarchy. Drawing is done via an NSTextFieldCell. When the user selects the NSTextField, the application delegate overrides windowWillReturnFieldEditor:toObject: to return a NSTextView, which also does custom drawing, but leaves some control in AppKit's hands.

Back in 2003, this was considered "amazingly simple and elegant". Surely there's a better way now?
http://www.cocoabuilder.com/archive/cocoa/81135-nstextfield-nstextfieldcell-nstextview-this-is-mess-need-help.html#81137

Anyhow, in Chrome's omnibox, AutocompleteTextField is the NSTextField subclass, AutocompleteTextField is the NSTextFieldCell subclass, and AutocompleteTextFieldEditor is the NSTextView subclass.

Each time the user types in the omnibox, the callback -[AutocompleteTextFieldEditor interpretKeyEvents:] is called, and eventually calls OmniboxViewMac::ApplyTextStyle. This updates the paragraph style of the attributed string of the NSTextField to have an exact maximumLineHeight and minimumLineHeight of 17, determined by -[AutocompleteTextFieldCell lineHeight]. 

Observation #1:
The frame of AutocompleteTextFieldEditor (set by AppKit) appears to use something along these lines: The x, y, and width are determined from -[AutocompleteTextFieldCell drawingRectForBounds]. The height is set to MAX(drawingRectForBounds.height, maximumLineHeight). 

Observation #2: 
The problem disappears by:
  - Modifying -[AutocompleteTextFieldCell lineHeight] to return 18 instead of 17.
  - Modifying -[AutocompleteTextFieldCell topTextFrameOffset] or -[AutocompleteTextFieldCell bottomTextFrameOffset] to return 2 instead of 3
  - Note that changing the frame of AutocompleteTextFieldEditor directly doesn't fix the problem.

Observation #3: 
Small variations to the size of the font has no effect. Decreasing font size by 4 pts makes the flickering go away.

Observation #4:
Each time the user types in the omnibox, OmniboxPopupViewMac::UpdatePopupAppearance is called at least 3 times. Each call reloads the contents of a NSTableViewController. Even if the NSTableViewController's Window is never shown, the very act of reloading the contents is necessary to create flickering.

Observation #5: Each time a character is entered into the omnibox, layout is invalidated twice. The first by the natural layout process from insertion of character. The second by OmniboxViewMac::ApplyTextStyle. If OmniboxViewMac::ApplyTextStyle() is turned into a no-op, flickering disappears. If OmniboxViewMac::ApplyTextStyle() is modified to only take effect on every 5th call, then text only flickers on every 5th character.

Observation #6:
On macOS 10.12 dp4, -[AutocompleteTextFieldEditor interpretKeyEvents:] is followed by two calls to -[AutocompleteTextFieldEditor drawRect:] on back-to-back frames. In the first call, -[NSTextView drawRect:] draws no characters. The second frame is drawn correctly. On macOS 10.11, there is only one call to -[AutocompleteTextFieldEditor drawRect:].

Observation #7:
I haven't been able to reproduce the flickering in a stand-alone app. Somehow the NSTableViewController from (#4) is relevant.

NSTextView layout and painting is opaque, and complex. Here's what I think is happening: 
  1. -[NSTextView interpretKeyEvents:] invalidates layout. 
  2. observer->OnDidChange() calls OmniboxViewMac::ApplyTextStyle, invalidates layout again. 
  3. Somehow, the new layout parameters in (2) cause AppKit to get in a weird state. 
  4. Some weird interaction with NSTableViewController causes back-to-back repaints, the first of which uses incorrect layout.

Workaround: Force a relayout at the end of -[AutocompleteTextFieldEditor interpretKeyEvents:].

NSTableView is used for the popup contents.  I wonder if there's some screwup in the graphics context tracking or something?  The popup and the field aren't really adjacent these days, but the height observations make me wonder if something on the popup window is overlapping the field (shadow on the popup?  focus ring on the field?).

BTW, I think where you said "2003" you meant "1993", and possibly "1990".

Screwup in graphics context tracking isn't enough to explain the back-to-back calls to drawRect:. There has to be some issue with layout invalidation/generation in NSTextView. The logic there is too hairy for me to want to go digging in with only disassembly.

Note that the problem persists even if the popup window is never shown on screen.

-drawRect: is called within a -lockFocus context, so you should be able to call -drawRect: as much as you want without anything being flushed to the screen.  Also, it's plausible that multiple -display type updates could happen without flushing the window's backing store to the screen.  Why I'm speculating around the table view is because I believe it lives in a child window, so updates to it such as resize could somehow be interfering with the buffering going on.  Like if the system thinks the child window exposed part of the parent window causing an intermediate backing store to be flushed.

Having the popup not being shown argues against that - though it's probably not using deferred backing store so just having it exist and bound as a child window could be sufficient to cause interactions.

<waves arms vigorously/>

-drawRect: is called within a -lockFocus context, so you should be able to call -drawRect: as much as you want without anything being flushed to the screen.  Also, it's plausible that multiple -display type updates could happen without flushing the window's backing store to the screen.  Why I'm speculating around the table view is because I believe it lives in a child window, so updates to it such as resize could somehow be interfering with the buffering going on.  Like if the system thinks the child window exposed part of the parent window causing an intermediate backing store to be flushed.

Having the popup not being shown argues against that - though it's probably not using deferred backing store so just having it exist and bound as a child window could be sufficient to cause interactions.

<waves arms vigorously/>

In the call to -[NSTextView drawRect:] that paints no content, all the NSTextStorage parameters are still correct (string, style, etc.). So there's definitely something wrong with AppKit.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c95767396148abe3e0b7d2ac07262a90dd76a6ae

commit c95767396148abe3e0b7d2ac07262a90dd76a6ae
Author: erikchen <erikchen@chromium.org>
Date: Fri Aug 12 00:52:23 2016

Fix omnibox flickering on macOS Sierra.

The implementation of -[AutocompleteTextFieldEditor interpretKeyEvents:]
invalidates layout twice, in ways that are somehow not compatible on macOS
Sierra. This causes repaints on the next two frames, the first of which does
not paint the text correctly. Forcing an explicit relayout causes there to be
only a single repaint, with the correct contents.

BUG= 634318 

Review-Url: https://codereview.chromium.org/2237003004
Cr-Commit-Position: refs/heads/master@{#411495}

[modify] https://crrev.com/c95767396148abe3e0b7d2ac07262a90dd76a6ae/chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.mm

Your change meets the bar and is auto-approved for M53 (branch: 2785)

erikchen@ - is it possible to give this change at least a little air time on Canary to make sure it doesn't regress anything?

If it is well baked in canary and canary results looks good, please merge your change by Monday (08/15) 5:00 PM PT or latest by Tuesday (08/16) 4:00 PM PT so we can take it in for next week Beta release. Thank you.

I've been triaging Mac bugs, and haven't seen any reported regressions. Merging to M53 now.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0c384764c4aaf6c453f0cf80fb2fdecc9124e472

commit 0c384764c4aaf6c453f0cf80fb2fdecc9124e472
Author: erikchen <erikchen@chromium.org>
Date: Mon Aug 15 18:23:28 2016

[Merge to 2785] Fix omnibox flickering on macOS Sierra.

> The implementation of -[AutocompleteTextFieldEditor interpretKeyEvents:]
> invalidates layout twice, in ways that are somehow not compatible on macOS
> Sierra. This causes repaints on the next two frames, the first of which does
> not paint the text correctly. Forcing an explicit relayout causes there to be
> only a single repaint, with the correct contents.
>
> BUG= 634318 
>
> Review-Url: https://codereview.chromium.org/2237003004
> Cr-Commit-Position: refs/heads/master@{#411495}
> (cherry picked from commit c95767396148abe3e0b7d2ac07262a90dd76a6ae)

Review URL: https://codereview.chromium.org/2241373002 .

Cr-Commit-Position: refs/branch-heads/2785@{#596}
Cr-Branched-From: 68623971be0cfc492a2cb0427d7f478e7b214c24-refs/heads/master@{#403382}

[modify] https://crrev.com/0c384764c4aaf6c453f0cf80fb2fdecc9124e472/chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.mm

Unable to reproduce the issue on Mac 10.11.6 using 53.0.2785.34, 54.0.2817.0(issue reported versions).Please find attached screencast.

Hence, assigning to MTV team as we don't have Mac 10.12.
@MTV: Could anyone look into this issue please.

Deleted: 634318.mp4 391 KB

	634318.mp4 391 KB View Download