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> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | benjamin, commit-queue, enrica, esprehn+autocc, glenn, kondapallykalyan, mmaxfield, rniwa | ||||||||
Priority: | P2 | Keywords: | InRadar, Regression | ||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Alexey Proskuryakov
2014-10-13 17:20:49 PDT
The actual site is http://helpx.adobe.com/jp/indesign/topics.html Created attachment 239880 [details]
proposed fix
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. 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. Unofficial r=me Comment on attachment 239886 [details]
with release build fix
Looks good to me.
Comment on attachment 239886 [details] with release build fix Clearing flags on attachment: 239886 Committed r174807: <http://trac.webkit.org/changeset/174807> All reviewed patches have been landed. Closing bug. |