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
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?
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.
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.
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...
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.
R=me, but I want to run EWS because I'm wondering if the platform results for OS X won't work on Yosemite.
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?
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.
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.
Committed r179168: <http://trac.webkit.org/changeset/179168>
This broke tests on Mac: https://build.webkit.org/builders/Apple%20Yosemite%20Release%20WK1%20(Tests)/builds/2319
(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.
Chris, would you be willing to do that?
(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.
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
(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.
(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>.
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?
(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.
>> ... >> 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.
This patch caused the bug 146492.