Bug 21937 – h1:first-letter is incorrectly inherited by run-in box

• Component: CSS
• Status: RESOLVED CONFIGURATION CHANGED

[Anantha Keesara]

What steps will reproduce the problem?
1. Open the attached page.

What is the expected result?
A link named "top" 

What happens instead?
A link named "Top" - first letter in "top" should not be in uppercase

FF3: ok
Safari4(526), Chrome154.5: not ok

[Anantha Keesara]

Created attachment 24734 [details]
Testcase

[Robert Blaut]

Created attachment 26290 [details]
same test case as text/html

I found interesting behavior when you compare rendering of test case bug sending as text/html and application/xhtml+xml

[Robert Blaut]

Confirmed as bug in both cases since "Despite appearing visually part of the following block box, a run-in element still inherits properties from its parent in the source tree." [http://www.w3.org/TR/CSS21/visuren.html#run-in]

[Yoshiki Hayashi]

It looks like you just have to skip run-ins when you try to find the block for :first-letter.

The test in the patch I'm going to attach depends on the following bug because otherwise :first-letter applied letter doesn't show up in dumpAsText.

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

[Yoshiki Hayashi]

Created attachment 57313 [details]
Proposed Patch

[Shinichiro Hamaji]

Comment on attachment 57313 [details]
Proposed Patch

Looks good, but putting r- for minor issues.

I've noticed that with the following HTML

<style type="text/css">
  h1:first-letter {text-transform: uppercase;}
</style>
<h1 style="display:run-in;">capitalized</h1><div>downcase</div>

the text "capitalized" isn't capitalized even with this patch. Is this issue related to this bug? Or, should we file another bug to resolve this issue?


I think this patch depends on the patch in Bug 39863 so we should not land this patch without fixing Bug 39863. For such cases, please set commit-queue- to tell reviewers/committers that this patch should not be landed.


LayoutTests/ChangeLog:8
 +          Need a short description and bug URL (OOPS!)
This line should be gone.

WebCore/ChangeLog:8
 +          Need a short description and bug URL (OOPS!)
Unnecessary. Or, you might want to describe something.

LayoutTests/fast/css/first-letter-after-run-in.html:18
 +  <div class="run-in">run-in</div><div class="upper">test</div>
I would say

<div class="run-in">not-capitalized</div><div class="upper">capitalized</div>

or something like this so we can remove the description about the expectation ("If you see...")

[Yoshiki Hayashi]

Thank you for review!

(In reply to comment #6)
> (From update of attachment 57313 [details])
> Looks good, but putting r- for minor issues.
> 
> I've noticed that with the following HTML
> 
> <style type="text/css">
>   h1:first-letter {text-transform: uppercase;}
> </style>
> <h1 style="display:run-in;">capitalized</h1><div>downcase</div>
> 
> the text "capitalized" isn't capitalized even with this patch. Is this issue related to this bug? Or, should we file another bug to resolve this issue?

That's a different bug.  Right now, WebKit doesn't apply first-letter to inline elements.  In this case, the h1 with run-in is converted to an inline element and inserted as the first element of the div so it won't get :first-letter.  The following link shows that div gets :first-letter while span does not.

http://www.plexode.com/cgi-bin/eval3.py#ht=%3Cstyle%3E%0Adiv%3A%3Afirst-letter%20%7B%0A%20%20text-transform%3A%20uppercase%3B%0A%7D%0Aspan%3A%3Afirst-letter%20%7B%0A%20%20text-transform%3A%20uppercase%3B%0A%7D%0A%3C%2Fstyle%3E%0A%0A%3Cdiv%3E%0Atest%0A%3C%2Fdiv%3E%0A%0A%3Cspan%3E%0Atest%0A%3C%2Fspan%3E%0A&ohh=1&ohj=1&jt=&ojh=1&ojj=1&ms=100&oth=0&otj=0&cex=1

> 
> I think this patch depends on the patch in Bug 39863 so we should not land this patch without fixing Bug 39863. For such cases, please set commit-queue- to tell reviewers/committers that this patch should not be landed.
> 
> 
> LayoutTests/ChangeLog:8
>  +          Need a short description and bug URL (OOPS!)
> This line should be gone.
> 
> WebCore/ChangeLog:8
>  +          Need a short description and bug URL (OOPS!)
> Unnecessary. Or, you might want to describe something.

Oops.  Removed.

> LayoutTests/fast/css/first-letter-after-run-in.html:18
>  +  <div class="run-in">run-in</div><div class="upper">test</div>
> I would say
> 
> <div class="run-in">not-capitalized</div><div class="upper">capitalized</div>
> 
> or something like this so we can remove the description about the expectation ("If you see...")

Done.

I also added a test case where run-in stays as a block level element and if :first-letter gets applied to that element.

[Yoshiki Hayashi]

Created attachment 57749 [details]
Proposed Patch

[Shinichiro Hamaji]

Comment on attachment 57749 [details]
Proposed Patch

Looks good. Thanks for your description and updating the patch!

> That's a different bug.  Right now, WebKit doesn't apply first-letter to inline elements.  In this case, the h1 with run-in is converted to an inline element and inserted as the first element of the div so it won't get :first-letter.  The following link shows that div gets :first-letter while span does not.

Ah, I didn't notice run-in box becomes an inline element. It seems the current spec says we don't need to apply ::first-letter for inline elements, so this would be a non-issue.

http://dev.w3.org/csswg/selectors3/#application-in-css

[Nikolas Zimmermann]

Comment on attachment 57749 [details]
Proposed Patch

LayoutTests/fast/css/first-letter-after-run-in-expected.txt:1
 +  not-capitalized
Can you use the script-tests framework, to create a better testcase, that actually tells what you're checkink. Of course it's clear from the testcase, but I think you could do better and explicitely explain what's happening, in the expected-txt output.

Can't review the actual RenderBlock change, you'll need to ask rendering gurus for that (Dave Hyatt / Dan Bernstein..)

[Brent Fulgham]

Safari, Chrome, and Firefox show the same rendering behavior for this test case. I do not believe any further compatibility issue remains.