|Summary:||REGRESSION (r169024): Undetermined text is not displayed in the search field of Adobe Help Website|
|Product:||WebKit||Reporter:||Alexey Proskuryakov <ap>|
|Component:||HTML Editing||Assignee:||Alexey Proskuryakov <ap>|
|Severity:||Normal||CC:||benjamin, commit-queue, enrica, esprehn+autocc, glenn, kondapallykalyan, mmaxfield, rniwa|
|Version:||528+ (Nightly build)|
Description Alexey Proskuryakov 2014-10-13 17:20:49 PDT
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>
Comment 1 Alexey Proskuryakov 2014-10-13 17:21:23 PDT
The actual site is http://helpx.adobe.com/jp/indesign/topics.html
Comment 2 Alexey Proskuryakov 2014-10-15 11:35:15 PDT
Created attachment 239880 [details] proposed fix
Comment 3 Myles C. Maxfield 2014-10-15 12:27:45 PDT
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.
Comment 4 Alexey Proskuryakov 2014-10-15 13:01:58 PDT
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.
Comment 5 Myles C. Maxfield 2014-10-16 13:03:46 PDT
Comment 6 Enrica Casucci 2014-10-16 15:38:26 PDT
Comment on attachment 239886 [details] with release build fix Looks good to me.
Comment 7 WebKit Commit Bot 2014-10-16 19:17:14 PDT
Comment on attachment 239886 [details] with release build fix Clearing flags on attachment: 239886 Committed r174807: <http://trac.webkit.org/changeset/174807>
Comment 8 WebKit Commit Bot 2014-10-16 19:17:19 PDT
All reviewed patches have been landed. Closing bug.