Bug 133744 – The computed value of line-height:normal is incorrect

• Component: CSS
• Status: RESOLVED FIXED

[Sylvain Galineau]

REPRO:

1. Load the attached HTML file
2. Open the JS console
3. Reported computed value of line-height for test paragraph is 'normal'

EXPECTED:
Per CSSOM [1], should be a pixel lengths i.e. "19.2px" given font-size:16px and a normal line-height of 1.2.

This works as expected in Gecko.

[1] http://dev.w3.org/csswg/cssom/#resolved-value

[Sylvain Galineau]

Created attachment 243594 [details]
fix.patch

IE, Gecko and Trident compute line-height: normal to a length.

The attached WIP fixes the issue and updates the related tests.

Two possible test issues:

1. fast/ruby/ruby-line-height.html: I fixed up the test to reflect the new pass values but I'm not sure the test was valid to begin with...
2. platform/mac/editing/attributed-string/* changes: no sure whether it is correct to update the mac-mavericks, mac, etc. versions to the same expected result?

[Simon Fraser (smfr)]

Comment on attachment 243594 [details]
fix.patch

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

> LayoutTests/ChangeLog:9
> +        LayoutTests updates
> +
> +        line-height:normal now computes to a lengh value
> +        This patch updates the tests to reflect the new computed value, as needed
> +        https://bugs.webkit.org/show_bug.cgi?id=133744
> +
> +        Reviewed by NOBODY (OOPS!).

Format should be:

https://bugs.webkit.org/show_bug.cgi?id=133744

Reviewed by NOBODY (OOPS!).

[Explanatory text]

> LayoutTests/platform/mac/editing/attributed-string/font-size-expected.txt:53
> +NSParagraphStyle:
> +Alignment 4
> +    LineSpacing: 0
> +    ParagraphSpacing: 0
> +    ParagraphSpacingBefore: 0
> +    HeadIndent: 0
> +    TailIndent: 0
> +    FirstLineHeadIndent: 0
> +    LineHeight: 18/0
> +    LineHeightMultiple: 0
> +    LineBreakMode: 0
> +    Tabs: ()
> +    DefaultTabInterval: 36
> +    Blocks: (null)
> +    Lists: (null)
> +    BaseWritingDirection: 0
> +    HyphenationFactor: 0
> +    TighteningFactor: 0.05
> +    HeaderLevel: 0

Why does all this new output show up?

> Source/WebCore/ChangeLog:8
> +        Use font's line spacing as computed line-height value when specified value is 'normal'
> +        This behavior is compatible with all other engines.
> +        
> +        https://bugs.webkit.org/show_bug.cgi?id=133744
> +
> +        Reviewed by NOBODY (OOPS!).

Wrong format. Use prepare-Changelog (or webkit-patch) to get the right one.

[Sylvain Galineau]

Regarding the ChangeLog format, I followed what's there. See most recent changes in the log for 140163, 138139 etc. Happy to change it but it wasn't clear, FWIW.

I'll dig into the reason for this new output. I suspect only values differing from a baseline are being dumped and the change is triggering this.

[Sylvain Galineau]

Created attachment 244270 [details]
Patch update

My bad on the ChangeLog; I read your comment then managed to look at my (correct) ChangeLog for another patch. Definitely did it wrong on this one. I have rebased and fixed the patch accordingly.

Regarding all the additional output in font-size-expected.txt: I believe this to be correct.

The cause lies in platform/*/editing/attributed-string/resources/dump-attributed-string.js. The code there will dump an NSParagraphStyle if it is different from the currentParagraphStyle. In the past, LineHeight always computed to 'normal' and dumped as '0/0' so this caused no variation from one paragraph to the next. Now, LineHeight is an actual value that changes with every paragraph so each and every one causes a paragraph style dump. 

I think we could maybe improve this logic to only dump the bits of a paragraph style that are different vs. the whole thing though that will invalidate a bunch of other tests...

[Dean Jackson]

Comment on attachment 244270 [details]
Patch update

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

> LayoutTests/ChangeLog:14166
> +        line-height:normal now computes to a lengh value
> +        This patch updates the tests to reflect the new computed value, as needed

Typo: length

Also, we try to end all sentences with a full stop, even in comments and change logs. The other ChangeLog has the same.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1515
> +    if (length.isNegative()) // If true, line-height not set; use the font's line spacing

Nit: missing full stop/period.

[Dean Jackson]

R=me, but I want to run EWS because I'm wondering if the platform results for OS X won't work on Yosemite.

[Darin Adler]

Comment on attachment 244270 [details]
Patch update

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

> LayoutTests/ChangeLog:14199
> +        * platform/mac-mavericks/platform/mac/editing/attributed-string/anchor-element-expected.txt:
> +        * platform/mac-mavericks/platform/mac/editing/attributed-string/basic-expected.txt:
> +        * platform/mac-mavericks/platform/mac/editing/attributed-string/font-size-expected.txt:
> +        * platform/mac-mavericks/platform/mac/editing/attributed-string/font-style-variant-effect-expected.txt:
> +        * platform/mac-mavericks/platform/mac/editing/attributed-string/font-weight-expected.txt:
> +        * platform/mac-mavericks/platform/mac/editing/attributed-string/letter-spacing-expected.txt:
> +        * platform/mac-mavericks/platform/mac/editing/attributed-string/text-decorations-expected.txt:
> +        * platform/mac-mavericks/platform/mac/editing/attributed-string/vertical-align-expected.txt:
> +        * platform/mac/editing/attributed-string/anchor-element-expected.txt:
> +        * platform/mac/editing/attributed-string/basic-expected.txt:
> +        * platform/mac/editing/attributed-string/font-size-expected.txt:
> +        * platform/mac/editing/attributed-string/font-style-variant-effect-expected.txt:
> +        * platform/mac/editing/attributed-string/font-weight-expected.txt:
> +        * platform/mac/editing/attributed-string/letter-spacing-expected.txt:
> +        * platform/mac/editing/attributed-string/text-decorations-expected.txt:
> +        * platform/mac/editing/attributed-string/vertical-align-expected.txt:

I’m really concerned about these test result changes. Are we OK with this change in editing behavior? Who OK’d this aspect of the patch?

[Darin Adler]

Comment on attachment 244270 [details]
Patch update

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

>> LayoutTests/ChangeLog:14199
>> +        * platform/mac/editing/attributed-string/vertical-align-expected.txt:
> 
> I’m really concerned about these test result changes. Are we OK with this change in editing behavior? Who OK’d this aspect of the patch?

OK, now I see that these are explained by a quirk in the string dumping code. It would be nice to make this test output more readable. I hope there really is no behavior change here.

[Sylvain Galineau]

Created attachment 244714 [details]
Patch update

I have updated the patch to fix the nits noted by Dean.

I agree it would be nice to fix this test script to streamline the ouput e.g. by only dumping which paragraph styles are changed. I am a bit short on time at the moment - about to leave work for 3 weeks - but I have filed https://bugs.webkit.org/show_bug.cgi?id=140514 and assigned it to myself to deal with this in a month or so.

[Dean Jackson]

Committed r179168: <http://trac.webkit.org/changeset/179168>

[Alexey Proskuryakov]

This broke tests on Mac:

https://build.webkit.org/builders/Apple%20Yosemite%20Release%20WK1%20(Tests)/builds/2319

[Chris Dumez]

(In reply to comment #11)
> This broke tests on Mac:
> 
> https://build.webkit.org/builders/Apple%20Yosemite%20Release%20WK1%20(Tests)/
> builds/2319

Based on the types of failures, it does not look like a bug. Likely the tests just need to be updated / rebaselined.

[Alexey Proskuryakov]

Chris, would you be willing to do that?

[Chris Dumez]

(In reply to comment #13)
> Chris, would you be willing to do that?

Rebaselined the tests for Mac in <http://trac.webkit.org/changeset/179174>. We'll need a rebaseline for fast/css/css2-system-fonts.html on iOS though.

[Alexey Proskuryakov]

That fixed two tests, what about the other two?

platform/mac/editing/attributed-string/font-size.html
platform/mac/editing/attributed-string/vertical-align.html

[Chris Dumez]

(In reply to comment #15)
> That fixed two tests, what about the other two?
> 
> platform/mac/editing/attributed-string/font-size.html
> platform/mac/editing/attributed-string/vertical-align.html

Sorry, I only saw 2 failures on the bot I was checking last night so I missed those. I'll take a look at these now.

[Chris Dumez]

(In reply to comment #16)
> (In reply to comment #15)
> > That fixed two tests, what about the other two?
> > 
> > platform/mac/editing/attributed-string/font-size.html
> > platform/mac/editing/attributed-string/vertical-align.html
> 
> Sorry, I only saw 2 failures on the bot I was checking last night so I
> missed those. I'll take a look at these now.

More rebaseline in <http://trac.webkit.org/changeset/179189>.

[Sylvain Galineau]

I'm sorry for the test issues :( Thanks for dealing with it.

I did rebaseline the test results for:
platform/mac/editing/attributed-string/font-size.html
platform/mac/editing/attributed-string/vertical-align.html

...in the currently attached patch. Was there something wrong with it?

On the other hand, these tests:

fast/css/font-shorthand-line-height-expected.txt:
fast/css/font-shorthand-line-height.html:
platform/mac-mavericks/fast/css/css2-system-fonts-expected.txt:
platform/mac/fast/css/css2-system-fonts-expected.txt:

Did not fail for me. I just rebased and re-reran Tools/Scripts/run-webkit-tests and they remain no-shows. Any thoughts why that might be?

[Chris Dumez]

(In reply to comment #18)
> I'm sorry for the test issues :( Thanks for dealing with it.
> 
> I did rebaseline the test results for:
> platform/mac/editing/attributed-string/font-size.html
> platform/mac/editing/attributed-string/vertical-align.html
> 
> ...in the currently attached patch. Was there something wrong with it?

We had 2 different expectations for these tests, one for Mavericks (that you updated) and another for Yosemite (which wasn't updated). This one was had to find and would have passed EWS / CommitQueue fine.

> 
> On the other hand, these tests:
> 
> fast/css/font-shorthand-line-height-expected.txt:
> fast/css/font-shorthand-line-height.html:
> platform/mac-mavericks/fast/css/css2-system-fonts-expected.txt:
> platform/mac/fast/css/css2-system-fonts-expected.txt:
> 
> Did not fail for me. I just rebased and re-reran
> Tools/Scripts/run-webkit-tests and they remain no-shows. Any thoughts why
> that might be?

I introduced those tests very recently which is why your patch probably did not include rebaselines for those. Likely, at the time of writing / testing your patch, those tests did not exist.

[Sylvain Galineau]

>> ...
>> Did not fail for me. I just rebased and re-reran
>> Tools/Scripts/run-webkit-tests and they remain no-shows. Any thoughts why
>> that might be?

> I introduced those tests very recently which is why your patch probably did not 
> include rebaselines for those. Likely, at the time of writing / testing your patch,
> those tests did not exist.

Ah, check. It also looks like by the time I rebased my local repo I also got your *-expected.txt updates. All good.

Thanks again.

[Ryosuke Niwa]

This patch caused the bug 146492.