Bug 143461 - Text-combine erroneously draws vertically after non-layout-causing style change
Summary: Text-combine erroneously draws vertically after non-layout-causing style change
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-04-06 17:33 PDT by Myles C. Maxfield
Modified: 2015-04-09 15:39 PDT (History)
10 users (show)

See Also:


Attachments
Patch (4.32 KB, patch)
2015-04-06 17:43 PDT, Myles C. Maxfield
darin: review+
Details | Formatted Diff | Diff
Patch for landing (4.22 KB, patch)
2015-04-08 17:41 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Myles C. Maxfield 2015-04-06 17:33:43 PDT
Text-combine erroneously draws vertically after non-layout-causing style change
Comment 1 Myles C. Maxfield 2015-04-06 17:43:38 PDT
Created attachment 250248 [details]
Patch
Comment 2 Myles C. Maxfield 2015-04-06 17:44:34 PDT
<rdar://problem/19285490>
Comment 3 Darin Adler 2015-04-06 18:24:02 PDT
Comment on attachment 250248 [details]
Patch

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

Worth considering finding a less fragile fix.

> Source/WebCore/rendering/RenderCombineText.cpp:46
> +    if (m_isCombined && (!oldStyle || diff > StyleDifferenceRepaintLayer)) {
> +        // Layouts cause the text to be recombined; therefore, only only un-combine when the style diff causes a layout.

It seems like a problem that this logic here about when layout will be caused is here and has to be kept in sync with the code that triggers the layout. Reading the code in RenderText::styleDidChange, that function checks diff == StyleDifferenceLayout so these two are already not matching!

I suggest considering checking selfNeedsLayout() instead, which is more direct. Or maybe something even more direct, doing this work inside the layout logic instead of trying to predict layout here in the styleDidChange function.
Comment 4 Myles C. Maxfield 2015-04-06 18:47:54 PDT
Comment on attachment 250248 [details]
Patch

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

> LayoutTests/fast/text/text-combine-style-change-no-layout-expected.html:6
> +        <div id="word" style="-webkit-text-combine: horizontal; color: red;">39</div>

Red is not good for a passing test.
Comment 5 Myles C. Maxfield 2015-04-08 15:08:59 PDT
Comment on attachment 250248 [details]
Patch

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

>> Source/WebCore/rendering/RenderCombineText.cpp:46
>> +        // Layouts cause the text to be recombined; therefore, only only un-combine when the style diff causes a layout.
> 
> It seems like a problem that this logic here about when layout will be caused is here and has to be kept in sync with the code that triggers the layout. Reading the code in RenderText::styleDidChange, that function checks diff == StyleDifferenceLayout so these two are already not matching!
> 
> I suggest considering checking selfNeedsLayout() instead, which is more direct. Or maybe something even more direct, doing this work inside the layout logic instead of trying to predict layout here in the styleDidChange function.

I think selfNeedsLayout() is the best solution - it unequivocally determines if a subsequent layout will occur.
Comment 6 Myles C. Maxfield 2015-04-08 17:41:17 PDT
Created attachment 250397 [details]
Patch for landing
Comment 7 WebKit Commit Bot 2015-04-09 15:37:53 PDT
Comment on attachment 250397 [details]
Patch for landing

Clearing flags on attachment: 250397

Committed r182609: <http://trac.webkit.org/changeset/182609>