Bug 110195 - [CSS Regions] Region styling (@region) breaks formatting via ::first-letter
Summary: [CSS Regions] Region styling (@region) breaks formatting via ::first-letter
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Takashi Sakamoto
URL:
Keywords:
Depends on:
Blocks: 71487
  Show dependency treegraph
 
Reported: 2013-02-19 02:05 PST by Mihai Balan
Modified: 2022-07-12 17:16 PDT (History)
15 users (show)

See Also:


Attachments
Ref test highlighting the problem (993 bytes, application/x-zip-compressed)
2013-02-19 02:07 PST, Mihai Balan
no flags Details
Patch (11.07 KB, patch)
2013-02-20 03:15 PST, Takashi Sakamoto
no flags Details | Formatted Diff | Diff
Patch (11.29 KB, patch)
2013-02-28 13:15 PST, Takashi Sakamoto
no flags Details | Formatted Diff | Diff
Patch (11.48 KB, patch)
2013-03-05 02:45 PST, Takashi Sakamoto
no flags Details | Formatted Diff | Diff
Patch (10.42 KB, patch)
2013-03-07 01:34 PST, Takashi Sakamoto
no flags Details | Formatted Diff | Diff
Patch (10.42 KB, patch)
2013-03-07 02:13 PST, Takashi Sakamoto
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mihai Balan 2013-02-19 02:05:45 PST
If content is styled both via ::first-letter and then an @region applies to it, too, then the properties set via ::first-letter are discarded.
However, if font-size is specified, the box for the first letter will be the same as if font-size applies, but the actual letter is displayed without the font-size specified in ::first-letter.

See attached ref-test for a clearer picture of the problem (the letter 'M' should be 2.5em tall, italic and monospace; all the text should be green).
Comment 1 Mihai Balan 2013-02-19 02:07:48 PST
Created attachment 189031 [details]
Ref test highlighting the problem
Comment 2 Takashi Sakamoto 2013-02-20 03:15:38 PST
Created attachment 189274 [details]
Patch
Comment 3 Takashi Sakamoto 2013-02-28 13:15:32 PST
Created attachment 190792 [details]
Patch
Comment 4 WebKit Review Bot 2013-02-28 14:15:35 PST
Comment on attachment 190792 [details]
Patch

Attachment 190792 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/16775977

New failing tests:
css3/flexbox/inline-flex-crash2.html
css3/flexbox/flexbox-ignore-container-firstLetter.html
css3/flexbox/flexbox-ignore-firstLetter.html
css3/flexbox/inline-flex-crash.html
Comment 5 Build Bot 2013-02-28 15:28:59 PST
Comment on attachment 190792 [details]
Patch

Attachment 190792 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/16672719
Comment 6 Build Bot 2013-02-28 22:13:26 PST
Comment on attachment 190792 [details]
Patch

Attachment 190792 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/16766317

New failing tests:
css3/flexbox/inline-flex-crash2.html
css3/flexbox/flexbox-ignore-container-firstLetter.html
css3/flexbox/flexbox-ignore-firstLetter.html
css3/flexbox/inline-flex-crash.html
Comment 7 Takashi Sakamoto 2013-03-05 02:45:40 PST
Created attachment 191446 [details]
Patch
Comment 8 Mihnea Ovidenie 2013-03-05 09:52:44 PST
Comment on attachment 191446 [details]
Patch

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

> Source/WebCore/ChangeLog:1
> +2013-02-20  Takashi Sakamoto  <tasak@google.com>

This is an informal review, please feel free to ignore it.

> Source/WebCore/ChangeLog:8
> +        While RenderRegion updates styles, it should consider the case,

In fact, you are talking about RenderRegion computing the styles in region for elements that are displayed inside the region.

> Source/WebCore/ChangeLog:17
> +        Added one more parameter, regionForStyling, to see a pseudo style in

see = set

> Source/WebCore/ChangeLog:20
> +        (StyleResolver):

You can get rid of lines like this in Changelogs.

> Source/WebCore/ChangeLog:26
> +        (WebCore):

I would rather put the method on the RenderBlock class itself rather than put it in the header file.

> Source/WebCore/rendering/RenderRegion.cpp:533
> +{

Please add an ASSERT here that your object is really an anonymous one.

> Source/WebCore/rendering/RenderRegion.cpp:536
> +    RenderStyle* parentStyle;

If you do: RenderStyle* parentStyle = parent->style(), you may get rid of the else statement.

> LayoutTests/fast/regions/pseudo-first-letter-inside-flowthread.html:28
> +<span>Morem ipsum dolor sit amet, consectetur adipisicing elit. Assumenda ipsam rem fugit accusantium fugiat ea.</span>

Should be Lorem ... here right? :) I mean L instead of M. Or you can as well put a meaningful text, like what is this test for (I would also like to have a reference to the bug you are trying to solve).
Comment 9 Takashi Sakamoto 2013-03-07 01:34:37 PST
Created attachment 191947 [details]
Patch
Comment 10 Takashi Sakamoto 2013-03-07 02:13:18 PST
Created attachment 191953 [details]
Patch
Comment 11 Takashi Sakamoto 2013-03-07 02:15:48 PST
Comment on attachment 191446 [details]
Patch

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

>> Source/WebCore/ChangeLog:8
>> +        While RenderRegion updates styles, it should consider the case,
> 
> In fact, you are talking about RenderRegion computing the styles in region for elements that are displayed inside the region.

I changed the description.

>> Source/WebCore/ChangeLog:17
>> +        Added one more parameter, regionForStyling, to see a pseudo style in
> 
> see = set

Done.

>> Source/WebCore/ChangeLog:26
>> +        (WebCore):
> 
> I would rather put the method on the RenderBlock class itself rather than put it in the header file.

I see. Done.

>> Source/WebCore/rendering/RenderRegion.cpp:533
>> +{
> 
> Please add an ASSERT here that your object is really an anonymous one.

I see. Done.

>> Source/WebCore/rendering/RenderRegion.cpp:536
>> +    RenderStyle* parentStyle;
> 
> If you do: RenderStyle* parentStyle = parent->style(), you may get rid of the else statement.

Done.

>> LayoutTests/fast/regions/pseudo-first-letter-inside-flowthread.html:28
>> +<span>Morem ipsum dolor sit amet, consectetur adipisicing elit. Assumenda ipsam rem fugit accusantium fugiat ea.</span>
> 
> Should be Lorem ... here right? :) I mean L instead of M. Or you can as well put a meaningful text, like what is this test for (I would also like to have a reference to the bug you are trying to solve).

I see. Done.
Comment 12 Elliott Sprehn 2013-03-21 00:19:27 PDT
Comment on attachment 191953 [details]
Patch

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

> Source/WebCore/rendering/RenderRegion.cpp:540
> +        const PseudoStyleRequest pseudoStyleRequest(FIRST_LETTER);

Scope these correctly inside the inner if.

> Source/WebCore/rendering/RenderRegion.cpp:541
> +        parentStyle = parent->firstLineStyle();

This will override the parentStyle if you fall through this if into the createAnonymousStyleWithDisplay below, is that intended? Seems safer to just use parent->style() in each place and not have this local.

> Source/WebCore/rendering/RenderRegion.cpp:546
> +                return styleInRegion.release();

Is it actually possible for this to be null? How would we have a first-letter render object but no style?
Comment 13 Michelangelo De Simone 2013-06-13 11:34:14 PDT
Still repros on today's nightly (r151543)
Comment 14 Andreas Kling 2014-02-05 11:19:48 PST
Comment on attachment 191953 [details]
Patch

Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.
Comment 15 Brent Fulgham 2022-07-12 17:16:55 PDT
CSS Regions were removed in Bug 174978.