Bug 137679 – REGRESSION (r169024): Undetermined text is not displayed in the search field of Adobe Help Website

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Alexey Proskuryakov]

Created attachment 239761 [details]
test case

http://trac.webkit.org/changeset/169024 caused a regression. Unconfirmed text appears to sometimes match ::selection rules, but it should not.

Steps to reproduce:

Enable Kotoeri Hiragana input source.
Type anything in the text input above (e.g. "hiragana").
Press a down arrow, so that a drop-down menu appears.
Results: the text disappears.

<rdar://problem/18450335>

[Alexey Proskuryakov]

The actual site is http://helpx.adobe.com/jp/indesign/topics.html

[Alexey Proskuryakov]

Created attachment 239880 [details]
proposed fix

[Myles C. Maxfield]

Comment on attachment 239880 [details]
proposed fix

View in context: https://bugs.webkit.org/attachment.cgi?id=239880&action=review

> Source/WebCore/rendering/InlineTextBox.cpp:538
> +    TextPaintStyle selectionPaintStyle = useCustomUnderlines ? textPaintStyle : computeTextSelectionPaintStyle(textPaintStyle, renderer(), lineStyle, paintInfo, paintSelectedTextOnly, paintSelectedTextSeparately, selectionShadow);

This doesn't make much sense - why should underlines dictate what the paint style of the text above it is? Perhaps we should rename the useCustomUnderlines variable.

Are you sure you don't want to instead pass these variables to computeTextSelectionPaintStyle instead? It seems like the decision whether to use textPaintStyle or something else is ontologically part of the text selection paint style computation.

[Alexey Proskuryakov]

Created attachment 239886 [details]
with release build fix

Turns out that we need to call computeTextSelectionPaintStyle even if we are not going to use the style, as the function also initializes other variables.

> This doesn't make much sense - why should underlines dictate what the paint style of the text above it is? Perhaps we should rename the useCustomUnderlines variable.

Do you have a proposal for a better name? The variable is just a result of calling compositionUsesCustomUnderlines(), so the name seems reasonable to me - it doesn't tell us what the value is supposed to be used for, but it tells us what it is.

> Are you sure you don't want to instead pass these variables to computeTextSelectionPaintStyle instead?

Yes, I considered this, and preferred to keep both useCustomUnderlines checks in the same function. We already use it in paint() like this:

        if (haveSelection && !useCustomUnderlines)
            paintSelection(context, boxOrigin, lineStyle, font, selectionPaintStyle.fillColor);

The logic is indeed somewhat unexpected, so it seems better to have it local in one function, not spread all over the code.

[Myles C. Maxfield]

Unofficial r=me

[Enrica Casucci]

Comment on attachment 239886 [details]
with release build fix

Looks good to me.

[WebKit Commit Bot]

Comment on attachment 239886 [details]
with release build fix

Clearing flags on attachment: 239886

Committed r174807: <http://trac.webkit.org/changeset/174807>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.