Bug 138970 – [iOS] Regression(r176202): line-height is wrong on marco.org

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Chris Dumez]

line-height is wrong on marco.org after r176202, on iOS.

Radar: <rdar://problem/19039387>

[Chris Dumez]

Created attachment 242052 [details]
Patch

[Chris Dumez]

For reference, this was the code before:
https://bugs.webkit.org/attachment.cgi?id=241714&action=review

[Simon Fraser (smfr)]

Comment on attachment 242052 [details]
Patch

Please add a layout test.

[Chris Dumez]

Created attachment 242065 [details]
Patch

[Chris Dumez]

@dbates: Is there a way I can explicitly NOT skip my new test on iOS (even though fast/css is skipped)? For e.g. Should I mark this specific test as [ Pass ] in the iphonesimulator TestExpectations file?

[Myles C. Maxfield]

Yes. For an example, see LayoutTests/platform/mac/TestExpectations in https://bugs.webkit.org/attachment.cgi?id=242061&action=review

[Chris Dumez]

Created attachment 242070 [details]
Patch

[Chris Dumez]

(In reply to comment #6)
> Yes. For an example, see LayoutTests/platform/mac/TestExpectations in
> https://bugs.webkit.org/attachment.cgi?id=242061&action=review

Ok, done.

[Simon Fraser (smfr)]

Comment on attachment 242070 [details]
Patch

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

> Source/WebCore/ChangeLog:23
> +        No new tests, iOS specific. Verified manually on marco.org
> +        using an iPhone 6 Plus.

This is a lie!

> Source/WebCore/css/StyleBuilderCustom.h:518
> +template <ApplyTextSizeMultiplierOrNot applyTextMultiplier>
>  inline bool convertLineHeight(StyleResolver& styleResolver, const CSSValue& value, Length& length, float multiplier = 1.f)

Seems bloaty to generate 2 copies of this mostly-identical code. Why not just pass in a param, or just pass in styleResolver.style() or just get it from styleResolver?

[Chris Dumez]

Created attachment 242079 [details]
Patch

[Myles C. Maxfield]

Can't we make a real test? Perhaps that two lines with AHEM with a particular line height is the same as the same two lines except with a third blank line in between them?

[Chris Dumez]

(In reply to comment #11)
> Can't we make a real test? Perhaps that two lines with AHEM with a
> particular line height is the same as the same two lines except with a third
> blank line in between them?

The bug is in the CSS StyleBuilder, I am testing the StyleBuilder.

[Chris Dumez]

Created attachment 242081 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 242081 [details]
Patch

Clearing flags on attachment: 242081

Committed r176490: <http://trac.webkit.org/changeset/176490>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.